Fix aborted navigates not clearing the pending navigation entry with PlzNavigate.

Fix aborted navigates not clearing the pending navigation entry with PlzNavigate.

This fixes BrowserTest.ClearPendingOnFailUnlessNTP.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[jam, 2016-10-01 05:57:34]

Description was changed from

==========
Fix aborted navigates not clearing the pending navigation entry with
PlzNavigate.

This fixes BrowserTest.ClearPendingOnFailUnlessNTP.

BUG=504347
==========

to

==========
Fix aborted navigates not clearing the pending navigation entry with
PlzNavigate.

This fixes BrowserTest.ClearPendingOnFailUnlessNTP.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2016-10-01 05:57:39]

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

[commit-bot: I haz the power, 2016-10-01 05:57:45]

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-01 06:29:30]

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

[commit-bot: I haz the power, 2016-10-01 06:29:33]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jam, 2016-10-03 04:02:44]

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

[jam, 2016-10-03 16:19:13]

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

[jam, 2016-10-03 16:19:14]

redirecting to Nasko since Camille was ooo

[nasko, 2016-10-03 16:48:08]

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

https://codereview.chromium.org/2380383004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:360:
frame_tree_node_->navigator()->FailedNavigation(
If not clearing the pending entry is the reason for this not working, shouldn't
we add that to ResetNavigationRequest? It makes a bit more sense to me than
calling FailedNavigation, especially since 204 and 205 are actually not errors.

[jam, 2016-10-03 17:09:16]

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

https://codereview.chromium.org/2380383004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:360:
frame_tree_node_->navigator()->FailedNavigation(
On 2016/10/03 16:48:08, nasko (slow) wrote:
> If not clearing the pending entry is the reason for this not working,
shouldn't
> we add that to ResetNavigationRequest? 

It wasn't clear to me that we want to clear the pending entry for all the other
callsites of ResetNavigationRequest?

I'll send tryjobs doing this to see if anything breaks.

> It makes a bit more sense to me than
> calling FailedNavigation, especially since 204 and 205 are actually not
errors.

[jam, 2016-10-03 17:11:07]

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

[commit-bot: I haz the power, 2016-10-03 17:11:38]

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-03 18:36:56]

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

[commit-bot: I haz the power, 2016-10-03 18:36:57]

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_...)

[jam, 2016-10-03 21:06:56]

ok check out some of the test failures by always doing it in
ResetNavigationRequest, e.g.

#0 0x00000308c41e base::debug::StackTrace::StackTrace()
#1 0x00000309cf5b logging::LogMessage::~LogMessage()
#2 0x000002556abd content::NavigationControllerImpl::DiscardPendingEntry()
#3 0x00000256408f content::NavigatorImpl::DiscardPendingEntryOnFailureIfNeeded()
#4 0x00000254cf37 content::FrameTreeNode::ResetNavigationRequest()
#5 0x0000028d9ce0 content::TestWebContents::TestSetIsLoading()
#6 0x00000116f4d3
content::ImmediateLoadObserver::DidStartNavigationToPendingEntry()
#7 0x00000281120a content::WebContentsImpl::DidStartNavigationToPendingEntry()
#8 0x000002561e73 content::NavigatorImpl::NavigateToEntry()
#9 0x000002562385 content::NavigatorImpl::NavigateToPendingEntry()
#10 0x00000255668a
content::NavigationControllerImpl::NavigateToPendingEntryInternal()
#11 0x000002551d85 content::NavigationControllerImpl::NavigateToPendingEntry()
#12 0x0000025525f6 content::NavigationControllerImpl::GoToIndex()
#13 0x0000027fb9b4 content::OverscrollNavigationOverlay::OnOverscrollCompleted()

So we'd have to expose in_navigate_to_pending_entry_ from
NavigationControllerImpl to FrameTreeNode, which seems like an implementation
detail. We also have to expose DiscardPendingEntryOnFailureIfNeeded. Even if we
rename it so that "failure" is not in the method name, it still seems
essentially the same thing. So far, I prefer patchset 1 or we can expose
DiscardPendingEntryOnFailureIfNeeded as patchset 2 does and just call it
directly from NavigationRequest::OnResponseStarted.

[jam, 2016-10-03 21:25:33]

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

[commit-bot: I haz the power, 2016-10-03 21:26:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nasko, 2016-10-03 22:06:19]

On 2016/10/03 21:06:56, jam wrote:
> ok check out some of the test failures by always doing it in
> ResetNavigationRequest, e.g.
> 
> #0 0x00000308c41e base::debug::StackTrace::StackTrace()
> #1 0x00000309cf5b logging::LogMessage::~LogMessage()
> #2 0x000002556abd content::NavigationControllerImpl::DiscardPendingEntry()
> #3 0x00000256408f
content::NavigatorImpl::DiscardPendingEntryOnFailureIfNeeded()
> #4 0x00000254cf37 content::FrameTreeNode::ResetNavigationRequest()
> #5 0x0000028d9ce0 content::TestWebContents::TestSetIsLoading()
> #6 0x00000116f4d3
> content::ImmediateLoadObserver::DidStartNavigationToPendingEntry()
> #7 0x00000281120a content::WebContentsImpl::DidStartNavigationToPendingEntry()
> #8 0x000002561e73 content::NavigatorImpl::NavigateToEntry()
> #9 0x000002562385 content::NavigatorImpl::NavigateToPendingEntry()
> #10 0x00000255668a
> content::NavigationControllerImpl::NavigateToPendingEntryInternal()
> #11 0x000002551d85 content::NavigationControllerImpl::NavigateToPendingEntry()
> #12 0x0000025525f6 content::NavigationControllerImpl::GoToIndex()
> #13 0x0000027fb9b4
content::OverscrollNavigationOverlay::OnOverscrollCompleted()
> 
> So we'd have to expose in_navigate_to_pending_entry_ from
> NavigationControllerImpl to FrameTreeNode, which seems like an implementation
> detail. We also have to expose DiscardPendingEntryOnFailureIfNeeded. Even if
we
> rename it so that "failure" is not in the method name, it still seems
> essentially the same thing. So far, I prefer patchset 1 or we can expose
> DiscardPendingEntryOnFailureIfNeeded as patchset 2 does and just call it
> directly from NavigationRequest::OnResponseStarted.

I agree that the first patch looked better. Let's just add a comment saying that
204/205 don't commit anything, so they are similar to the user aborting the
navigation.

[nasko, 2016-10-03 22:14:36]

As discussed, PS3 LGTM!

[jam, 2016-10-03 22:16:28]

The CQ bit was unchecked by jam@chromium.org

[jam, 2016-10-03 22:16:30]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2016-10-03 22:16:49]

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-03 23:06:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-03 23:08:45]

Description was changed from

==========
Fix aborted navigates not clearing the pending navigation entry with
PlzNavigate.

This fixes BrowserTest.ClearPendingOnFailUnlessNTP.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix aborted navigates not clearing the pending navigation entry with
PlzNavigate.

This fixes BrowserTest.ClearPendingOnFailUnlessNTP.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-10-03 23:08:46]

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