New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 596348 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Long OOO (go/where-is-mgiuca)
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug

Blocked on:
issue 590882

Blocking:
issue 742145



Sign in to add a comment

UpdateShortcutWorker thread-unsafe use of gfx::Image

Project Member Reported by mgiuca@chromium.org, Mar 21 2016

Issue description

Version: 51 (goes back approximately forever)
OS: Windows

UpdateShortcutWorker (in update_shortcut_worker_win.h) gets a favicon from a NavigationEntryImpl and then passes it to another thread, which calls ToImageSkia on it. Images are not thread safe. This can trigger a CHECK-fail in Image::AddRepresentation if ToImageSkia is called on the image simultaneously from two threads.

Continuing research from  Issue 590882 .
 
Blocking: 600229

Comment 2 by mgiuca@chromium.org, Jul 18 2017

Blocking: 742145

Comment 3 by mgiuca@chromium.org, Jul 19 2017

Blocking: -600229

Comment 4 by mgiuca@chromium.org, Jul 21 2017

Cc: tzik@chromium.org tapted@chromium.org mgiuca@chromium.org rsesek@chromium.org
 Issue 742145  has been merged into this issue.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e6cf2c3cd8720693a2b07744ccf9d42cda14f423

commit e6cf2c3cd8720693a2b07744ccf9d42cda14f423
Author: Matt Giuca <mgiuca@chromium.org>
Date: Fri Aug 11 05:07:47 2017

web_app: Copy the favicon Image to avoid thread safety violation.

GetShortcutInfoForTab creates a new Image, ensuring that ShortcutInfo's
favicon has a separate backing store to the WebContents' favicon.

This fixes a race condition where the Image is shared between the UI
thread (used to render the favicon) and the FILE thread (used to write
the icon to disk). It does not actually copy the pixel data, just the
reference to the ImageSkia.

Bug:  596348 
Change-Id: I5b7591664dbabd4350fb625280cee3ddb3227733
Reviewed-on: https://chromium-review.googlesource.com/578915
Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493672}
[modify] https://crrev.com/e6cf2c3cd8720693a2b07744ccf9d42cda14f423/chrome/browser/web_applications/update_shortcut_worker_win.cc
[modify] https://crrev.com/e6cf2c3cd8720693a2b07744ccf9d42cda14f423/chrome/browser/web_applications/web_app.cc
[modify] https://crrev.com/e6cf2c3cd8720693a2b07744ccf9d42cda14f423/chrome/browser/web_applications/web_app.h

Comment 6 by mgiuca@chromium.org, Aug 11 2017

Status: Fixed (was: Started)
Yay this should be fixed now.

Sign in to add a comment