SnapshotCache access de-allocated objects during tests |
|||||||
Issue descriptionSnapshotCache can access the TaskScheduler global instance after it has been destroyed. The TaskScheduler global instance is reset between tests (it can be initialised by different scoped objects like TestWebThreadBundle or ScopedTaskEnvironment). SnapshotCache access a local leaky singleton of type base::SequencedTaskRunner (via g_snapshot_task_runner variable). This object takes a pointer to TaskScheduler::GetInstance() to post tasks. This pointer becomes invalid when the next test is run. Changing the base::SequencedTaskRunner to be owned by SnapshotCache won't fix the problem because SnapshotCache is a singleton (accessed through SnapshotCache +sharedInstance). Options are: 1. use WebThread::GetBlockingPool() to post the task, but this mean losing the priority trait (base::TaskPriority::USER_VISIBLE) and could lead to snapshot appearing with a delay, 2. change SnapshotCache to not be a singleton but pass it to the different objects that need to use it, and have SnapshotCache owns the base::SequencedTaskRunner. As mentioned earlier, going with 1. mean losing the priority trait and can lead to visual artifacts. Going with 2. can be difficult as SnapshotCache is accessed from TabModel, SideSwipeController, SnapshotManager and MainController (only called from -purgeSnapshots after the TabModels have been created). This mean the object should probably be owned by the non-incognito ChromeBrowserState as a KeyedService. As all those objects have access to the ChromeBrowserState directly or via a WebState, it would work.
,
May 30 2017
It looks like the code was changed from using the BlockingPool in favor of using a local base::SequencedTaskRunner as part of issue #667892 , so option 1. is not viable. Need to implement option 2., i.e. change SnapshotCache to not be a singleton.
,
May 30 2017
,
May 30 2017
Option 3 is to add a ClearTaskRunnerForTesting() method that is called at the end of tests that use the SnapshotCache. The TaskRunner would be recreated (with a pointer to the current TaskScheduler instance) the first time it's accessed after a call to ClearTaskRunnerForTesting().
,
May 30 2017
The issue is that it is hard to know which tests uses SnapshotCache as it is a singleton. Any test that access any of the objects mentioned in #1 can potentially indirectly access to SnapshotCache. And any test written in the future would have to be "fixed" by adding this call. I don't think this is sustainable. We've discussed this in our team and decided to go with option 2. I estimate that this will take one or two days as the class has to be converted to C++ (or at least a C++ wrapper needs to be written because KeyedService is a C++ class).
,
May 30 2017
Sylvain, please fill Component field if you Assigning the bug.
,
May 31 2017
,
May 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2046b091ea5f80064c33054b36fcc0fd261aa0c5 commit 2046b091ea5f80064c33054b36fcc0fd261aa0c5 Author: Sylvain Defresne <sdefresne@chromium.org> Date: Wed May 31 19:08:45 2017 [ios] Cleanup of SnapshotCache implementation. In preparation of converting SnapshotCache to C++ and not accessing it as a singleton, refactor its implementation to remove duplicated methods, access of the singleton in helper methods, ... BUG= 727597 Change-Id: I29ab42007ad651f84e9967477d3ab7a9db44e332 Reviewed-on: https://chromium-review.googlesource.com/519387 Reviewed-by: Jean-François Geyelin <jif@chromium.org> Commit-Queue: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/heads/master@{#475971} [modify] https://crrev.com/2046b091ea5f80064c33054b36fcc0fd261aa0c5/ios/chrome/browser/snapshots/snapshot_cache.h [modify] https://crrev.com/2046b091ea5f80064c33054b36fcc0fd261aa0c5/ios/chrome/browser/snapshots/snapshot_cache.mm [modify] https://crrev.com/2046b091ea5f80064c33054b36fcc0fd261aa0c5/ios/chrome/browser/snapshots/snapshot_cache_internal.h [modify] https://crrev.com/2046b091ea5f80064c33054b36fcc0fd261aa0c5/ios/chrome/browser/snapshots/snapshot_cache_unittest.mm [modify] https://crrev.com/2046b091ea5f80064c33054b36fcc0fd261aa0c5/ios/chrome/browser/snapshots/snapshot_manager.mm
,
Jun 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7 commit 539a7e7305ec7d65e13d1b9f2a125d340f4b31b7 Author: Sylvain Defresne <sdefresne@chromium.org> Date: Thu Jun 08 11:07:15 2017 [ios] Remove use of blocking pool from SnapshotCache. Add a SnapshotCacheFactory to tie the SnapshotCache lifetime to that of ChromeBrowserState and change client code to access it via the factory. Use a local base::SequencedTaskRunner to post task in background instead of using the blocking pool. As the base::SequencedTaskRunner keep a reference to base::TaskScheduler, the prevents SnapshotCache from being a singleton (as it would outlive base::TaskScheduler). Use a SEQUENCE_CHECKER to ensure the calls are made on the correct sequence (instead on checking the current thread). Fix unit tests to wait for TaskScheduler to become idle instead of the blocking pool. BUG= 727597 , 667892 Change-Id: Ib4295fb714298e72baaea7d1408fdb52ccf4075f Reviewed-on: https://chromium-review.googlesource.com/527115 Reviewed-by: Jean-François Geyelin <jif@chromium.org> Commit-Queue: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/heads/master@{#477939} [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/app/main_controller.mm [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/browser_state/BUILD.gn [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/snapshots/BUILD.gn [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/snapshots/snapshot_cache.h [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/snapshots/snapshot_cache.mm [add] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/snapshots/snapshot_cache_factory.h [add] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/snapshots/snapshot_cache_factory.mm [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/snapshots/snapshot_cache_unittest.mm [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/snapshots/snapshot_manager.h [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/snapshots/snapshot_manager.mm [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/tabs/tab.mm [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/tabs/tab_model.mm [modify] https://crrev.com/539a7e7305ec7d65e13d1b9f2a125d340f4b31b7/ios/chrome/browser/ui/side_swipe/side_swipe_controller.mm
,
Jun 8 2017
,
Jun 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d2d5872bcd1cef4b18fd331419562a44c15a86b commit 4d2d5872bcd1cef4b18fd331419562a44c15a86b Author: Louis Romero <lpromero@chromium.org> Date: Thu Jun 08 13:10:42 2017 Revert "[ios] Remove use of blocking pool from SnapshotCache." This reverts commit 539a7e7305ec7d65e13d1b9f2a125d340f4b31b7. Reason for revert: This broke the iOS bot on the Chromium waterfall: https://uberchromegw.corp.google.com/i/chromium.fyi/builders/ios-simulator/builds/6286 Original change's description: > [ios] Remove use of blocking pool from SnapshotCache. > > Add a SnapshotCacheFactory to tie the SnapshotCache lifetime to that > of ChromeBrowserState and change client code to access it via the > factory. > > Use a local base::SequencedTaskRunner to post task in background > instead of using the blocking pool. As the base::SequencedTaskRunner > keep a reference to base::TaskScheduler, the prevents SnapshotCache > from being a singleton (as it would outlive base::TaskScheduler). > > Use a SEQUENCE_CHECKER to ensure the calls are made on the correct > sequence (instead on checking the current thread). > > Fix unit tests to wait for TaskScheduler to become idle instead of > the blocking pool. > > BUG= 727597 , 667892 > > Change-Id: Ib4295fb714298e72baaea7d1408fdb52ccf4075f > Reviewed-on: https://chromium-review.googlesource.com/527115 > Reviewed-by: Jean-François Geyelin <jif@chromium.org> > Commit-Queue: Sylvain Defresne <sdefresne@chromium.org> > Cr-Commit-Position: refs/heads/master@{#477939} TBR=jif@chromium.org,sdefresne@chromium.org No-Presubmit: true No-Tree-Checks: true No-Try: true BUG= 727597 , 667892 Change-Id: I8eb2ec27ac97e20ed3bde0babf63b95f12b0be26 Reviewed-on: https://chromium-review.googlesource.com/528096 Reviewed-by: Louis Romero <lpromero@chromium.org> Commit-Queue: Louis Romero <lpromero@chromium.org> Cr-Commit-Position: refs/heads/master@{#477950} [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/app/main_controller.mm [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/browser/browser_state/BUILD.gn [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/browser/snapshots/BUILD.gn [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/browser/snapshots/snapshot_cache.h [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/browser/snapshots/snapshot_cache.mm [delete] https://crrev.com/61a28216c8afa4d39dedb3dda02c179cf22d5f63/ios/chrome/browser/snapshots/snapshot_cache_factory.h [delete] https://crrev.com/61a28216c8afa4d39dedb3dda02c179cf22d5f63/ios/chrome/browser/snapshots/snapshot_cache_factory.mm [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/browser/snapshots/snapshot_cache_unittest.mm [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/browser/snapshots/snapshot_manager.h [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/browser/snapshots/snapshot_manager.mm [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/browser/tabs/tab.mm [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/browser/tabs/tab_model.mm [modify] https://crrev.com/4d2d5872bcd1cef4b18fd331419562a44c15a86b/ios/chrome/browser/ui/side_swipe/side_swipe_controller.mm
,
Jun 8 2017
,
Jun 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe commit 528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe Author: Sylvain Defresne <sdefresne@chromium.org> Date: Thu Jun 08 16:26:07 2017 [ios] Remove use of blocking pool from SnapshotCache. Add a SnapshotCacheFactory to tie the SnapshotCache lifetime to that of ChromeBrowserState and change client code to access it via the factory. Use a local base::SequencedTaskRunner to post task in background instead of using the blocking pool. As the base::SequencedTaskRunner keep a reference to base::TaskScheduler, the prevents SnapshotCache from being a singleton (as it would outlive base::TaskScheduler). Use a SEQUENCE_CHECKER to ensure the calls are made on the correct sequence (instead on checking the current thread). Fix unit tests to wait for TaskScheduler to become idle instead of the blocking pool. BUG= 727597 , 667892 Change-Id: Icde812f74985698c4416a4dbc9c3f8b36c1e9522 Reviewed-on: https://chromium-review.googlesource.com/528193 Reviewed-by: Jean-François Geyelin <jif@chromium.org> Commit-Queue: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/heads/master@{#477989} [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/app/main_controller.mm [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/browser_state/BUILD.gn [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/snapshots/BUILD.gn [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/snapshots/snapshot_cache.h [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/snapshots/snapshot_cache.mm [add] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/snapshots/snapshot_cache_factory.h [add] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/snapshots/snapshot_cache_factory.mm [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/snapshots/snapshot_cache_unittest.mm [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/snapshots/snapshot_manager.h [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/snapshots/snapshot_manager.mm [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/tabs/tab.mm [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/tabs/tab_model.mm [modify] https://crrev.com/528c17b6de75d7aef7d7ee3e3d2abe68bb2884fe/ios/chrome/browser/ui/side_swipe/side_swipe_controller.mm
,
Jun 8 2017
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by sdefresne@chromium.org
, May 30 2017