Only use pending navigation params for browser-initiated navigations.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 75bb31eb4732ef4c141537b0b8d236f3bd05a214..c1d6013a7ef146a8ba16551c16280fd66679898f 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -3566,22 +3566,26 @@ void RenderFrameImpl::didFinishLoad(blink::WebLocalFrame* frame) {
                                       ds->request().url()));
 }
 
-void RenderFrameImpl::didNavigateWithinPage(blink::WebLocalFrame* frame,
+void RenderFrameImpl::didNavigateWithinPage(
+    blink::WebLocalFrame* frame,
     const blink::WebHistoryItem& item,
-    blink::WebHistoryCommitType commit_type) {
+    blink::WebHistoryCommitType commit_type,
+    bool content_initiated) {
   TRACE_EVENT1("navigation", "RenderFrameImpl::didNavigateWithinPage",
                "id", routing_id_);
   DCHECK_EQ(frame_, frame);
-  // If this was a reference fragment navigation that we initiated, then we
-  // could end up having a non-null pending navigation params.  We just need to
-  // update the ExtraData on the datasource so that others who read the
-  // ExtraData will get the new NavigationState.  Similarly, if we did not
-  // initiate this navigation, then we need to take care to reset any pre-
-  // existing navigation state to a content-initiated navigation state.
-  // UpdateNavigationState conveniently takes care of this for us.
+  // If this was a browser-initiated navigation, then there could be pending
+  // navigation params, so use them. Otherwise, just reset the document state
+  // here, since if pending navigation params exist they are for some other
+  // navigation <https://crbug.com/597239>.
   DocumentState* document_state =
       DocumentState::FromDataSource(frame->dataSource());
-  UpdateNavigationState(document_state, true /* was_within_same_page */);
+  if (content_initiated) {

[Charlie Reis, 2016/05/12 20:51:48]

Would it be better to put this fix inside UpdateNavigationState?  That method is
incorrectly assuming that the presence of pending_navigation_params_ means the
current navigation is browser-initiated, and it's called from more than just
this location (i.e., also didCreateDataSource).

Or do we not know if it's browser-initiated in didCreateDataSource?  (Could
there be a similar bug on that path?)

[Avi (use Gerrit), 2016/05/12 23:41:17]

Right now we don't know if it's browser-initiated in didCreateDataSource. I
suppose there might be a similar bug there, but I'm straining to figure that
out.

It might be worth it to plumb that too, just to cover ourselves.

[Charlie Reis, 2016/05/13 00:46:17]

I could be ok leaving it as a TODO for this CL and fixing it next.  That said,
we probably still want to change UpdateNavigationState to take in this bool. 
Then didCreateDataSource can have a "bool content_initiated =
!!pending_nav_params_;" with a giant TODO over it saying that it's not a safe
assumption.

That way we know exactly where the remaining bug is, and any future callers of
UpdateNavigationState are forced to get it right.

[Avi (use Gerrit), 2016/05/13 01:35:44]

It's probably not difficult to plumb it too. I'll look at it after writing a
test. Perhaps you want it in its own CL anyway.

[Avi (use Gerrit), 2016/05/13 16:45:19]

FYI, at the beginning of RenderFrameImpl::didCreateDataSource it already makes
that EXACT ASSUMPTION:

void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
                                          blink::WebDataSource* datasource) {
  DCHECK(!frame_ || frame_ == frame);

  bool content_initiated = !pending_navigation_params_.get();

[Charlie Reis, 2016/05/13 18:26:55]

Ha!  Ok, yes, let's revisit that case (and any others) after this lands.  This
CL is strictly better than what we have today.

+    document_state->set_navigation_state(
+        NavigationStateImpl::CreateContentInitiated());
+  } else {
+    UpdateNavigationState(document_state, true /* was_within_same_page */);
+  }
   static_cast<NavigationStateImpl*>(document_state->navigation_state())
       ->set_was_within_same_page(true);