Make removeChild, parserRemoveChild and removeChildren more consistent.

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.

[esprehn, 2016-11-03 19:57:37]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 19:58:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2016-11-03 20:00:24]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 20:01:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 22:05:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 22:05:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[esprehn, 2016-11-03 22:45:41]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 22:46:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2016-11-03 23:06:49]

Description was changed from

==========
Make removeChild, parserRemoveChild and removeChildren more consistent.

BUG=
==========

to

==========
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.
==========

[esprehn, 2016-11-04 00:15:47]

esprehn@chromium.org changed reviewers:
+ dcheng@chromium.org, tkent@chromium.org

[esprehn, 2016-11-04 00:16:12]

Description was changed from

==========
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.
==========

to

==========
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.
==========

[dcheng, 2016-11-04 02:15:54]

https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:491:
SubframeLoadingDisabler subframeLoadingDisabler(*this);
I think this should be scoped to |oldChild|.

https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:590:
SubframeLoadingDisabler subframeLoadingDisabler(*this);
I think this should be scoped to |oldChild|

https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:623:
subframeLoadingDisabler.release();
I think we won't need this if the disabler is scoped to the old child.

[commit-bot: I haz the power, 2016-11-04 03:06:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 03:06:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[esprehn, 2016-11-04 23:04:08]

https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:491:
SubframeLoadingDisabler subframeLoadingDisabler(*this);
On 2016/11/04 at 02:15:54, dcheng wrote:
> I think this should be scoped to |oldChild|.

Err yeah

https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:590:
SubframeLoadingDisabler subframeLoadingDisabler(*this);
On 2016/11/04 at 02:15:54, dcheng wrote:
> I think this should be scoped to |oldChild|

Yup

https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:623:
subframeLoadingDisabler.release();
On 2016/11/04 at 02:15:54, dcheng wrote:
> I think we won't need this if the disabler is scoped to the old child.

childrenChanged is going to run script, so we need to release the
subframeLoadingDisabler otherwise it would block an iframe load if the script
inside here inserted the child again somewhere else in the tree.

[esprehn, 2016-11-04 23:39:14]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[esprehn, 2016-11-04 23:39:21]

Okay want to take another look?

[commit-bot: I haz the power, 2016-11-04 23:39:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-05 01:27:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-05 01:27:44]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-11-07 07:31:52]

lgtm

[esprehn, 2016-11-07 20:01:29]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2016-11-07 20:02:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-11-07 20:50:57]

(Sorry for not replying sooner. My main thoughts are around
SubframeLoadingDisabler.release(): I think we can avoid the need for it
completely, from my reading of the code.

https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:533: // Script could run
inside childrenChanged, enable iframe loading again.
Nit: it's not clear to me what "enable iframe loading again" here means, there's
no SubframeLoadDisabler in this scope.

https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:590:
SubframeLoadingDisabler subframeLoadingDisabler(oldChild);
I'd like to understand why this needs to be scoped at this level.

Lines 592-604 don't run script.
Scripting is forbidden from 607-617.

So it's not clear to me why this SubframeLoadingDisabler helps (since we need to
release it before the next point we can run script anyway).

https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:671:
document().removeFocusedElementOfSubtree(this, true);
From reading this, perhaps we should just scope SubframeLoadingDisabler inside
removeFocusedElementOfSubtree()? The only time we're actually disabling subframe
loading is inside this statement: in the rest of this function, scripting is
either forbidden (from 679 - 690), or scripting and subframe loading are
enabled.

[dcheng, 2016-11-07 21:12:14]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2016-11-07 21:13:37]

(Sorry, unchecking CQ for now, since I just want to understand how the different
parts of this patch interact.)

[esprehn, 2016-11-07 22:42:33]

https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:533: // Script could run
inside childrenChanged, enable iframe loading again.
On 2016/11/07 at 20:50:57, dcheng wrote:
> Nit: it's not clear to me what "enable iframe loading again" here means,
there's no SubframeLoadDisabler in this scope.

There is one on the stack, see above subframeLoadingDisabler on line 491, I
apparently forgot the release() here though. I'll fix that.

https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:590:
SubframeLoadingDisabler subframeLoadingDisabler(oldChild);
On 2016/11/07 at 20:50:57, dcheng wrote:
> I'd like to understand why this needs to be scoped at this level.
> 
> Lines 592-604 don't run script.
> Scripting is forbidden from 607-617.
> 
> So it's not clear to me why this SubframeLoadingDisabler helps (since we need
to release it before the next point we can run script anyway).

Anything that finds a ScriptForbiddenScope bypass could insert a frame which
would then not get detached because we're after the ChildFrameDisconnector step.
Also if something in C++, ex. in a user agent shadow root, inserted an iframe it
would also get missed and not detached. So we put a SubframeLoadingDisabler on
the stack to prevent those bugs and the resulting UXSS for defense in depth.

Once we call ChildFrameDisconnector().disconnect(), nothing can insert frames in
that child until after notifyNodeRemoved or bad stuff happens. This enforces
that.

https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:671:
document().removeFocusedElementOfSubtree(this, true);
On 2016/11/07 at 20:50:57, dcheng wrote:
> From reading this, perhaps we should just scope SubframeLoadingDisabler inside
removeFocusedElementOfSubtree()? The only time we're actually disabling subframe
loading is inside this statement: in the rest of this function, scripting is
either forbidden (from 679 - 690), or scripting and subframe loading are
enabled.

That wouldn't have prevented the recent UXSS, since they found a way around the
ScriptForbiddenScope and ran script inside of notifyNodeRemoved.

Inserting an iframe in JS or C++ anywhere in here after ChildFrameDisconnector
but before we finish calling notifyNodeRemoved is bad.

[dcheng, 2016-11-08 01:51:12]

https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:671:
document().removeFocusedElementOfSubtree(this, true);
On 2016/11/07 22:42:32, esprehn wrote:
> On 2016/11/07 at 20:50:57, dcheng wrote:
> > From reading this, perhaps we should just scope SubframeLoadingDisabler
inside
> removeFocusedElementOfSubtree()? The only time we're actually disabling
subframe
> loading is inside this statement: in the rest of this function, scripting is
> either forbidden (from 679 - 690), or scripting and subframe loading are
> enabled.
> 
> That wouldn't have prevented the recent UXSS, since they found a way around
the
> ScriptForbiddenScope and ran script inside of notifyNodeRemoved.
> 
> Inserting an iframe in JS or C++ anywhere in here after ChildFrameDisconnector
> but before we finish calling notifyNodeRemoved is bad.

Sorry, one more question, because I'd really like to avoid the manual release().
I'm wondering if we can structure the remove methods like this:

  {
    ChildFrameDisconnector(...).disconnect();
    HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
    {
      SubframeLoadingDisabler subframeLoadingDisabler(*this);
      document().removeFocusedElementOfSubtree(tree, this);

      DocumentOrderedMap::removeScope treeRemoveScope;
      {
        EventDispatchForbiddenScope assertNoEventDispatch;
        ScriptForbiddenScope forbidScript;

        while (Node* oldChild = m_firstChild) {
          removeBetween(0, ...);
          notifyNodeRemoved(*oldChild);
        }
      }
    }
    ChildrenChange change = {...}
    childrenChanged(change);
  }

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?

[esprehn, 2016-11-09 02:11:43]

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?

[dcheng, 2016-11-09 03:23:08]

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.