Only use pending navigation params for browser-initiated navigations.

content/renderer/render_frame_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 3548 matching lines @@
   }
 
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers(),
                     DidFinishLoad(frame));
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, DidFinishLoad());
 
   Send(new FrameHostMsg_DidFinishLoad(routing_id_,
                                       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);
 
   didCommitProvisionalLoad(frame, item, commit_type);
 }
 
 void RenderFrameImpl::didUpdateCurrentHistoryItem() {
   render_view_->StartNavStateSyncTimerIfNecessary(this);
 }
 
@@ skipping 2549 matching lines @@
   // event target. Potentially a Pepper plugin will receive the event.
   // In order to tell whether a plugin gets the last mouse event and which it
   // is, we set |pepper_last_mouse_event_target_| to null here. If a plugin gets
   // the event, it will notify us via DidReceiveMouseEvent() and set itself as
   // |pepper_last_mouse_event_target_|.
   pepper_last_mouse_event_target_ = nullptr;
 #endif
 }
 
 }  // namespace content