This is a challenging test to get right, as I learned last year.  :)  The test itself is preventing regression of  issue 348447 .  It has a page which uses script code to create and navigate a frame (treated as a "client redirect"), and it checks that we won't load the corresponding history item if you go back to the page.  (Instead, the client redirect script navigation is expected to win.)

Nate's original fix in https://codereview.chromium.org/296533004/ caused us to abort the history navigation in NavigationScheduler::schedule if a client redirect comes in before the history navigation commits.

When I switched to the new session history logic (where subframe history items are kept in the browser process), I needed to find different ways to abort this history navigation.  As of https://codereview.chromium.org/2208933002/, we check in two places:
1) RenderFrameImpl::NavigateInternal, which ignores an incoming history item if a client redirect has occurred in the meantime.
2) RenderFrameImpl::decidePolicyForNavigation, which sends a FrameHostMsg_CancelInitialHistoryLoad to the browser if a client redirect occurs after the renderer has already asked for a history item for the frame.  (This is important for the cross-process case, where the history item would end up sent to a different process.)

All of these fixes likely have a bit of raciness involved, I think, but tend to do the right thing.  Similar bugs in this area have caused some major regressions (e.g., issue 626416 and  issue 657896 ), so we need to be careful about how to fix this for PlzNavigate.

In PlzNavigate, the history item is not sent to the renderer process until it's ready to commit, so my earlier fix in RenderFrameImpl::NavigateInternal can't help.  We generally don't want the renderer to ignore commits sent down by the browser process either (especially since they might be sent to a different process that doesn't know about the client redirect).  Instead, it seems like we should get FrameHostMsg_CancelInitialHistoryLoad to cancel the history NavigationRequest in the browser process, so that the commit never gets sent to the renderer.

ananta@: Does that approach get the test to pass?  I think it's closer to the intent of the original fixes than https://codereview.chromium.org/2697753002/.  (I don't see the connection with whether the history navigation is considered renderer-initiated, which seems orthogonal.)

Hi Charlie

Thanks for the detailed explanation :) When we receive a history navigation in the browser for PlzNavigate (NavigatorImpl::RequestNavigation()) is called.
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_impl.cc?q=::RequestNAv+package:%5Echromium$&l=1199

It waits for the unload handler on the page to run before resuming the navigation. Before the unload handler ack comes back from the renderer, we get a BeginNavigation IPC for the client redirect. That ends up getting cancelled as we have a browser initiated navigation pening.

I tried sending the FrameHostMsg_CancelInitialHistoryLoad  IPC in RenderFrameImpl::BeginNavigate() before the BeginNavigation IPC. That does not work because we don't have a pending RenderFrameHost in the browser as the navigation has not started yet. If you are suggesting that we also look into cancelling the navigation request which hasn't started yet, then please let me know. I will try that anyways to see the result.

Hi Charlie

After some more debugging, there are cases where the history navigation starts in the browser (NavigationRequest state is STARTED) and then the BeginNavigation IPC comes in from the renderer for the redirect. This is then aborted in favor of the history navigation which is flagged as a browser navigation).

I tried this change in the RenderFrameHostImpl::OnCancelInitialHistoryLoad() function which works. We send the IPC in the RenderFrameImpl::BeginNavigation() function. 

Please look at the else statement in this function.

void RenderFrameHostImpl::OnCancelInitialHistoryLoad() {
  // A Javascript navigation interrupted the initial history load.  Check if an
  // initial subframe cross-process navigation needs to be canceled as a result.
  // TODO(creis, clamy): Cancel any cross-process navigation in PlzNavigate.
  if (GetParent() && !frame_tree_node_->has_committed_real_load()) {
    if (frame_tree_node_->render_manager()->pending_frame_host()) {
      frame_tree_node_->render_manager()->CancelPendingIfNecessary(
        frame_tree_node_->render_manager()->pending_frame_host());
    } else {
      // This code is new.
      NavigationRequest* request = frame_tree_node_->navigation_request();
      if (request &&
          request->request_params().is_history_navigation_in_new_child &&
              (request->state() != NavigationRequest::RESPONSE_STARTED)) {
          frame_tree_node_->ResetNavigationRequest(false);
      }
    }
  }
} 

