Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1838083006:
Fix a use-after-free error in WaitOrCountBookmarks (Closed)
DescriptionFix a use-after-free error in WaitOrCountBookmarks
The use-after-free error is reproduced when WaitOrCountBookmarks is
executed twice before the bookmark model is loaded.
BUG=596693
TEST=Added a unit test, with patched code passing and unpatched code failing.
Committed: https://crrev.com/592ca50b6619173d09a6531e4202d22f7e647c74
Cr-Commit-Position: refs/heads/master@{#385528}
Patch Set 1 #Patch Set 2 : Add testing #
Total comments: 2
Patch Set 3 : The test now checks that the bookmarking test runs exactly once. #
Total comments: 2
Patch Set 4 : Rebase and update conflicting code #Patch Set 5 : Fix errors in code. #
Messages
Total messages: 28 (12 generated)
Description was changed from ========== Fix a use-after-free error in WaitOrCountBookmarks The use-after-free error is reproduced when WaitOrCountBookmarks is executed twice before the bookmark model is loaded. BUG=596693 ========== to ========== Fix a use-after-free error in WaitOrCountBookmarks The use-after-free error is reproduced when WaitOrCountBookmarks is executed twice before the bookmark model is loaded. BUG=596693 ==========
lwchkg@gmail.com changed reviewers: + mlerman@chromium.org, vmpstr@chromium.org
Dear all, PTAL. Regards, WC Leung.
On 2016/03/30 15:53:27, lwchkg wrote: > Dear all, > > PTAL. > > Regards, > WC Leung. Can you write a unit test that would fail without this, and now passes? Thanks.
Uploaded a new patch with the unit test. FYI, some testing code is adapted from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/t... . > Can you write a unit test that would fail without this, and now passes? > > Thanks.
https://codereview.chromium.org/1838083006/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_unittest.cc (right): https://codereview.chromium.org/1838083006/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_unittest.cc:145: // Load the bookmark model to allow the bookmark counting task to finish. This Rather than saying "this checks for a bug..." you can say "This verifies that multiple pending GatherStatistics() calls only results in one callback for gathering bookmarks", or similar. Then, you can verify that :)
New patch uploaded. https://codereview.chromium.org/1838083006/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_unittest.cc (right): https://codereview.chromium.org/1838083006/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_unittest.cc:145: // Load the bookmark model to allow the bookmark counting task to finish. This On 2016/04/04 15:38:58, Mike Lerman wrote: > Rather than saying "this checks for a bug..." you can say "This verifies that > multiple pending GatherStatistics() calls only results in one callback for > gathering bookmarks", or similar. Then, you can verify that :) Verification is hard... but done.
https://codereview.chromium.org/1838083006/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_unittest.cc (right): https://codereview.chromium.org/1838083006/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_unittest.cc:158: scoped_refptr<ProfileStatisticsAggregator> aggregator_scoped = Why delete this aggregator? It won't actually complete anything, its tasks will never fire. Don't you want to do similar to the previous version, where you call gatherStatistics twice, but then LoadBookmarkModel only once, and expect that the StatHelper is only called once?
https://codereview.chromium.org/1838083006/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_unittest.cc (right): https://codereview.chromium.org/1838083006/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_unittest.cc:158: scoped_refptr<ProfileStatisticsAggregator> aggregator_scoped = On 2016/04/04 20:26:27, Mike Lerman wrote: > Why delete this aggregator? It won't actually complete anything, its tasks will > never fire. > > Don't you want to do similar to the previous version, where you call > gatherStatistics twice, but then LoadBookmarkModel only once, and expect that > the StatHelper is only called once? By constructing the aggregator it makes a copy of the scoped_refptr on every tasks and also on every reply of the out-of-thread tasks. Keeping an extra copy of the scoped_refptr alive here makes ProfileStatiaticsAggregator not to destruct after the tasks finish. Anyway, we are checking that the StatHelper is called not more than once with bookmarks. So we need some guarantee that all tasks are finished before checking. This is only possible by monitoring the destruction of ProfileStatisticsAggregator. (Just waiting for the bookmark task return once does not guarantee that it is called not more than once.) It is currently impossible to get a callback function returned by GatherStatistics, so a slightly modified version of GatherStatistics is executed instead. (Of course we can add the callback in GatherStatistics, but to do this we need to patch a non-test file to have the unpatched code failing the test.)
okay, lgtm.
Description was changed from ========== Fix a use-after-free error in WaitOrCountBookmarks The use-after-free error is reproduced when WaitOrCountBookmarks is executed twice before the bookmark model is loaded. BUG=596693 ========== to ========== Fix a use-after-free error in WaitOrCountBookmarks The use-after-free error is reproduced when WaitOrCountBookmarks is executed twice before the bookmark model is loaded. BUG=596693 TEST=Added a unit test, with patched code passing and unpatched code failing. ==========
Just tested with unpatched code failed with a crash dump similar to the bug report's. Will land the CL now.
The CQ bit was checked by lwchkg@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838083006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838083006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lwchkg@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/1838083006/#ps60001 (title: "Rebase and update conflicting code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838083006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838083006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lwchkg@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/1838083006/#ps80001 (title: "Fix errors in code.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838083006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838083006/80001
Message was sent while issue was closed.
Description was changed from ========== Fix a use-after-free error in WaitOrCountBookmarks The use-after-free error is reproduced when WaitOrCountBookmarks is executed twice before the bookmark model is loaded. BUG=596693 TEST=Added a unit test, with patched code passing and unpatched code failing. ========== to ========== Fix a use-after-free error in WaitOrCountBookmarks The use-after-free error is reproduced when WaitOrCountBookmarks is executed twice before the bookmark model is loaded. BUG=596693 TEST=Added a unit test, with patched code passing and unpatched code failing. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix a use-after-free error in WaitOrCountBookmarks The use-after-free error is reproduced when WaitOrCountBookmarks is executed twice before the bookmark model is loaded. BUG=596693 TEST=Added a unit test, with patched code passing and unpatched code failing. ========== to ========== Fix a use-after-free error in WaitOrCountBookmarks The use-after-free error is reproduced when WaitOrCountBookmarks is executed twice before the bookmark model is loaded. BUG=596693 TEST=Added a unit test, with patched code passing and unpatched code failing. Committed: https://crrev.com/592ca50b6619173d09a6531e4202d22f7e647c74 Cr-Commit-Position: refs/heads/master@{#385528} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/592ca50b6619173d09a6531e4202d22f7e647c74 Cr-Commit-Position: refs/heads/master@{#385528} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
