[TEST] Check when the BackForward PageTransition is dropped.

[TEST] Check when the BackForward PageTransition is dropped.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel

[arthursonzogni, 2017-06-23 15:57:04]

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

[commit-bot: I haz the power, 2017-06-23 15:57:12]

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, 2017-06-23 16:23:23]

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

[commit-bot: I haz the power, 2017-06-23 16:23:23]

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

[arthursonzogni, 2017-06-26 08:32:17]

arthursonzogni@chromium.org changed reviewers:
+ clamy@chromium.org, jam@chromium.org, nasko@chromium.org

[arthursonzogni, 2017-06-26 08:32:18]

FYI: I rerun nasko's CL (https://codereview.chromium.org/2951483002/) with a
modified CHECK(<condition>). It looks for navigation that starts with a
ui::PAGE_TRANSITION_FORWARD_BACK and ends with something else.

Unfortunately this CHECK is triggered with and without PlzNavigate. There is
still a few one that fails only with PlzNavigate. For instance:

content_browsertests:
* NavigationControllerBrowserTest.SubframeBackFromReplaceState

content_unittests:
* RenderFrameHostManagerTest.NavigateAfterMissingSwapOutACK
* NavigationControllerTest.GoBackWithUserAgentOverrideChange
* NavigationControllerTest.RemoveEntry
* NavigationControllerTest.BackSubframe
* OverscrollNavigationOverlayTest.Navigation_PaintUpdate
* NavigationControllerTest.GoToOffset
* NavigationControllerTest.Forward
* RenderProcessHostUnitTest.DoNotReuseError
* NavigationControllerTest.RestoreNavigateAfterFailure
* NavigationControllerTest.RestoreNavigate
* NavigationControllerTest.TransientEntry
* RenderFrameHostManagerTest.DisownOpenerAfterNavigation
* NavigationControllerTest.Forward_GeneratesNewPage
* NavigationControllerTest.Back_GeneratesNewPage
* WebContentsImplTest.NavigationEntryContentState
* OverscrollNavigationOverlayTest.CancelAfterSuccessfulNavigation
* WebContentsImplTest.NavigateFromRestoredRegularUrl
* WebContentsImplTest.NavigateFromRestoredSitelessUrl
* NavigationControllerTest.LoadURL_WithBindings
* NavigationControllerTest.RemoveEntryWithPending
* RenderFrameHostManagerTest.DisownOpenerDuringNavigation

And a lot of webkit layout tests.

[arthursonzogni, 2017-06-26 13:55:53]

Patchset #2 (id:20001) has been deleted

[Charlie Reis, 2017-06-27 03:59:28]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2017-06-27 03:59:29]

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

https://codereview.chromium.org/2958573002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:750: params.transition &
ui::PAGE_TRANSITION_FORWARD_BACK);
Are you worried at all about the other direction?  It seems odd to allow cases
that FORWARD_BACK wasn't initially true but becomes true during the navigation. 
(That is, I'm curious why this is an A => B check instead of A == B.)

[arthursonzogni, 2017-06-27 09:12:59]

On 2017/06/27 03:59:29, Charlie Reis (slow) wrote:
>
https://codereview.chromium.org/2958573002/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/navigation_handle_impl.cc (right):
> 
>
https://codereview.chromium.org/2958573002/diff/1/content/browser/frame_host/...
> content/browser/frame_host/navigation_handle_impl.cc:750: params.transition &
> ui::PAGE_TRANSITION_FORWARD_BACK);
> Are you worried at all about the other direction?  It seems odd to allow cases
> that FORWARD_BACK wasn't initially true but becomes true during the
navigation. 
> (That is, I'm curious why this is an A => B check instead of A == B.)

Yes, this CL does only (A => B)
Nasko's CL (https://codereview.chromium.org/2951483002/) does (A == B)

I wanted to look first at the cases I am the most interested in, hence this CL.
Yes, I agree that A <= B needs some investigations too.

[clamy, 2017-06-27 10:25:27]

On 2017/06/27 09:12:59, arthursonzogni wrote:
> On 2017/06/27 03:59:29, Charlie Reis (slow) wrote:
> >
>
https://codereview.chromium.org/2958573002/diff/1/content/browser/frame_host/...
> > File content/browser/frame_host/navigation_handle_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2958573002/diff/1/content/browser/frame_host/...
> > content/browser/frame_host/navigation_handle_impl.cc:750: params.transition
&
> > ui::PAGE_TRANSITION_FORWARD_BACK);
> > Are you worried at all about the other direction?  It seems odd to allow
cases
> > that FORWARD_BACK wasn't initially true but becomes true during the
> navigation. 
> > (That is, I'm curious why this is an A => B check instead of A == B.)
> 
> Yes, this CL does only (A => B)
> Nasko's CL (https://codereview.chromium.org/2951483002/) does (A == B)
> 
> I wanted to look first at the cases I am the most interested in, hence this
CL.
> Yes, I agree that A <= B needs some investigations too.

So a bit background here. We're investigating a regression in bf navigation
performance with PlzNavigate enabled. We landed specific metrics, and realized
that half of the bf navigations at ready to commit stage are not longer
considered bf navigations when they commit. Which is weird. I wrote the CL
Arthur referenced (it's not Nasko's :)) in order to check if this kind of things
was happening in our tests. Nasko investigated the reverse case from this CL, ie
not bf becomes bf at commit, and found it happens for pushStates. Since we
initialize the transition at WillStartRequest stage, which doesn't happen for
same-document navigations, we only get the right transition when we call
DidCommitNavigation on the NavigationHandle. Now we're trying to understand the
case where the transition has the bf flag at the start, but it is dropped at
commit.