Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2478573002: Make removeChild, parserRemoveChild and removeChildren more consistent. (Closed)

Created:
1 year ago by esprehn
Modified:
6 months, 1 week ago
Reviewers:
tkent, dcheng
CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make removeChild, parserRemoveChild and removeChildren more consistent. This makes the order of operations inside the various DOM mutation methods consistent so events, disabler scopes, and side effects are all sequenced the same. It also introduces a SubframeLoadingDisabler, ScriptForbiddenScope and EventDispatchForbiddenScope around more code that should have those effects. This makes the DOM API more consistent, for example by preventing iframe loading in the same place in all remove paths, and also fixes potential UXSS bugs in cases where a frame is loaded inside the removal logic. Finally I added lots of comments to explain what's going on and what steps are running script.

Patch Set 1 #

Patch Set 2 : Add the SubframeLoadingDisabler to parserRemoveChild. #

Patch Set 3 : Revert error message for tests #

Total comments: 6

Patch Set 4 : SubframeLoadingDisabler (oldChild) #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+159 lines, -92 lines) Patch
M third_party/WebKit/Source/core/dom/ContainerNode.h View 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/dom/ContainerNode.cpp View 1 2 3 4 chunks +151 lines, -89 lines 7 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 2 chunks +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h View 1 chunk +3 lines, -1 line 0 comments Download
Trybot results:  win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-simulator   ios-device   linux_chromium_rel_ng   linux_chromium_clobber_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_compile_dbg_ng   linux_chromium_asan_rel_ng   chromeos_daisy_chromium_compile_only_ng   chromeos_x86-generic_chromium_compile_only_ng   chromium_presubmit   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   linux_android_rel_ng   blimp_linux_dbg   android_n5x_swarming_rel   cast_shell_android   android_cronet   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_compile_dbg_ng   mac_chromium_rel_ng   ios-simulator   ios-device   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_clobber_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_compile_dbg_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_asan_rel_ng   chromeos_x86-generic_chromium_compile_only_ng   chromium_presubmit   chromeos_daisy_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   blimp_linux_dbg   linux_android_rel_ng   cast_shell_android   android_cronet   android_n5x_swarming_rel   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe 

Messages

Total messages: 31 (19 generated)
esprehn
1 year ago (2016-11-04 00:15:48 UTC) #11
dcheng
https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Source/core/dom/ContainerNode.cpp File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Source/core/dom/ContainerNode.cpp#newcode491 third_party/WebKit/Source/core/dom/ContainerNode.cpp:491: SubframeLoadingDisabler subframeLoadingDisabler(*this); I think this should be scoped to ...
1 year ago (2016-11-04 02:15:54 UTC) #13
esprehn
https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Source/core/dom/ContainerNode.cpp File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Source/core/dom/ContainerNode.cpp#newcode491 third_party/WebKit/Source/core/dom/ContainerNode.cpp:491: SubframeLoadingDisabler subframeLoadingDisabler(*this); On 2016/11/04 at 02:15:54, dcheng wrote: > ...
1 year ago (2016-11-04 23:04:08 UTC) #16
esprehn
Okay want to take another look?
1 year ago (2016-11-04 23:39:21 UTC) #18
tkent
lgtm
1 year ago (2016-11-07 07:31:52 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2478573002/60001
1 year ago (2016-11-07 20:02:41 UTC) #24
dcheng
(Sorry for not replying sooner. My main thoughts are around SubframeLoadingDisabler.release(): I think we can ...
1 year ago (2016-11-07 20:50:57 UTC) #25
dcheng
(Sorry, unchecking CQ for now, since I just want to understand how the different parts ...
1 year ago (2016-11-07 21:13:37 UTC) #27
esprehn
https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Source/core/dom/ContainerNode.cpp File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Source/core/dom/ContainerNode.cpp#newcode533 third_party/WebKit/Source/core/dom/ContainerNode.cpp:533: // Script could run inside childrenChanged, enable iframe loading ...
1 year ago (2016-11-07 22:42:33 UTC) #28
dcheng
https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Source/core/dom/ContainerNode.cpp File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Source/core/dom/ContainerNode.cpp#newcode671 third_party/WebKit/Source/core/dom/ContainerNode.cpp:671: document().removeFocusedElementOfSubtree(this, true); On 2016/11/07 22:42:32, esprehn wrote: > On ...
1 year ago (2016-11-08 01:51:12 UTC) #29
esprehn
On 2016/11/08 at 01:51:12, dcheng wrote: > https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Source/core/dom/ContainerNode.cpp > File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): > ... > ...
1 year ago (2016-11-09 02:11:43 UTC) #30
dcheng
1 year ago (2016-11-09 03:23:08 UTC) #31
On 2016/11/09 02:11:43, esprehn wrote:
> On 2016/11/08 at 01:51:12, dcheng wrote:
> >
>
https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):
> > ...
> > 
> > Basically, hoist the UpdateSuspendScope a little higher, so that the scopers
> don't have scopes that intersect in an unusual way. It means that hierarchy
> updates in focus element changes would be deferred as well (unnecessarily),
but
> maybe that's OK?
> 
> Yeah I can do that, it just means the function ends up super deeply nested.
> You'd prefer that?

I think I'd be OK with that, it's a bit easier to match up braces.

Powered by Google App Engine
This is Rietveld efc10ee0f