Always call StorageMonitor::RemoveObserver in ~MediaFileSystemRegistry.

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

[fdoray, 2017-01-26 16:01:09]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 16:02:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 16:28:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 16:28:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-01-26 21:03:14]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 21:03:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 21:53:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 21:53:55]

Dry run: This issue passed the CQ dry run.

[fdoray, 2017-01-26 22:15:38]

fdoray@chromium.org changed reviewers:
+ reillyg@chromium.org

[fdoray, 2017-01-26 22:15:38]

PTAL

[fdoray, 2017-01-26 22:17:10]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 22:17:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2017-01-26 22:19:33]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 22:19:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 23:09:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 23:09:51]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2017-01-27 02:09:41]

lgtm

[fdoray, 2017-01-27 03:18:38]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-01-27 03:19:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 03:24:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485487118717160,
"parent_rev": "d8e6e751f1e8982648cb06bf74889544a5733d04", "commit_rev":
"4f5235b9f2a3d151c85f497e9963809f11fe7bac"}

[commit-bot: I haz the power, 2017-01-27 03:25:08]

Description was changed from

==========
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
==========

to

==========
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/+/4f5235b9f2a3d151c85f497e9963...
==========

[commit-bot: I haz the power, 2017-01-27 03:25:09]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4f5235b9f2a3d151c85f497e9963...