Cancel javascript: URL navigations if the frame was navigated.

Don't allow navigations in Document::detach.

When navigating to a javascript: URL, Blink detaches the original
Document. This process may detach plugin elements, causing a nested
message loop to run.

Document::detach() creates a ScriptForbiddenScope to prevent script from
breaking invariants. Since plugins were detached synchronously, any
script trying to execute in the nested message loop would be blocked.

However, the fix for https://crbug.com/524120 defers plugin updates to
happen outside the ScriptForbiddenScope. Thus, it is now possible to
attach a *new* Document with a synchronous navigation while the old
Document is being detached.

BUG=546545
Committed: https://crrev.com/66ad73d642b9cf824f4b1f300811ed1ee6963da7
Cr-Commit-Position: refs/heads/master@{#360190}

[dcheng, 2015-11-16 03:02:50]

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

[dcheng, 2015-11-16 08:24:29]

Originally, I wanted to make a more comprehensive fix: performing deferred
widget operations inside a ScriptForbiddenScope can sometimes result in a
release assert. However, since I'd like to merge the fix (and I'm having trouble
with changing when widget updates run), this is something simpler that should
fix the problem for now.

I don't 100% understand how this happens, but without my patch, the test case
hits an assert in LocalFrame::setView():
    ASSERT(!document() || !document()->isActive());
This fires when creating a new FrameView in
client()->transitionToCommittedForNewPage() in
FrameLoader::replaceDocumentWhileExecutingJavaScriptURL(). This violates the
invariant that a new FrameView can only be created when there is no Document
attached.

If that assert is skipped over, then another asserts fires in
FrameView::scheduleRelayout():
    ASSERT(m_frame->view() == this);

This is what leads me to believe that this is due to a new Document being
attached while the old one is being detached. Unfortunately, it's hard to write
a test that doesn't use Flash (since running script in plugin detach is a
Flash-specific compatibility hack).

https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:341: if (originalDocument
!= m_frame->document())
I considered two other approaches:
- Checking m_frame->document()->loader() == documentLoader: this breaks
javascript: URL navigation in the initial empty document case.
- ownerDocument != m_frame->document(): this doesn't work if another Document
other than this one initiated the JS url navigation

[haraken, 2015-11-16 08:36:00]

LGTM

https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:319:
RefPtrWillBeRawPtr<DocumentLoader>
documentLoader(m_frame->document()->loader());

Nit: It would be better to move this to just above
documentLoader->replaceDocumentWhileExecutingJavaScriptURL.

[haraken, 2015-11-16 08:37:06]

>
https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/FrameLoader.cpp:319:
> RefPtrWillBeRawPtr<DocumentLoader>
> documentLoader(m_frame->document()->loader());
> 
> Nit: It would be better to move this to just above
> documentLoader->replaceDocumentWhileExecutingJavaScriptURL.

If this is a fix for merging, you don't need to address the comment :)

[Nate Chapin, 2015-11-16 19:16:34]

https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:341: if (originalDocument
!= m_frame->document())
On 2015/11/16 08:24:29, dcheng wrote:
> I considered two other approaches:
> - Checking m_frame->document()->loader() == documentLoader: this breaks
> javascript: URL navigation in the initial empty document case.
> - ownerDocument != m_frame->document(): this doesn't work if another Document
> other than this one initiated the JS url navigation

...can we just stop the madness and disable navigation inside detach? What does
that break?

[dcheng, 2015-11-16 19:19:19]

On 2015/11/16 at 19:16:34, japhet wrote:
>
https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/1444183003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/FrameLoader.cpp:341: if
(originalDocument != m_frame->document())
> On 2015/11/16 08:24:29, dcheng wrote:
> > I considered two other approaches:
> > - Checking m_frame->document()->loader() == documentLoader: this breaks
> > javascript: URL navigation in the initial empty document case.
> > - ownerDocument != m_frame->document(): this doesn't work if another
Document
> > other than this one initiated the JS url navigation
> 
> ...can we just stop the madness and disable navigation inside detach? What
does that break?

Not sure!

I was actually looking into it, because this patch (still) doesn't work: the
invariant that is /supposed/ to hold after running Document::detach() is that
m_frame->document() is either null or inactive, and m_frame->view() is detached.
Unsurprisingly, attaching a new Document inside Document::detach() breaks this.
This results in a dangling Document/FrameView.

[dcheng, 2015-11-16 23:36:02]

Description was changed from

==========
Cancel javascript: URL navigations if the frame was navigated.

When navigating to a javascript: URL, Blink detaches the original
Document. This process may detach plugin elements, causing a nested
message loop to run.

Document::detach() creates a ScriptForbiddenScope to prevent script from
breaking invariants. Since plugins were detached synchronously, any
script trying to execute in the nested message loop would be blocked.

