Track down crash during RenderFrameImpl::didCommitProvisionalLoad.

Track down crash during RenderFrameImpl::didCommitProvisionalLoad.

It's possible that the RenderFrame isn't being deleted during swap
in some case, in which case the RenderView doesn't have its
main_render_frame_ cleared.

BUG=575245
TEST=Crashes in OnSwapOut instead of didCommitProvisionalLoad.

Committed: https://crrev.com/c6d547dcd9331ac29eb261dd2087f9980a2eb197
Cr-Commit-Position: refs/heads/master@{#369577}

[Charlie Reis, 2016-01-13 23:30:49]

creis@chromium.org changed reviewers:
+ dcheng@chromium.org, nasko@chromium.org

[Charlie Reis, 2016-01-13 23:30:50]

Nasko and Daniel, can you take a look?

This won't fix the crash, but it will indicate whether my theory is correct.  It
might also point us to some URLs that cause the crash, which could be useful for
finding ways for swap() to return false without detaching the frame.

Additional questions about how to fix the bug itself below.  Maybe we can land
this before that's resolved, though.

https://codereview.chromium.org/1581193002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1581193002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1507: frame_->swap(proxy->web_frame());
dcheng: I'm not sure what we should do differently here if swap returns false. 
In some of those cases, the frame will have been detached (via JS), and in
others it might not (e.g., m_provisionalDocumentLoader check in
FrameLoader::prepareForCommit).

Can swap have an invariant that the frame is always detached afterward?  (It
might be able to make the distinction between the cases mentioned above.)  

Do we need the return value, given that no one is listening to it?

[dcheng, 2016-01-14 00:15:26]

+japhet

https://codereview.chromium.org/1581193002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1581193002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1507: frame_->swap(proxy->web_frame());
On 2016/01/13 at 23:30:49, Charlie Reis wrote:
> dcheng: I'm not sure what we should do differently here if swap returns false.
 In some of those cases, the frame will have been detached (via JS), and in
others it might not (e.g., m_provisionalDocumentLoader check in
FrameLoader::prepareForCommit).
> 
> Can swap have an invariant that the frame is always detached afterward?  (It
might be able to make the distinction between the cases mentioned above.)  

We need to figure out the behavior of sync navigations in unload events first.
From my really simple testing, it appears that Chrome is the only browser (I
haven't tested Edge) that allows a fragment navigation in an unload event to
take effect. japhet@ points out that we should probably test pushState /
replaceState / about:blank as well.

This is an edge case I'm perfectly happy not to support, but we should probably
get some language in the spec about this (I don't see anything about this right
now). It's also sort of a web-facing change, even if it's just Chrome.

> 
> Do we need the return value, given that no one is listening to it?

Well, if we wanted to preserve current Chrome behavior, running code after the
failed swap could be incorrect: for example, mandoline updated some WebFrame
pointers after calling swap(). These updates are incorrect if the swap() is
cancelled.

[dcheng, 2016-01-14 00:24:29]

I also found
https://regenerator-runtime-only.jepso.com/list/whatwg/topic/navigation-trigg...,
which seems to claim that gecko does respect navigation in unload handlers for
same-origin navigations?

Argh, this is all very confusing.

[dcheng, 2016-01-14 00:33:35]

Oh, so apparently the spec does say something about this:
https://html.spec.whatwg.org/multipage/browsers.html#navigating-across-documents

So it looks like the navigation in unload handlers is only allowed for
same-origin navigations (see steps 3-5 for the fun details). Given that, it
seems like it might be reasonable to enforce the invariant requested. We should
add some asserts and tests for this behavior of swap() too.

[Charlie Reis, 2016-01-14 18:06:12]

On 2016/01/14 00:33:35, dcheng wrote:
> Oh, so apparently the spec does say something about this:
>
https://html.spec.whatwg.org/multipage/browsers.html#navigating-across-documents
> 
> So it looks like the navigation in unload handlers is only allowed for
> same-origin navigations (see steps 3-5 for the fun details). Given that, it
> seems like it might be reasonable to enforce the invariant requested. We
should
> add some asserts and tests for this behavior of swap() too.

Hmm, this is subtle.  Here's the language from step 3 of the spec:

"If there is a preexisting attempt to navigate the browsing context, and the
source browsing context is the same as the browsing context being navigated, and
that attempt is currently running the unload a document algorithm, and the
origin of the URL of the resource being loaded in that navigation is not the
same origin as the origin of the URL of the resource being loaded in this
navigation, then abort these steps without affecting the preexisting attempt to
navigate the browsing context."

I think that means the two navigation attempts have to be same origin with each
other, not necessarily with the current page.  For example, a page going from
foo.com to bar.com/1 could have an unload handler that interrupts and goes to
bar.com/2 instead.  If so, Chrome can't support that-- for cross-process
navigations, we run the unload handler in the background *after* the new page
has committed.  (That is, we've already switched to bar.com/1 when telling
foo.com to unload, so we can't go to bar.com/2 "instead.")

If we wanted to only allow unload navigations if both navigation attempts are
same origin with the current page (e.g., foo.com to foo.com/dest, interrupted by
foo.com/instead), we could do that.  And I think I agree: that approach would
mean we never do a navigation during unload for a cross-process navigation, so
we could guarantee that the frame is always detached after WebFrame::swap.

(I'm still trying to repro the crash by navigating during an unload handler,
without any luck so far.)

[Charlie Reis, 2016-01-14 21:02:32]

Nasko/Daniel: Would you be ok approving this to land while we decide the right
way to handle the swap behavior?

At the moment, we're likely leaving the RenderFrame in the tree in the renderer
process while the browser process has moved on to a new RenderFrameHost.  This
could probably lead to more scary bugs than just the didCommitProvisionalLoad
one, so I'd like to crash earlier.  That would also give us some crash reports
to analyze (e.g., for repro URLs).

[Charlie Reis, 2016-01-14 21:02:44]

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

[commit-bot: I haz the power, 2016-01-14 21:03:37]

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

[commit-bot: I haz the power, 2016-01-14 21:48:43]

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

[commit-bot: I haz the power, 2016-01-14 21:48:43]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-01-14 22:11:39]

Landing this to help diagnose, LGTM.

[Charlie Reis, 2016-01-14 22:15:02]

Thanks.  Daniel sounded ok with it when we chatted earlier, so I'll proceed and
we can follow up separately.

[Charlie Reis, 2016-01-14 22:15:09]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-01-14 22:17:28]

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

[commit-bot: I haz the power, 2016-01-14 22:23:17]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-01-14 22:25:38]

Description was changed from

==========
Track down crash during RenderFrameImpl::didCommitProvisionalLoad.

It's possible that the RenderFrame isn't being deleted during swap
in some case, in which case the RenderView doesn't have its
main_render_frame_ cleared.

BUG=575245
TEST=Crashes in OnSwapOut instead of didCommitProvisionalLoad.
==========

to

==========
Track down crash during RenderFrameImpl::didCommitProvisionalLoad.

It's possible that the RenderFrame isn't being deleted during swap
in some case, in which case the RenderView doesn't have its
main_render_frame_ cleared.

BUG=575245
TEST=Crashes in OnSwapOut instead of didCommitProvisionalLoad.

Committed: https://crrev.com/c6d547dcd9331ac29eb261dd2087f9980a2eb197
Cr-Commit-Position: refs/heads/master@{#369577}
==========

[commit-bot: I haz the power, 2016-01-14 22:25:39]

Patchset 1 (id:??) landed as
https://crrev.com/c6d547dcd9331ac29eb261dd2087f9980a2eb197
Cr-Commit-Position: refs/heads/master@{#369577}