Diagnose RenderFrameImpl::didCommitProvisionalLoad crash.

Diagnose RenderFrameImpl::didCommitProvisionalLoad crash.

These temporary crash keys should reveal whether the RenderView has
changed or been deleted between the local-to-remote swap and the
remote-to-local commit.

BUG=575245
TEST=New crash keys present in crash reports.

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

[Charlie Reis, 2016-01-20 19:59:49]

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

[Charlie Reis, 2016-01-20 19:59:49]

Robert: Can you review crash_keys.cc?

Nasko: Can you review render_frame_impl.cc?  I'm trying to understand how
main_render_frame_ could be non-null in didCommitProvisionalLoad, if it's null
at the end of SwapOut.  I'm hoping these keys will reveal whether we're talking
about the same RenderView (or a possibly deleted RenderView, in which case we
might crash on the main_render_frame_->GetRoutingID() call).

It's possible that some other tab in the same process might swap or commit
before the didCommit crash, and thus the swap IDs won't be for the same
conceptual frame as the commit IDs.  We can ignore those cases by checking if
the swap_proxy_id and commit_proxy_id match or not.

[Robert Sesek, 2016-01-20 20:00:17]

lgtm

[nasko, 2016-01-20 21:03:11]

Just one question, otherwise it looks fine.

https://codereview.chromium.org/1613453002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1613453002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3122:
base::IntToString(proxy->routing_id()));
Wouldn't |proxy| be deleted by now? The swap() call above should result in a
detach for the RemoteFrame and the associated proxy will be deleted.

[Charlie Reis, 2016-01-20 21:16:46]

Thanks!  Fixed.

https://codereview.chromium.org/1613453002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1613453002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3122:
base::IntToString(proxy->routing_id()));
On 2016/01/20 21:03:11, nasko wrote:
> Wouldn't |proxy| be deleted by now? The swap() call above should result in a
> detach for the RemoteFrame and the associated proxy will be deleted.

You're right!  It's garbage here.  Good catch.

[Charlie Reis, 2016-01-20 21:24:04]

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

[commit-bot: I haz the power, 2016-01-20 21:25:44]

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

[nasko, 2016-01-20 21:29:52]

LGTM

[Charlie Reis, 2016-01-20 21:31:01]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2016-01-20 21:31:06]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2016-01-20 21:31:06]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/1613453002/#ps40001
(title: "Fix proxy bug")

[commit-bot: I haz the power, 2016-01-20 21:33:34]

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

[commit-bot: I haz the power, 2016-01-20 23:07:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-20 23:08:24]

Description was changed from

==========
Diagnose RenderFrameImpl::didCommitProvisionalLoad crash.

These temporary crash keys should reveal whether the RenderView has
changed or been deleted between the local-to-remote swap and the
remote-to-local commit.

BUG=575245
TEST=New crash keys present in crash reports.
==========

to

==========
Diagnose RenderFrameImpl::didCommitProvisionalLoad crash.

These temporary crash keys should reveal whether the RenderView has
changed or been deleted between the local-to-remote swap and the
remote-to-local commit.

BUG=575245
TEST=New crash keys present in crash reports.

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

[commit-bot: I haz the power, 2016-01-20 23:08:25]

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