Check if an iframe doesn't get detached twice

Check if an iframe doesn't get detached twice

BUG=

[haraken, 2016-10-07 10:31:51]

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

[commit-bot: I haz the power, 2016-10-07 10:32:06]

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-10-07 11:40:05]

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

[commit-bot: I haz the power, 2016-10-07 11:40:06]

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

[haraken, 2016-10-07 11:47:08]

haraken@chromium.org changed reviewers:
+ dcheng@chromium.org

[haraken, 2016-10-07 11:47:09]

See PS1. It looks like there are cases where Frame::detach() is called multiple
times. Is this expected?

[haraken, 2016-10-07 11:47:38]

On 2016/10/07 11:47:09, haraken wrote:
> See PS1. It looks like there are cases where Frame::detach() is called
multiple
> times. Is this expected?

(I'm not intending to land this CL.)

[dcheng, 2016-10-07 16:03:42]

Yes, unfortunately this is possible. LocalFrame::detach() detaches child frames
first, which can run script and cause |this| to be detached reentrantly before
the first LocalFrame::detach() has finished. That is why LocalFrame::detach() is
full of if (!client()) checks =(

[haraken, 2016-10-08 02:18:59]

On 2016/10/07 16:03:42, dcheng wrote:
> Yes, unfortunately this is possible. LocalFrame::detach() detaches child
frames
> first, which can run script and cause |this| to be detached reentrantly before
> the first LocalFrame::detach() has finished. That is why LocalFrame::detach()
is
> full of if (!client()) checks =(

Thanks Dcheng for the hint!

Is it unload handlers? Or is there any other scripts that can run during
Frame::detach()? It looks dangerous to run scripts during Frame::detach()...

[dcheng, 2016-10-08 04:59:50]

On 2016/10/08 02:18:59, haraken wrote:
> On 2016/10/07 16:03:42, dcheng wrote:
> > Yes, unfortunately this is possible. LocalFrame::detach() detaches child
> frames
> > first, which can run script and cause |this| to be detached reentrantly
before
> > the first LocalFrame::detach() has finished. That is why
LocalFrame::detach()
> is
> > full of if (!client()) checks =(
> 
> Thanks Dcheng for the hint!
> 
> Is it unload handlers? Or is there any other scripts that can run during
> Frame::detach()? It looks dangerous to run scripts during Frame::detach()...

We did a lot of tweaking to make it hopefully safe to run script. However, it is
very complicated as a result. I've annotated the CL with some comments
describing how we tried to make this safe. I guess we should add some more
comments to this code.

https://codereview.chromium.org/2395423002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2395423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:386:
m_loader.stopAllLoaders();
This can run script by causing a frame to be considered load complete: for an
example stack, see
https://bugs.chromium.org/p/chromium/issues/detail?id=561873#c127.

Note: the stack in that bug is past where we dispatch the load event; it's
actually a little higher in the stack in FrameLoader::checkCompleted, which can
call Document::implicitClose().

https://codereview.chromium.org/2395423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:390: SubframeLoadingDisabler
disabler(*document());
At this point, we disallow child frames from being attached. Firefox actually
doesn't disallow this, so there is one more "unload child frames" step in
Firefox's Document::shutdown() equivalent.

https://codereview.chromium.org/2395423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:391:
m_loader.dispatchUnloadEvent();
This runs the actual unload / pagehide event, which can run script.

https://codereview.chromium.org/2395423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:392: detachChildren();
This recurses into child frames and calls Frame::detach(), which can also
dispatch events.

https://codereview.chromium.org/2395423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:396: if (!client())
Because we just ran script, we must check that we are still attached: if we are
not, then that means one of the previous scripts already reentrantly removed us.
The rest of the work is already done, so we must early return.

https://codereview.chromium.org/2395423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:403:
m_loader.stopAllLoaders();
I don't honestly remember what this comment is about. I guess it's if a script
adds a <img> or <link rel="stylesheet" ...> or anything else that could start a
fetch.

https://codereview.chromium.org/2395423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:404: m_loader.detach();
This can also dispatch events: this aborts any XHRs that were still in progress.
This will fire the 'abort' event on the XHR if there is one.

https://codereview.chromium.org/2395423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:405: document()->shutdown();
Now we shutdown the Document. This is not supposed to run script, but apparently
it can...

The history behind
https://chromium.googlesource.com/chromium/src/+/d3e0e6655a412956248d217baa39...
is complicated.

Originally, when I cleaned up this code, executing script in a
ScriptForbiddenScope was a non-fatal error. So I added a ScriptForbiddenScope to
prevent script from running in the middle of detaching a DOM node (Document).

Then we made executing script in ScriptForbiddenScope a fatal error. So esprehn@
added HTMLFrameOwnerElement::UpdateSuspendScope to prevent plugins from running
script inside ScriptForbiddenScope.

However, since plugins could now run script in Document::detach, they were able
to queue navigations that would violate invariants during detach. So we added
FrameNavigationDisabler to prevent that...

https://codereview.chromium.org/2395423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:409: ScriptForbiddenScope
forbidScript;
Now no more script is allowed, and everything is a lot simpler =)

https://codereview.chromium.org/2395423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:411: if (!client())
However, I'm not sure why this check is after m_loader.clear() instead of
before. It should probably be before m_loader.clear()...

[haraken, 2016-10-11 05:20:35]

T

[haraken, 2016-10-11 05:23:26]

Thanks a lot for the detailed clarification! I now understand the complexity.

I'm hitting the complexity (for some complicated reasons) while I'm removing
DOMWindowProperty. In any case, I now understand that we need to assume that
Frame::detach may be reentered. Let me investigate a bit more :)