Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
219413002:
Hash iterators: Check for concurrent modification and fix 1 place where it happened (Closed)
|
Created:
6 years, 8 months ago by Erik Corry Modified:
6 years, 8 months ago Reviewers:
Eric Willigers, jochen (gone - plz use gerrit), Mads Ager (chromium), dstockwell, Nate Chapin, Mikhail CC:
blink-reviews, shans, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), Mikhail, abarth-chromium, dstockwell, Timothy Loh, adamk+blink_chromium.org, darktears, Steve Block, dino_apple.com, Inactive, Eric Willigers Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionHash iterators: Check for concurrent modification and fix 1 place where it happened
R=ager@chromium.org, dstockwell@chromium.org, ericwilligers@chromium.org, jochen@chromium.org, mikhail.pozdnyakov@intel.com
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170679
Patch Set 1 #
Total comments: 11
Patch Set 2 : CR feedback #
Total comments: 1
Patch Set 3 : Revert DocumentTimeline checks and make asserts more lenient #
Total comments: 6
Patch Set 4 : Move a few calls to registerModification #Patch Set 5 : Fix incorrect use of iterators and remove() in HistoryController.cpp #Messages
Total messages: 28 (0 generated)
interesting how much the debug build binary size is affected (maybe this check is even worth having a separate flag). https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h File Source/wtf/HashTable.h (right): https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h#newco... Source/wtf/HashTable.h:102: #ifndef NDEBUG Since the actual check is assertion, maybe it's better to rely on 'ASSERT_ENABLED'/'ASSERT_DISABLED' flags? https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h#newco... Source/wtf/HashTable.h:380: void registerModification() { m_modifications++; } modified() ? https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h#newco... Source/wtf/HashTable.h:385: void checkModifications(int64_t mods) const { ASSERT(mods == m_modifications); } is this method used? https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h#newco... Source/wtf/HashTable.h:1055: other.m_modifications = tmpModifications; why not 'swap' ?
LGTM This check definitely seems worth adding. https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h File Source/wtf/HashTable.h (right): https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h#newco... Source/wtf/HashTable.h:377: void checkModifications(int64_t mods) const { } mods -> modifications (or better, remove the name) https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h#newco... Source/wtf/HashTable.h:385: void checkModifications(int64_t mods) const { ASSERT(mods == m_modifications); } mods -> modifications (but as Mikhail writes, it looks unused?)
https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h File Source/wtf/HashTable.h (right): https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h#newco... Source/wtf/HashTable.h:102: #ifndef NDEBUG On 2014/03/31 14:08:09, mikhail.pozdnyakov wrote: > Since the actual check is assertion, maybe it's better to rely on > 'ASSERT_ENABLED'/'ASSERT_DISABLED' flags? Done. https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h#newco... Source/wtf/HashTable.h:377: void checkModifications(int64_t mods) const { } On 2014/03/31 14:18:47, Mads Ager (chromium) wrote: > mods -> modifications (or better, remove the name) gone https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h#newco... Source/wtf/HashTable.h:380: void registerModification() { m_modifications++; } On 2014/03/31 14:08:09, mikhail.pozdnyakov wrote: > modified() ? That looks more like a question than an action to me. https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h#newco... Source/wtf/HashTable.h:385: void checkModifications(int64_t mods) const { ASSERT(mods == m_modifications); } On 2014/03/31 14:18:47, Mads Ager (chromium) wrote: > mods -> modifications (but as Mikhail writes, it looks unused?) It's not used yet, so I removed it for now. https://codereview.chromium.org/219413002/diff/1/Source/wtf/HashTable.h#newco... Source/wtf/HashTable.h:1055: other.m_modifications = tmpModifications; On 2014/03/31 14:08:09, mikhail.pozdnyakov wrote: > why not 'swap' ? Done.
https://codereview.chromium.org/219413002/diff/20001/Source/core/animation/Do... File Source/core/animation/DocumentTimeline.h (right): https://codereview.chromium.org/219413002/diff/20001/Source/core/animation/Do... Source/core/animation/DocumentTimeline.h:102: // over it (this happens during pauseAnimationsForTesting). So the problem is that we're trying to modify the set while iterating over it, but I think the modification is trying to add a value already in the set. Is this is an illegal operation now? If this is just in pauseAnimationsForTesting, can we just copy the set to a vector and iterate over that?
On 2014/03/31 23:33:22, Timothy Loh wrote: > https://codereview.chromium.org/219413002/diff/20001/Source/core/animation/Do... > File Source/core/animation/DocumentTimeline.h (right): > > https://codereview.chromium.org/219413002/diff/20001/Source/core/animation/Do... > Source/core/animation/DocumentTimeline.h:102: // over it (this happens during > pauseAnimationsForTesting). > So the problem is that we're trying to modify the set while iterating over it, > but I think the modification is trying to add a value already in the set. Is > this is an illegal operation now? > > If this is just in pauseAnimationsForTesting, can we just copy the set to a > vector and iterate over that? It's only adding things that were already in the set. I think in some ways it would be nicer to just forbid adding things to the set, but this case is not actually dangerous, so I reverted the changes to DocumentTimeline and made the assert only trigger if you add things that were not already there.
Size of out/*/lib/libblink_web.so Debug with asserts: 953483232 Debug without asserts: 949805304 Size increase 0.38%
Thanks, the patch looks nice but it seems some of 'registerModification()' calls can be omitted (sorry for not noticing before!) https://codereview.chromium.org/219413002/diff/40001/Source/wtf/HashTable.h File Source/wtf/HashTable.h (right): https://codereview.chromium.org/219413002/diff/40001/Source/wtf/HashTable.h#n... Source/wtf/HashTable.h:564: registerModification(); 'lookupForWriting()' does not actually do modifications itself and it is be called only from 'reinsert()' atm. So maybe it's better to leave 'registerModification()' only inside 'reinsert()' (and it should save a bit of binary size =) ). https://codereview.chromium.org/219413002/diff/40001/Source/wtf/HashTable.h#n... Source/wtf/HashTable.h:627: registerModification(); I guess this can be also omitted as 'fullLookupForWriting' is only called from 'addPassingHashCode' and modification is already registered there. https://codereview.chromium.org/219413002/diff/40001/Source/wtf/HashTable.h#n... Source/wtf/HashTable.h:806: registerModification(); looks like it should be consistent with the check at 'add' method and register modifications only if a new entry (which did not exist before) is added.
https://codereview.chromium.org/219413002/diff/40001/Source/wtf/HashTable.h File Source/wtf/HashTable.h (right): https://codereview.chromium.org/219413002/diff/40001/Source/wtf/HashTable.h#n... Source/wtf/HashTable.h:564: registerModification(); On 2014/04/01 09:28:33, mikhail.pozdnyakov wrote: > 'lookupForWriting()' does not actually do modifications itself and it is be > called only from 'reinsert()' atm. So maybe it's better to leave > 'registerModification()' only inside 'reinsert()' (and it should save a bit of > binary size =) ). Done. https://codereview.chromium.org/219413002/diff/40001/Source/wtf/HashTable.h#n... Source/wtf/HashTable.h:627: registerModification(); On 2014/04/01 09:28:33, mikhail.pozdnyakov wrote: > I guess this can be also omitted as 'fullLookupForWriting' is only called from > 'addPassingHashCode' and modification is already registered there. Done. https://codereview.chromium.org/219413002/diff/40001/Source/wtf/HashTable.h#n... Source/wtf/HashTable.h:806: registerModification(); On 2014/04/01 09:28:33, mikhail.pozdnyakov wrote: > looks like it should be consistent with the check at 'add' method and register > modifications only if a new entry (which did not exist before) is added. Done.
LGTM
The CQ bit was checked by erik.corry@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/219413002/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on blink_presubmit
lgtm
The CQ bit was checked by erik.corry@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/219413002/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by erik.corry@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/219413002/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ found a place where we were updating a collection while iterating over it. CCing Nate since it's his code.
On 2014/04/02 13:41:58, Erik Corry wrote: > The CQ found a place where we were updating a collection while iterating over > it. CCing Nate since it's his code. LGTM
The CQ bit was checked by erik.corry@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erik.corry@gmail.com/219413002/70001
Message was sent while issue was closed.
Change committed as 170679 |
