Add checks to understand crash in NavigationHandle::DidCommitNavigation

Add checks to understand crash in NavigationHandle::DidCommitNavigation

This CL adds checks to try to understand why a CHECK is being hit in the wild
in NavigationHandle::DidCommitNavigation.

BUG=589365
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[clamy, 2016-02-25 14:52:58]

Description was changed from

==========
Add checks to understand crash in NavigationHandle::DidCommitNavigation

This CL adds checks to try to understand why a CHECK is being hit in the wild
in NavigationHandle::DidCommitNavigation.

BUG=589365
==========

to

==========
Add checks to understand crash in NavigationHandle::DidCommitNavigation

This CL adds checks to try to understand why a CHECK is being hit in the wild
in NavigationHandle::DidCommitNavigation.

BUG=589365
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-02-25 16:25:27]

Description was changed from

==========
Add checks to understand crash in NavigationHandle::DidCommitNavigation

This CL adds checks to try to understand why a CHECK is being hit in the wild
in NavigationHandle::DidCommitNavigation.

BUG=589365
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add checks to understand crash in NavigationHandle::DidCommitNavigation

This CL adds checks to try to understand why a CHECK is being hit in the wild
in NavigationHandle::DidCommitNavigation.

BUG=589365
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-02-25 16:25:27]

clamy@chromium.org changed reviewers:
+ nasko@chromium.org

[clamy, 2016-02-25 16:25:39]

@nasko:PTAL

[nasko, 2016-02-25 16:47:19]

I'm not sure how those two checks will be helpful. The only way for things to go
haywire is for the DidStartLoading notifications to cancel/alter the navigation.
If that was the case, we would've seen other crashes before.

If we remove those checks after a few days, then I'm fine with it, but I don't
see a reason for them to stay in the codebase permanently. LGTM

https://codereview.chromium.org/1735123002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1735123002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1052:
CHECK(navigation_handle_);
I don't see how this will be nullptr. It is tested on line 1025 and 1033. If it
was nullptr, it will be allocated on 1035.

https://codereview.chromium.org/1735123002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1053:
CHECK_EQ(navigation_handle_->GetURL(), validated_params.url);
Same thing here. It is checked on 1026 and if the handle was deleted, the new
one is explicitly initialized with validated_params.url.

[clamy, 2016-02-26 12:41:02]

Thanks! This was meant to be temporary, added a TODO to reflect that.

https://codereview.chromium.org/1735123002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1735123002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1052:
CHECK(navigation_handle_);
On 2016/02/25 16:47:19, nasko wrote:
> I don't see how this will be nullptr. It is tested on line 1025 and 1033. If
it
> was nullptr, it will be allocated on 1035.

Removed the check.

https://codereview.chromium.org/1735123002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1053:
CHECK_EQ(navigation_handle_->GetURL(), validated_params.url);
On 2016/02/25 16:47:19, nasko wrote:
> Same thing here. It is checked on 1026 and if the handle was deleted, the new
> one is explicitly initialized with validated_params.url.

I know but we're still hitting the check in NavigationHandleImpl (which does not
make much sense to me). This is just temporarily there to ensure that we have
the right values before calling NavigatorImpl::DidNavigate.

[clamy, 2016-02-26 12:41:08]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-02-26 12:41:08]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1735123002/#ps40001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-02-26 12:42:12]

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

[commit-bot: I haz the power, 2016-02-26 14:14:20]

Description was changed from

==========
Add checks to understand crash in NavigationHandle::DidCommitNavigation

This CL adds checks to try to understand why a CHECK is being hit in the wild
in NavigationHandle::DidCommitNavigation.

BUG=589365
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add checks to understand crash in NavigationHandle::DidCommitNavigation

This CL adds checks to try to understand why a CHECK is being hit in the wild
in NavigationHandle::DidCommitNavigation.

BUG=589365
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-02-26 14:14:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-26 14:15:23]

Description was changed from

==========
Add checks to understand crash in NavigationHandle::DidCommitNavigation

This CL adds checks to try to understand why a CHECK is being hit in the wild
in NavigationHandle::DidCommitNavigation.

BUG=589365
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add checks to understand crash in NavigationHandle::DidCommitNavigation

This CL adds checks to try to understand why a CHECK is being hit in the wild
in NavigationHandle::DidCommitNavigation.

BUG=589365
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-02-26 14:15:24]

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

[clamy, 2016-03-02 14:28:13]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1756143002/ by clamy@chromium.org.

The reason for reverting is: This has been in Canary for 5 days and was not
triggered, so reverting this temporary CHECK..