However, the fix for https://crbug.com/524120 defers plugin updates to
happen outside the ScriptForbiddenScope. Thus, it is now possible to
attach a *new* Document while detaching the old one. In this case, the
javascript: navigation must be cancelled.

BUG=546545
==========

to

==========
Don't allow navigations in Document::detach.

When navigating to a javascript: URL, Blink detaches the original
Document. This process may detach plugin elements, causing a nested
message loop to run.

Document::detach() creates a ScriptForbiddenScope to prevent script from
breaking invariants. Since plugins were detached synchronously, any
script trying to execute in the nested message loop would be blocked.

However, the fix for https://crbug.com/524120 defers plugin updates to
happen outside the ScriptForbiddenScope. Thus, it is now possible to
attach a *new* Document with a synchronous navigation while the old
Document is being detached.

BUG=546545
==========

[dcheng, 2015-11-16 23:50:48]

OK, new patch.

Unfortunately, the loading stack is a house of cards that is on fire, so this is
still probably not the right long-term fix. But it prevents at least this
problem =(

[dcheng, 2015-11-17 01:34:24]

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

[commit-bot: I haz the power, 2015-11-17 01:35:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1444183003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1444183003/200001

[commit-bot: I haz the power, 2015-11-17 02:17:03]

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

[commit-bot: I haz the power, 2015-11-17 02:17:04]

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

[dcheng, 2015-11-17 03:21:56]

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

[commit-bot: I haz the power, 2015-11-17 03:22:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1444183003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1444183003/200001

[commit-bot: I haz the power, 2015-11-17 05:01:24]

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

[commit-bot: I haz the power, 2015-11-17 05:01:25]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2015-11-17 17:47:10]

LGTM

Any hope of a test?

[esprehn, 2015-11-17 17:59:11]

On 2015/11/17 at 17:47:10, japhet wrote:
> LGTM
> 
> Any hope of a test?

Seems like you should be able to write a Sim test?

[dcheng, 2015-11-17 20:31:14]

On 2015/11/17 at 17:59:11, esprehn wrote:
> On 2015/11/17 at 17:47:10, japhet wrote:
> > LGTM
> > 
> > Any hope of a test?
> 
> Seems like you should be able to write a Sim test?

How would you feel about a followup patch with the test? After poking around, I
think it should be possible, but it involves creating some new fakes for
instantiating a test WebPlugin with the desired anti-patterns. I'd like to land
this ASAP to merge.

[dcheng, 2015-11-17 21:02:11]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-11-17 21:02:11]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1444183003/#ps200001
(title: "Constify")

[commit-bot: I haz the power, 2015-11-17 21:04:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1444183003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1444183003/200001

[dcheng, 2015-11-17 21:05:35]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2015-11-17 21:35:02]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-11-17 21:35:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1444183003/#ps220001
(title: "More comments")

[commit-bot: I haz the power, 2015-11-17 21:35:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1444183003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1444183003/220001

[commit-bot: I haz the power, 2015-11-17 23:06:57]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-11-17 23:07:53]

Patchset 12 (id:??) landed as
https://crrev.com/66ad73d642b9cf824f4b1f300811ed1ee6963da7
Cr-Commit-Position: refs/heads/master@{#360190}

[Yuki, 2016-05-18 07:13:57]

Description was changed from

==========
Don't allow navigations in Document::detach.

When navigating to a javascript: URL, Blink detaches the original
Document. This process may detach plugin elements, causing a nested
message loop to run.

Document::detach() creates a ScriptForbiddenScope to prevent script from
breaking invariants. Since plugins were detached synchronously, any
script trying to execute in the nested message loop would be blocked.

However, the fix for https://crbug.com/524120 defers plugin updates to
happen outside the ScriptForbiddenScope. Thus, it is now possible to
attach a *new* Document with a synchronous navigation while the old
Document is being detached.

BUG=546545

Committed: https://crrev.com/66ad73d642b9cf824f4b1f300811ed1ee6963da7
Cr-Commit-Position: refs/heads/master@{#360190}
==========

to

==========
Don't allow navigations in Document::detach.

When navigating to a javascript: URL, Blink detaches the original
Document. This process may detach plugin elements, causing a nested
message loop to run.

Document::detach() creates a ScriptForbiddenScope to prevent script from
breaking invariants. Since plugins were detached synchronously, any
script trying to execute in the nested message loop would be blocked.

However, the fix for https://crbug.com/524120 defers plugin updates to
happen outside the ScriptForbiddenScope. Thus, it is now possible to
attach a *new* Document with a synchronous navigation while the old
Document is being detached.

BUG=546545

Committed: https://crrev.com/66ad73d642b9cf824f4b1f300811ed1ee6963da7
Cr-Commit-Position: refs/heads/master@{#360190}
==========