Thanks for the extra details!  I see why you were looking at browser vs renderer initiated navigations now (due to the check in NavigatorImpl::OnBeginNavigation which aborts the client redirect due to the presence of the history NavigationRequest).

In comment 2, I think you're talking about the beforeunload logic, right?  That's different from unload, which runs in the background after commit.  (Those are two separate JS events.)

I'm curious, does the beforeunload logic end up affecting the outcome?  Whether we've heard the beforeunload ACK or not when the client redirect's BeginNavigation IPC arrives, we'll have the NavigationRequest for the history navigation on the FrameTreeNode.  I think it only affects whether we're in RESPONSE_STARTED or WAITING_FOR_RENDERER_RESPONSE and whether there's a speculative RFH, right?

Also, I agree that there won't be a pending_frame_host() on frame_tree_node_->render_manager(), since we don't create pending RFHs at all in PlzNavigate.  (There may or may not be a speculative one, depending on whether the beforeunload ACK has arrived.)

Anyway, here's my thoughts:

1) Your approach to modify OnCancelInitialHistoryLoad to cancel the NavigationRequest if it's a history navigation in a new child makes sense to me-- that ensures that the BeginNavigation IPC for the client redirect will be used instead.

2) I think we can simplify that approach to not depend on OnCancelInitialHistoryLoad at all.  Instead, we can change NavigatorImpl::OnBeginNavigation to cancel the ongoing_navigation_request if it is_history_navigation_in_new_child, just before the renderer-initiated vs browser-initiated check that it currently does.  In that sense, let the BeginNavigation IPC be the only signal we need, and avoid the need for sending a separate FrameHostMsg_CancelInitialHistoryLoad first.  Something like this?

  // Client redirects during the initial history navigation of a child frame
  // should take precedence over the history navigation (despite being renderer-
  // initiated).  See  https://crbug.com/348447  and  https://crbug.com/691168 .
  if (ongoing_navigation_request &&
      ongoing_navigation_request->request_params()
          .is_history_navigation_in_new_child) {
    // Preemptively clear this local pointer before deleting the request.
    ongoing_navigation_request = nullptr;
    frame_tree_node->ResetNavigationRequest(false);
  }


3) Technically, it seems like we don't need to run beforeunload in the case of is_history_navigation_in_new_child (in NavigatorImpl::RequestNavigation), since this is a newly created child frame that can't really have a beforeunload handler yet.  This may not be necessary for fixing the bug, but it does seem like it might simplify the timeline in this case.  (Might be worth doing as a separate CL?)

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

commit 232b610b1a9ae4893764f54a0c952f11e1619686
Author: ananta <ananta@chromium.org>
Date: Sun Feb 19 03:30:34 2017

PlzNavigate: Cancel ongoing history navigations in the browser when we receive a navigation request from the renderer.

This should fix the http/tests/navigation/back-to-dynamic-iframe.html layout test which
initiates history navigations on the frame and in the process also changes the src tag on the
frame in JS. The expectation is that the JS changes should win. In the PlzNavigate case the history navigations win
because they are marked as browser initiated.

Proposed fix with a lot of help from creis is to cancel ongoing history navigations for child frames when we receive
a navigation request for a frame. Additionally we don't run unload handlers for history navigations for newly created
child frames as they won't have any.

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

Review-Url: https://codereview.chromium.org/2701093002
Cr-Commit-Position: refs/heads/master@{#451507}

[modify] https://crrev.com/232b610b1a9ae4893764f54a0c952f11e1619686/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/232b610b1a9ae4893764f54a0c952f11e1619686/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation