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

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 "base/auto_reset.h"
@@ skipping 929 matching lines @@
 #endif
   IPC_END_MESSAGE_MAP()
 
   return handled;
 }
 
 void RenderFrameImpl::OnNavigate(const FrameMsg_Navigate_Params& params) {
   TRACE_EVENT2("navigation", "RenderFrameImpl::OnNavigate",
                "id", routing_id_,
                "url", params.common_params.url.possibly_invalid_spec());
+
   bool is_reload =
       RenderViewImpl::IsReload(params.common_params.navigation_type);
+  bool is_history_navigation = params.commit_params.page_state.IsValid();
   WebURLRequest::CachePolicy cache_policy =
       WebURLRequest::UseProtocolCachePolicy;
   if (!RenderFrameImpl::PrepareRenderViewForNavigation(
       params.common_params.url, params.common_params.navigation_type,
-      params.commit_params.page_state, true, params.pending_history_list_offset,
-      params.page_id, &is_reload, &cache_policy)) {
+      params.commit_params.page_state, is_history_navigation,
+      params.current_history_list_offset, params.page_id, &is_reload,
+      &cache_policy)) {
     return;
   }
 
-  int pending_history_list_offset = params.pending_history_list_offset;
-  int current_history_list_offset = params.current_history_list_offset;
-  int current_history_list_length = params.current_history_list_length;
+  render_view_->history_list_offset_ = params.current_history_list_offset;
+  render_view_->history_list_length_ = params.current_history_list_length;
   if (params.should_clear_history_list) {
-    CHECK_EQ(pending_history_list_offset, -1);
-    CHECK_EQ(current_history_list_offset, -1);
-    CHECK_EQ(current_history_list_length, 0);
-  }
-  render_view_->history_list_offset_ = current_history_list_offset;
-  render_view_->history_list_length_ = current_history_list_length;
-  if (render_view_->history_list_length_ >= 0) {
-    render_view_->history_page_ids_.resize(
-        render_view_->history_list_length_, -1);
-  }
-  if (pending_history_list_offset >= 0 &&
-      pending_history_list_offset < render_view_->history_list_length_) {
-    render_view_->history_page_ids_[pending_history_list_offset] =
-        params.page_id;
+    CHECK_EQ(-1, render_view_->history_list_offset_);
+    CHECK_EQ(0, render_view_->history_list_length_);
   }
 
   GetContentClient()->SetActiveURL(params.common_params.url);
 
   WebFrame* frame = frame_;
   if (!params.frame_to_navigate.empty()) {
     // TODO(nasko): Move this lookup to the browser process.
     frame = render_view_->webview()->findFrameByName(
         WebString::fromUTF8(params.frame_to_navigate));
     CHECK(frame) << "Invalid frame name passed: " << params.frame_to_navigate;
@@ skipping 17 matching lines @@
     bool reload_original_url =
         (params.common_params.navigation_type ==
          FrameMsg_Navigate_Type::RELOAD_ORIGINAL_REQUEST_URL);
     bool ignore_cache = (params.common_params.navigation_type ==
                          FrameMsg_Navigate_Type::RELOAD_IGNORING_CACHE);
 
     if (reload_original_url)
       frame->reloadWithOverrideURL(params.common_params.url, true);
     else
       frame->reload(ignore_cache);
-  } else if (params.commit_params.page_state.IsValid()) {
+  } else if (is_history_navigation) {
     // We must know the page ID of the page we are navigating back to.
     DCHECK_NE(params.page_id, -1);
     scoped_ptr<HistoryEntry> entry =
         PageStateToHistoryEntry(params.commit_params.page_state);
     if (entry) {
       // Ensure we didn't save the swapped out URL in UpdateState, since the
       // browser should never be telling us to navigate to swappedout://.
       CHECK(entry->root().urlString() != WebString::fromUTF8(kSwappedOutURL));
       render_view_->history_controller()->GoToEntry(entry.Pass(), cache_policy);
     }
@@ skipping 1318 matching lines @@
     render_view_->webview()->resetScrollAndScaleState();
     internal_data->set_must_reset_scroll_and_scale_state(false);
   }
   internal_data->set_use_error_page(false);
 
   bool is_new_navigation = commit_type == blink::WebStandardCommit;
   if (is_new_navigation) {
     // We bump our Page ID to correspond with the new session history entry.
     render_view_->page_id_ = render_view_->next_page_id_++;
 
-    // Don't update history_page_ids_ (etc) for kSwappedOutURL, since
+    // Don't update history list values for kSwappedOutURL, since
     // we don't want to forget the entry that was there, and since we will
     // never come back to kSwappedOutURL.  Note that we have to call
     // UpdateSessionHistory and update page_id_ even in this case, so that
     // the current entry gets a state update and so that we don't send a
     // state update to the wrong entry when we swap back in.
     if (GetLoadingUrl() != GURL(kSwappedOutURL)) {
       // Advance our offset in session history, applying the length limit.
       // There is now no forward history.
       render_view_->history_list_offset_++;
       if (render_view_->history_list_offset_ >= kMaxSessionHistoryEntries)
         render_view_->history_list_offset_ = kMaxSessionHistoryEntries - 1;
       render_view_->history_list_length_ =
           render_view_->history_list_offset_ + 1;
-      render_view_->history_page_ids_.resize(
-          render_view_->history_list_length_, -1);
-      render_view_->history_page_ids_[render_view_->history_list_offset_] =
-          render_view_->page_id_;
     }
   } else {
     // Inspect the navigation_state on this frame to see if the navigation
     // corresponds to a session history navigation...  Note: |frame| may or
     // may not be the toplevel frame, but for the case of capturing session
     // history, the first committed frame suffices.  We keep track of whether
     // we've seen this commit before so that only capture session history once
     // per navigation.
     //
     // Note that we need to check if the page ID changed. In the case of a
     // reload, the page ID doesn't change, and UpdateSessionHistory gets the
     // previous URL and the current page ID, which would be wrong.
     if (navigation_state->pending_page_id() != -1 &&
         navigation_state->pending_page_id() != render_view_->page_id_ &&
         !navigation_state->request_committed()) {
       // This is a successful session history navigation!
       render_view_->page_id_ = navigation_state->pending_page_id();
 
       render_view_->history_list_offset_ =
           navigation_state->pending_history_list_offset();
-
-      // If the history list is valid, our list of page IDs should be correct.
-      DCHECK(render_view_->history_list_length_ <= 0 ||
-             render_view_->history_list_offset_ < 0 ||
-             render_view_->history_list_offset_ >=
-                 render_view_->history_list_length_ ||
-             render_view_->history_page_ids_[render_view_->history_list_offset_]
-                  == render_view_->page_id_);
     }
   }
 
   bool sent = Send(
       new FrameHostMsg_DidAssignPageId(routing_id_, render_view_->page_id_));
   CHECK(sent);  // http://crbug.com/407376
 
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers_,
                     DidCommitProvisionalLoad(frame, is_new_navigation));
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_,
@@ skipping 1266 matching lines @@
 
 // PlzNavigate
 void RenderFrameImpl::OnCommitNavigation(
     const ResourceResponseHead& response,
     const GURL& stream_url,
     const CommonNavigationParams& common_params,
     const CommitNavigationParams& commit_params) {
   CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   bool is_reload = false;
+  bool is_history_navigation = commit_params.page_state.IsValid();
   WebURLRequest::CachePolicy cache_policy =
       WebURLRequest::UseProtocolCachePolicy;
   if (!RenderFrameImpl::PrepareRenderViewForNavigation(
       common_params.url, common_params.navigation_type,
-      commit_params.page_state, false, -1, -1, &is_reload, &cache_policy)) {
+      commit_params.page_state, is_history_navigation,
+      -999 /* current_history_list_offset */, -1,
+      &is_reload, &cache_policy)) {
     return;
   }
 
   GetContentClient()->SetActiveURL(common_params.url);
 
   // Create a WebURLRequest that blink can use to get access to the body of the
   // response through a stream in the browser. Blink will then commit the
   // navigation.
   // TODO(clamy): Have the navigation commit directly, without going through
   // loading a WebURLRequest.
@@ skipping 401 matching lines @@
 #else
   return scoped_ptr<MediaStreamRendererFactory>(
       static_cast<MediaStreamRendererFactory*>(NULL));
 #endif
 }
 
 bool RenderFrameImpl::PrepareRenderViewForNavigation(
     const GURL& url,
     FrameMsg_Navigate_Type::Value navigate_type,
     const PageState& state,
-    bool check_history,
-    int pending_history_list_offset,
+    bool is_history_navigation,
+    int current_history_list_offset,
     int32 page_id,
     bool* is_reload,
     WebURLRequest::CachePolicy* cache_policy) {
   MaybeHandleDebugURL(url);
   if (!render_view_->webview())
     return false;
 
   FOR_EACH_OBSERVER(
       RenderViewObserver, render_view_->observers_, Navigate(url));
 
   // If this is a stale back/forward (due to a recent navigation the browser
-  // didn't know about), ignore it.
-  if (check_history && render_view_->IsBackForwardToStaleEntry(
-      state, pending_history_list_offset, page_id, *is_reload))
+  // didn't know about), ignore it. Only check if swapped in because if the
+  // frame is swapped out, it won't commit before asking the browser. The value
+  // -999 is a special bogus current_history_list_offset that disables the stale

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

Can we just leave the old check_history bool instead of using a magic number
here?  Then we can just pass -1 from OnCommitNavigation and not worry about what
it means.  The TODO(clamy) can be to remove |check_history|.

[Avi (use Gerrit), 2014/12/08 22:06:03]

Done.

+  // navigation detection; TODO(clamy): hook that up and remove it.
+  if (!render_view_->is_swapped_out() && is_history_navigation &&
+      current_history_list_offset != -999 &&
+      render_view_->history_list_offset_ != current_history_list_offset) {
     return false;
+  }
 
   if (!is_swapped_out_ || frame_->parent())
     return true;
 
   // This is a swapped out main frame, so swap the renderer back in.
   // We marked the view as hidden when swapping the view out, so be sure to
   // reset the visibility state before navigating to the new URL.
   render_view_->webview()->setVisibilityState(
       render_view_->visibilityState(), false);
 
@@ skipping 102 matching lines @@
 
 #if defined(ENABLE_BROWSER_CDMS)
 RendererCdmManager* RenderFrameImpl::GetCdmManager() {
   if (!cdm_manager_)
     cdm_manager_ = new RendererCdmManager(this);
   return cdm_manager_;
 }
 #endif  // defined(ENABLE_BROWSER_CDMS)
 
 }  // namespace content