Use ChildFrameDisconnector when detaching child frames of a LocalFrame.

Use ChildFrameDisconnector when detaching child frames of a LocalFrame.

Currently, UpdateSuspendScope is used to defer widget updates when the
DOM hierarchy is mutated. This is used to prevent script from running
in the middle of DOM mutations, since plugins can run script when
destroyed.

This is part 1 of 2 CLs to remove the need for UpdateSuspendScope. Part
1 changes LocalFrame detach to always use ChildFrameDisconnector to
detach child frames. Part 2 will rework ChildFrameDisconnector to also
detach plugin elements. This should eliminate the need to defer widget
updates, since script will never have to run during the actual mutation
of internal state.

BUG=524113, 528867, 561683, 621362
Committed: https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0
Cr-Commit-Position: refs/heads/master@{#405040}

[dcheng, 2016-07-11 12:13:51]

Description was changed from

==========
Use ChildFrameDisconnector when detaching child frames of a LocalFrame.

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

to

==========
Use ChildFrameDisconnector when detaching child frames of a LocalFrame.

Currently, UpdateSuspendScope is used to defer widget updates when the
DOM hierarchy is mutated. This is used to prevent script from running
in the middle of DOM mutations, since plugins cab run script when
destroyed.

This is part 1 of 2 CLs to remove the need for UpdateSuspendScope. Part
1 changes LocalFrame detach to always use ChildFrameDisconnector to
detach child frames. Part 2 will rework ChildFrameDisconnector to also
detach plugin elements. This should eliminate the need to defer widget
updates, since script will never have to run during the actual mutation
of internal state.

BUG=524113,528867,561683,621362
==========

[dcheng, 2016-07-11 12:13:51]

dcheng@chromium.org changed reviewers:
+ sigbjornf@opera.se

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

dcheng@chromium.org changed reviewers:
+ sigbjornf@opera.com
- sigbjornf@opera.se

[dcheng, 2016-07-11 12:15:41]

dcheng@chromium.org changed reviewers:
+ japhet@chromium.org
- sigbjornf@opera.com

[dcheng, 2016-07-11 12:15:42]

Note that this is somewhat speculative: I'm still working on persisted widget
handling in the followup. But the theory should be sound... I hope

[esprehn, 2016-07-11 19:37:30]

Description was changed from

==========
Use ChildFrameDisconnector when detaching child frames of a LocalFrame.

Currently, UpdateSuspendScope is used to defer widget updates when the
DOM hierarchy is mutated. This is used to prevent script from running
in the middle of DOM mutations, since plugins cab run script when
destroyed.

This is part 1 of 2 CLs to remove the need for UpdateSuspendScope. Part
1 changes LocalFrame detach to always use ChildFrameDisconnector to
detach child frames. Part 2 will rework ChildFrameDisconnector to also
detach plugin elements. This should eliminate the need to defer widget
updates, since script will never have to run during the actual mutation
of internal state.

BUG=524113,528867,561683,621362
==========

to

==========
Use ChildFrameDisconnector when detaching child frames of a LocalFrame.

Currently, UpdateSuspendScope is used to defer widget updates when the
DOM hierarchy is mutated. This is used to prevent script from running
in the middle of DOM mutations, since plugins can run script when
destroyed.

This is part 1 of 2 CLs to remove the need for UpdateSuspendScope. Part
1 changes LocalFrame detach to always use ChildFrameDisconnector to
detach child frames. Part 2 will rework ChildFrameDisconnector to also
detach plugin elements. This should eliminate the need to defer widget
updates, since script will never have to run during the actual mutation
of internal state.

BUG=524113,528867,561683,621362
==========

[esprehn, 2016-07-11 19:39:29]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-07-11 19:39:30]

Hmm, what about inside updateStyle when we unload plugins?

https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrame.cpp:204: for (Frame* child =
tree().firstChild(); child; child = child->tree().nextSibling())
why do we need to keep this code path for remote frames? I think in most pages
the frame tree is interleaved Remote and Local frames so you're likely to end up
down this code path really often.

[dcheng, 2016-07-12 01:14:41]

