OS: Windows 10 Enterprise Version 1703.

Able to reproduce the issue and is a regression broken in M61 Branch for Windows, Mac & Linux OS. Below are the bisect details for the same:

Bisect Info:
============
61.0.3163.58 - Good Build
61.0.3163.59 - Bad Build

Change Log: https://chromium.googlesource.com/chromium/src/+log/61.0.3163.58..61.0.3163.59?pretty=fuller&n=10000

From the above change log suspecting below change could be a possible culprit:

https://chromium.googlesource.com/chromium/src/+/a1f4989b4d6b627346282bfc1431159c8b9eba45

@tyoshino: Assigning to you, request you to please take a look into it. Please help us to find an owner if not with respect to your change. 

P.S: Unable to provide per revision bisect as the issue is regressed in M61 3163 branch, script is showing an error when tried to narrow it. Issue is also observed on canary build 63.0.3214.0.

Adding Blocker label, please undo if not the case.

Thanks.!

cc'ing scheib@ since this is "Blink>Location"

Applying OS= Android, Chrome as this is a Blink issue.

+amineer@ 

Comment 3: a1f4989b4d6b627346282bfc1431159c8b9eba45 only affects the Origin header to be sent. So, I don't think it's the culprit. But I couldn't find any other candidate from the blame list.

It's midnight here. I'll take a look tomorrow.

+Takashi for help as he should be familiar with history and reload.

Not Geolocation related.

Note that this also doesn't work in Firefox and IE. (e.g. same behavior as with PlzNavigate).

removing ReleaseBlock-Stable for now, as this behavior matches Safari as well.

This is indeed specific to the PlzNavigate field trial (--enable-browser-side-navigation).  At first glance, I agree with jam@ that it makes sense to match other browsers here if possible.  (The location.reload() call is happening within the page before the back navigation commits, so it makes sense to me that it might cancel the back navigation.)

That said, we should make sure that PlzNavigate is actually behaving correctly here.  I'm seeing a DCHECK failure on debug builds which might indicate there's a bug here even if we agree with the end result.

void NavigatorImpl::OnBeforeUnloadACK(FrameTreeNode* frame_tree_node,
                                      bool proceed,
                                      const base::TimeTicks& proceed_time) {
  // ...
  DCHECK_EQ(NavigationRequest::WAITING_FOR_RENDERER_RESPONSE,
            navigation_request->state());


clamy@: Can you investigate to see how to fix the DCHECK, and whether there are problems with the current PlzNav behavior?

More bisects using the attached html files reveal the following:

* broken since at least M23

* fixed in 40.0.2214.0
  r303314 "Roll src/third_party/WebKit"
  specifically https://crrev.com/669013002 "Remove BackForwardClient, cleanup starting a history navigation"

* broken in 61.0.3150.0+ r484295 if the following flags are enabled:
  chrome://flags/#network-service
  chrome://flags/#browser-side-navigation

  Disabling both flags fixes the bug in the affected builds, including Canary.

==========
Repro:
  1. open test.html
  2. click the link
  3. click the button
Expected: it navigates back to test.html
Observed: it stays on test2.html

Deleted: test.html 53 bytes

test.html 53 bytes View Download

Deleted: test2.html 120 bytes

test2.html 120 bytes View Download

chrome://flags/#network-service is very experimental, no need to test with it.

#17, indeed, I've mentioned network-service by accident, it's not relevant to the repro in #16.

Not sure if related, here's a bisect with html from #16 with just --enable-browser-side-navigation
419721 and older crash/hang immediately or upon clicking the button (Windows 7)
419731 has the reported bug
https://chromium.googlesource.com/chromium/src/+log/fd0db3c9..abe91890?pretty=fuller
Apparently the crashes were fixed by r419726 "PlzNavigate: fix issue with cache headers during POSTs"

ranjitkan@: Does the QA procedure include any step to ensure that a bisect is run with the same Finch configuration? If not, it should be worth considering.

@ tyoshino: If the finch has to enabled by any Flag we can include the flag while we are invoking the chrome builds using the bisect script. 

In this case, the issue was regressed in between the branch (3163) and the bisect script do not provide or invoke chrome builds when it is broken in between the branches. The script generated "ZeroDivisionError: float division by zero" 

So I have investigated the issue. In terms of the navigation staying on the current page, that seems like a logical behavior. history.back() is not a synchronous navigation so when the reload navigation comes just after the call to history.back(), it cancels the history navigation. Considering that this is also the behavior of all other browsers, I'm inclined to leave it as is.

The DCHECK is being hit because of the following race condition:
- we start a navigation through the NavigationController path (history.back())
- the navigation doesn't start immediately and sends a BeforeUnload IPC to the renderer
- we start a new renderer-initiated navigation via BeginNavigation (reload). This deletes the previous NavigationRequest and starts the new one immediately - because it doesn't need for BeforeUnload to execute. BeforeUnload executed before the renderer sent the navigation to the browser.
- the BeforeUnload IPC comes back, and the NavigationRequest is already started, which it doesn't expect.

I have a simple CL that fixes this by not attempting to start the NavigationRequest in that case: https://chromium-review.googlesource.com/c/chromium/src/+/667144.

Without the CL, the side effect is that we might try to start the navigation twice, which is not ideal from a data consumption POV. Also WCOs will see the navigation starting, then being aborted, and then a new one starting with the same URL. I don't think we should take PlzNavigate off from M61 for that though.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ce7a6154279fe1e5c031716233fb4c1f178a3780

commit ce7a6154279fe1e5c031716233fb4c1f178a3780
Author: clamy <clamy@chromium.org>
Date: Mon Sep 18 16:55:11 2017

PlzNavigate: dont start the requets on BeforeUnloadACK if cancelled

This CL fixes an issue in PlzNavigate where we will try to start the
NavigationRequest of a FrameTreeNode when we receive the BeforeUnload
ACK. However, the NavigationRequest is not the browser-initiated
NavigationRequest that prompted the BeforeUnload IPC to be dispatched,
but a new NavigationRequest created following a BeginNavigation IPC.
Since in that case it already started, we don't need to start it twice.

BUG= 764241 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Id7d7b8fed0bf586be052bffe7243af40da4df79d
Reviewed-on: https://chromium-review.googlesource.com/667144
Reviewed-by: Charlie Reis (OOO until 9/25) <creis@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502592}
[modify] https://crrev.com/ce7a6154279fe1e5c031716233fb4c1f178a3780/content/browser/browser_side_navigation_browsertest.cc
[modify] https://crrev.com/ce7a6154279fe1e5c031716233fb4c1f178a3780/content/browser/frame_host/navigator_impl.cc

This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Can you please confirm and verify if this has been fixed in canary?

Nothing's changed for me in Windows on Chrome Canary 63.0.3221.0 (r503308) using the original StR in c#0 or test files in c#16.
Only disabling PlzNavigate solves the bug just like before.

Comment 30: That's the intention, per comments 13-15.  We intend to match other browsers' behavior in this case, so the fix did not change the outcome.  It was just meant to avoid triggering a DCHECK (crash in a debug build) and starting the navigation twice.

clamy@: Given that there's no visible change in behavior, do we need the merge to M62?  Sounds like the downside is navigating a second time and confusing some observers, but maybe that fix can wait for M63?

Marking fixed, since I think we've reached the desired outcome.

Per discussion with creis@ over chat, marking this merge-rejected due to concerns in comment #31.