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

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

Created:
1 year, 5 months ago by esprehn
Modified:
11 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

Messages

Total messages: 31 (19 generated)
esprehn
1 year, 5 months 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, 5 months 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, 5 months ago (2016-11-04 23:04:08 UTC) #16
esprehn
Okay want to take another look?
1 year, 5 months ago (2016-11-04 23:39:21 UTC) #18
tkent
lgtm
1 year, 5 months 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, 5 months 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, 5 months 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, 5 months 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, 5 months 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, 5 months 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, 5 months ago (2016-11-09 02:11:43 UTC) #30
dcheng
1 year, 5 months 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 408576698