Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1555653002:
Handle some failing DocumentOrderedMap ID lookups across tree removals. (Closed)
|
Created:
4 years, 11 months ago by sof Modified:
4 years, 11 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle some failing DocumentOrderedMap ID lookups across tree removals.
r366066's attempt to better handle failing DocumentOrderedMap lookups
while an element is being removed from a tree with duplicate IDs, didn't
accommodate all cases where the document map might end up being consulted.
Widen the assert and have it scope over node removals; should the unlikely
case happen, recognize that the tree is in a transitory state and allow
the lookup to quietly fail.
TBR=esprehn
BUG=571351
Committed: https://crrev.com/592eb5df9a5d469cdacb48559945e0dfdd7abb10
Cr-Commit-Position: refs/heads/master@{#368321}
Patch Set 1 #Patch Set 2 : add test #
Total comments: 11
Patch Set 3 : simplify removal scope tracking, make it global #Patch Set 4 : combat unused-variable warning-as-error errors #Patch Set 5 : non-assert buildfix #
Messages
Total messages: 36 (16 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Handle failing DocumentOrderedMap lookups across tree removals. r366066's attempt to better handle failing DocumentOrderedMap lookups while an element is being removed from a tree with duplicate IDs, didn't accommodate all cases where the document map might end up being consulted. Widen the assert and have it scope over node removals; should the unlikely case happen, recognize that the tree is in a transitory state and allow the lookup to quietly fail. R= BUG=571351 ========== to ========== Handle some failing DocumentOrderedMap ID lookups across tree removals. r366066's attempt to better handle failing DocumentOrderedMap lookups while an element is being removed from a tree with duplicate IDs, didn't accommodate all cases where the document map might end up being consulted. Widen the assert and have it scope over node removals; should the unlikely case happen, recognize that the tree is in a transitory state and allow the lookup to quietly fail. R= BUG=571351 ==========
sigbjornf@opera.com changed reviewers: + esprehn@chromium.org, tkent@chromium.org
please take a look (next year.)
I guess this is okay, really we should probably collect all the observers in a Vector during the remove and notify them after the remove is complete instead just like the didNotifySubtreeInsertionsToDocument system. https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:590: TreeScope::RemoveScope treeRemoveScope(this); DocumentOrderedMap::RemoveScope https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:644: TreeScope::RemoveScope treeRemoveScope(this); no need to pass anything if you just increment the static, this can just be DocumentOrderedMap::RemoveScope removeScope; which compiles into a no-op without ASSERTs. https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp (right): https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp:56: void DocumentOrderedMap::enterTreeRemoveScope() no need for the methods, put the class in DocumentOrderedMap.cpp and increment/decrement the static int https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentOrderedMap.h (right): https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentOrderedMap.h:68: void enterTreeRemoveScope(); put the class here, add an #elseif and define the class to be a no-op. https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentOrderedMap.h:97: int m_treeRemovalScopeLevel = 0; I would just use a static int https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/TreeScope.cpp (right): https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/TreeScope.cpp:587: // While removing a ContainerNode, lookups in the underlying id map will this comment should be with the RemoveScope class in the DocumentOrderedMap header https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/TreeScope.h (right): https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/TreeScope.h:150: explicit RemoveScope(ContainerNode*); reference, this should also be nested inside the map, not in the TreeScope.
https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp (right): https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp:56: void DocumentOrderedMap::enterTreeRemoveScope() On 2016/01/05 07:48:33, esprehn wrote: > no need for the methods, put the class in DocumentOrderedMap.cpp and > increment/decrement the static int That's what ps#1 did, but then I got worried about concurrent updates to multiple Documents. That's not a valid concern?
On 2016/01/05 at 07:53:37, sigbjornf wrote: > https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp (right): > > https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp:56: void DocumentOrderedMap::enterTreeRemoveScope() > On 2016/01/05 07:48:33, esprehn wrote: > > no need for the methods, put the class in DocumentOrderedMap.cpp and > > increment/decrement the static int > > That's what ps#1 did, but then I got worried about concurrent updates to multiple Documents. That's not a valid concern? The DOM isn't thread safe so it'd have to be recursive updates. I'm not sure how that would happen. I'd start there and do this later once we have a test case for the recursive update (if it's possible at all).
On 2016/01/05 08:34:56, esprehn wrote: > On 2016/01/05 at 07:53:37, sigbjornf wrote: > > > https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp (right): > > > > > https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/dom/DocumentOrderedMap.cpp:56: void > DocumentOrderedMap::enterTreeRemoveScope() > > On 2016/01/05 07:48:33, esprehn wrote: > > > no need for the methods, put the class in DocumentOrderedMap.cpp and > > > increment/decrement the static int > > > > That's what ps#1 did, but then I got worried about concurrent updates to > multiple Documents. That's not a valid concern? > > The DOM isn't thread safe so it'd have to be recursive updates. I'm not sure how > that would happen. I'd start there and do this later once we have a test case > for the recursive update (if it's possible at all). ok - yes, mutation in a tree while removing something from another, is what it would have to be.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1555653002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1555653002/60001
Description was changed from ========== Handle some failing DocumentOrderedMap ID lookups across tree removals. r366066's attempt to better handle failing DocumentOrderedMap lookups while an element is being removed from a tree with duplicate IDs, didn't accommodate all cases where the document map might end up being consulted. Widen the assert and have it scope over node removals; should the unlikely case happen, recognize that the tree is in a transitory state and allow the lookup to quietly fail. R= BUG=571351 ========== to ========== Handle some failing DocumentOrderedMap ID lookups across tree removals. r366066's attempt to better handle failing DocumentOrderedMap lookups while an element is being removed from a tree with duplicate IDs, didn't accommodate all cases where the document map might end up being consulted. Widen the assert and have it scope over node removals; should the unlikely case happen, recognize that the tree is in a transitory state and allow the lookup to quietly fail. R=esprehn BUG=571351 ==========
https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentOrderedMap.h (right): https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentOrderedMap.h:68: void enterTreeRemoveScope(); On 2016/01/05 07:48:33, esprehn wrote: > put the class here, add an #elseif and define the class to be a no-op. Done. https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentOrderedMap.h:97: int m_treeRemovalScopeLevel = 0; On 2016/01/05 07:48:33, esprehn wrote: > I would just use a static int Done. https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/TreeScope.cpp (right): https://codereview.chromium.org/1555653002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/TreeScope.cpp:587: // While removing a ContainerNode, lookups in the underlying id map will On 2016/01/05 07:48:33, esprehn wrote: > this comment should be with the RemoveScope class in the DocumentOrderedMap > header Done, moved to DocumentOrderedMap::RemoveScope declaration.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1555653002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1555653002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1555653002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1555653002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
good to go?
On 2016/01/06 08:29:02, sof wrote: > good to go? ping?
landing, will follow up should there be more to attend to.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1555653002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1555653002/100001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== Handle some failing DocumentOrderedMap ID lookups across tree removals. r366066's attempt to better handle failing DocumentOrderedMap lookups while an element is being removed from a tree with duplicate IDs, didn't accommodate all cases where the document map might end up being consulted. Widen the assert and have it scope over node removals; should the unlikely case happen, recognize that the tree is in a transitory state and allow the lookup to quietly fail. R=esprehn BUG=571351 ========== to ========== Handle some failing DocumentOrderedMap ID lookups across tree removals. r366066's attempt to better handle failing DocumentOrderedMap lookups while an element is being removed from a tree with duplicate IDs, didn't accommodate all cases where the document map might end up being consulted. Widen the assert and have it scope over node removals; should the unlikely case happen, recognize that the tree is in a transitory state and allow the lookup to quietly fail. TBR=esprehn BUG=571351 ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1555653002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1555653002/100001
Message was sent while issue was closed.
Description was changed from ========== Handle some failing DocumentOrderedMap ID lookups across tree removals. r366066's attempt to better handle failing DocumentOrderedMap lookups while an element is being removed from a tree with duplicate IDs, didn't accommodate all cases where the document map might end up being consulted. Widen the assert and have it scope over node removals; should the unlikely case happen, recognize that the tree is in a transitory state and allow the lookup to quietly fail. TBR=esprehn BUG=571351 ========== to ========== Handle some failing DocumentOrderedMap ID lookups across tree removals. r366066's attempt to better handle failing DocumentOrderedMap lookups while an element is being removed from a tree with duplicate IDs, didn't accommodate all cases where the document map might end up being consulted. Widen the assert and have it scope over node removals; should the unlikely case happen, recognize that the tree is in a transitory state and allow the lookup to quietly fail. TBR=esprehn BUG=571351 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Handle some failing DocumentOrderedMap ID lookups across tree removals. r366066's attempt to better handle failing DocumentOrderedMap lookups while an element is being removed from a tree with duplicate IDs, didn't accommodate all cases where the document map might end up being consulted. Widen the assert and have it scope over node removals; should the unlikely case happen, recognize that the tree is in a transitory state and allow the lookup to quietly fail. TBR=esprehn BUG=571351 ========== to ========== Handle some failing DocumentOrderedMap ID lookups across tree removals. r366066's attempt to better handle failing DocumentOrderedMap lookups while an element is being removed from a tree with duplicate IDs, didn't accommodate all cases where the document map might end up being consulted. Widen the assert and have it scope over node removals; should the unlikely case happen, recognize that the tree is in a transitory state and allow the lookup to quietly fail. TBR=esprehn BUG=571351 Committed: https://crrev.com/592eb5df9a5d469cdacb48559945e0dfdd7abb10 Cr-Commit-Position: refs/heads/master@{#368321} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/592eb5df9a5d469cdacb48559945e0dfdd7abb10 Cr-Commit-Position: refs/heads/master@{#368321} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