On 2016/07/11 19:39:30, esprehn wrote:
> Hmm, what about inside updateStyle when we unload plugins?
> 

Blah, plugins are why we can't have nice things. I guess we'll still need it for
updateStyle, though I think we can do something a bit simpler: we can just post
a task to the event loop to do the deferred updates. While this doesn't work for
detach–script expects to run while the element is still in an active
document–for style changes, the element (should?) still be in active document,
just hidden.

https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrame.cpp:204: for (Frame* child =
tree().firstChild(); child; child = child->tree().nextSibling())
On 2016/07/11 19:39:30, esprehn wrote:
> why do we need to keep this code path for remote frames? I think in most pages
> the frame tree is interleaved Remote and Local frames so you're likely to end
up
> down this code path really often.

ChildFrameDisconnector is based on DOM nodes, which remote frames don't have, so
we need an alternate path for remote frames. It's a bit unfortunate, but it's
not much different from before: previously, both versions delegate to a
non-virtual Frame::detachChildren: now they just happen to delegate to different
implementations of detachChildren.

[esprehn, 2016-07-13 02:35:46]

https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:455: if (document())
if (Document* document = this->document())

avoids the nested virtual calls down there.

https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrame.cpp:207:
child->detach(FrameDetachType::Remove);
this doesn't use a SubframeLoadingDisabler, so the script in here can load more
frames which then get missed by the above logic in the RemoteFrame case. That's
probably a security bug that already exists.

[esprehn, 2016-07-13 02:38:43]

lgtm, though I think long term perhaps we should centralize this logic in one
place. All the special code paths for RemoteFrame are sad.

I see now that you don't need a SubframeLoadingDisabler since RemoteFrame has no
Document.

[dcheng, 2016-07-13 02:51:34]

https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2134113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:455: if (document())
On 2016/07/13 02:35:45, esprehn wrote:
> if (Document* document = this->document())
> 
> avoids the nested virtual calls down there.

Done.

[dcheng, 2016-07-13 02:51:41]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-07-13 02:51:42]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2134113002/#ps60001
(title: "Use local")

[commit-bot: I haz the power, 2016-07-13 02:54:03]

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-07-13 02:58:40]

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

[commit-bot: I haz the power, 2016-07-13 02:58:42]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[dcheng, 2016-07-13 03:19:52]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-07-13 03:19:52]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2134113002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-13 03:22:46]

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-07-13 05:16:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-13 05:16:15]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 05:17:14]

Description was changed from

==========
Use ChildFrameDisconnector when detaching child frames of a LocalFrame.

Currently, UpdateSuspendScope is used to defer widget updates when the
DOM hierarchy is mutated. This is used to prevent script from running
in the middle of DOM mutations, since plugins can run script when
destroyed.

This is part 1 of 2 CLs to remove the need for UpdateSuspendScope. Part
1 changes LocalFrame detach to always use ChildFrameDisconnector to
detach child frames. Part 2 will rework ChildFrameDisconnector to also
detach plugin elements. This should eliminate the need to defer widget
updates, since script will never have to run during the actual mutation
of internal state.

BUG=524113,528867,561683,621362
==========

to

==========
Use ChildFrameDisconnector when detaching child frames of a LocalFrame.

Currently, UpdateSuspendScope is used to defer widget updates when the
DOM hierarchy is mutated. This is used to prevent script from running
in the middle of DOM mutations, since plugins can run script when
destroyed.

This is part 1 of 2 CLs to remove the need for UpdateSuspendScope. Part
1 changes LocalFrame detach to always use ChildFrameDisconnector to
detach child frames. Part 2 will rework ChildFrameDisconnector to also
detach plugin elements. This should eliminate the need to defer widget
updates, since script will never have to run during the actual mutation
of internal state.

BUG=524113,528867,561683,621362

Committed: https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0
Cr-Commit-Position: refs/heads/master@{#405040}
==========

[commit-bot: I haz the power, 2016-07-13 05:17:16]

Patchset 5 (id:??) landed as
https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0
Cr-Commit-Position: refs/heads/master@{#405040}