Avoid stale navigation requests without excessive page id knowledge in the renderer process.

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 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/browser/web_contents/web_contents_impl.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 1920 matching lines @@
                     DidAttachInterstitialPage());
 }
 
 void WebContentsImpl::DetachInterstitialPage() {
   if (ShowingInterstitialPage())
     GetRenderManager()->remove_interstitial_page();
   FOR_EACH_OBSERVER(WebContentsObserver, observers_,
                     DidDetachInterstitialPage());
 }
 
-void WebContentsImpl::SetHistoryLengthAndPrune(
-    const SiteInstance* site_instance,
-    int history_length,
-    int32 minimum_page_id) {
-  // SetHistoryLengthAndPrune doesn't work when there are pending cross-site
+void WebContentsImpl::SetHistoryOffsetAndLength(int history_offset,
+                                                int history_length) {
+  // SetHistoryOffsetAndLength doesn't work when there are pending cross-site

[Charlie Reis, 2014/12/03 23:48:07]

Does this matter anymore?

It would be great to remove this if not, because it would silently corrupt the
renderer's view of history length/offset in release builds if we got it wrong.

(I also worry it would affect the CreateRenderViewForRenderManager call below,
though perhaps that happens as we're creating the pending RVH and before we've
assigned it.)

[Avi (use Gerrit), 2014/12/04 21:15:16]

In what way?

Blink needs to know where we are in the history list. If someone does pruning in
the browser process, the renderer needs to know somehow.

[Charlie Reis, 2014/12/04 23:13:58]

Sorry, I was ambiguous about "this."  I meant this block (1943-1948), where we
skip the IPC if there's a pending RenderViewHost.  Skipping the IPC in some
cases makes this harder to reason about, and I'm not sure we need to anymore.

[Avi (use Gerrit), 2014/12/05 22:16:39]

I'm not sure why this was put in. If it's about the list of page ids, then sure,
let's nuke it. I'm tending to think that way (why would sending two numbers be
affected by cross-site navigations?), but I'm not sure. I'm going to pull it and
see what happens.

[Charlie Reis, 2014/12/05 22:51:15]

Acknowledged.

   // navigations. Callers should ensure that this is the case.
   if (GetRenderManager()->pending_render_view_host()) {
     NOTREACHED();
     return;
   }
-  RenderViewHostImpl* rvh = GetRenderViewHostImpl();
-  if (!rvh) {
-    NOTREACHED();
-    return;
-  }
-  if (site_instance && rvh->GetSiteInstance() != site_instance) {
-    NOTREACHED();
-    return;
-  }
-  Send(new ViewMsg_SetHistoryLengthAndPrune(GetRoutingID(),
-                                            history_length,
-                                            minimum_page_id));
+  SetHistoryOffsetAndLengthForView(
+      GetRenderViewHost(), history_offset, history_length);
+}
+
+void WebContentsImpl::SetHistoryOffsetAndLengthForView(
+    RenderViewHost* render_view_host,
+    int history_offset,
+    int history_length) {
+  render_view_host->Send(new ViewMsg_SetHistoryOffsetAndLength(
+      render_view_host->GetRoutingID(), history_offset, history_length));
 }
 
 void WebContentsImpl::ReloadFocusedFrame(bool ignore_cache) {
   RenderFrameHost* focused_frame = GetFocusedFrame();
   if (!focused_frame)
     return;
 
   focused_frame->Send(new FrameMsg_Reload(
       focused_frame->GetRoutingID(), ignore_cache));
 }
@@ skipping 2163 matching lines @@
 
   if (!static_cast<RenderViewHostImpl*>(
           render_view_host)->CreateRenderView(base::string16(),
                                               opener_route_id,
                                               proxy_routing_id,
                                               max_page_id,
                                               created_with_opener_)) {
     return false;
   }
 
+  SetHistoryOffsetAndLengthForView(render_view_host,

[Charlie Reis, 2014/12/03 23:48:07]

Interesting.  We don't have a similar call here today-- is that a bug, or was it
handled somewhere else?

[Avi (use Gerrit), 2014/12/04 21:15:16]

Today, those values are set with the navigation, with values in
FrameMsg_Navigate_Params.

But in the new world, we're deciding if something is a stale nav by comparing
the values in FrameMsg_Navigate_Params to the current values. With a new
RenderView, the first nav that comes in would fail because the values in
FrameMsg_Navigate_Params would be compared to the blank ones.

So either we set a flag in the RenderView that says "we were just initialized
and we know that these history list values are bogus so don't worry about the
check" or we do this, to actually set up the values. I chose the latter but am
not attached to it.

[Charlie Reis, 2014/12/04 23:13:58]

I like your approach.  Let's keep it.

+                                   controller_.GetLastCommittedEntryIndex(),
+                                   controller_.GetEntryCount());
+
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
   // Force a ViewMsg_Resize to be sent, needed to make plugins show up on
   // linux. See crbug.com/83941.
   if (rwh_view) {
     if (RenderWidgetHost* render_widget_host = rwh_view->GetRenderWidgetHost())
       render_widget_host->WasResized();
   }
 #endif
 
   return true;
@@ skipping 200 matching lines @@
   node->render_manager()->ResumeResponseDeferredAtStart();
 }
 
 void WebContentsImpl::SetForceDisableOverscrollContent(bool force_disable) {
   force_disable_overscroll_content_ = force_disable;
   if (view_)
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
 }  // namespace content