Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2654303002: Always call StorageMonitor::RemoveObserver in ~MediaFileSystemRegistry. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 3 weeks ago by fdoray
Modified:
4 months, 3 weeks ago
CC:
chromium-reviews, Lei Zhang, tommycli
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Always call StorageMonitor::RemoveObserver in ~MediaFileSystemRegistry. To make ObserverListThreadSafe TaskScheduler-friendly, we plan to dispatch notifications to observers in separate tasks (previously, observers registered from the same thread were notified in the same task). This change causes a use-after-free in GalleryWatchManagerTest.TestStorageRemovedAfterProfileShutdown: 1. StorageMonitor sends a notification to its observers. 2. The notification is dispatched to GalleryWatchManager, which requests the loop to exit after the current task (ConfirmWatch). 3. StorageMonitor is destroyed. 4. MediaFileSystemRegistry is destroyed. Because StorageMonitor has already been destroyed, it doesn't call StorageMonitor::RemoveObserver. 5. TestBrowserThreadBundle is destroyed. In its destructor, it dispatches the notification to the deleted MediaFileSystemRegistry, which causes a use-after-free. When observers registered from the same thread are notified from the same task, MediaFileSystemRegistry is notified before the loop exits (and hence no notification is dispatched to a deleted MediaFileSystemRegistry in step 5). This CL moves step 4 before step 3. This allows MediaFileSystemRegistry to call StorageMonitor::RemoveObserver from its destructor and prevents it from receiving notifications after being destroyed. BUG=675631 Review-Url: https://codereview.chromium.org/2654303002 Cr-Commit-Position: refs/heads/master@{#446569} Committed: https://chromium.googlesource.com/chromium/src/+/4f5235b9f2a3d151c85f497e9963809f11fe7bac

Patch Set 1 #

Patch Set 2 : fix test error #

Patch Set 3 : add DCHECK #

Patch Set 4 : add include #

Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -12 lines) Patch
M chrome/browser/media_galleries/gallery_watch_manager.cc View 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/media_galleries/gallery_watch_manager_unittest.cc View 2 chunks +11 lines, -0 lines 0 comments Download
M chrome/browser/media_galleries/media_file_system_registry.cc View 2 chunks +3 lines, -5 lines 0 comments Download
M chrome/browser/media_galleries/media_file_system_registry_unittest.cc View 1 1 chunk +10 lines, -1 line 0 comments Download
M chrome/browser/media_galleries/media_galleries_permission_controller.cc View 1 2 3 3 chunks +5 lines, -4 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 22 (18 generated)
fdoray
PTAL
4 months, 3 weeks ago (2017-01-26 22:15:38 UTC) #10
Reilly Grant (use Gerrit)
lgtm
4 months, 3 weeks ago (2017-01-27 02:09:41 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2654303002/60001
4 months, 3 weeks ago (2017-01-27 03:19:02 UTC) #19
commit-bot: I haz the power
4 months, 3 weeks ago (2017-01-27 03:25:09 UTC) #22
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4f5235b9f2a3d151c85f497e9963...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318