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());
+
+  // If this is a stale back/forward (due to a recent navigation the browser

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

Hmm, I wonder why clamy@ disabled the stale back/forward check for PlzNavigate
in OnCommitNavigation.  It seems to me that they would still want it in that
path.

(For PlzNavigate, "CommitNavigation" means "hey RenderFrame, here's a network
response I'd like you to commit; let me know when you're done."  If the
RenderFrame has committed a synchronous navigation in the meantime and we're
telling it to commit a forward navigation, the stale back/forward logic should
catch it.)

At any rate, your change to pull this code out of PrepareRenderViewForNavigation
is correct based on how it's currently structured.  I'll follow up with clamy@
to see if she wants to move it back in.

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

Please do. Right now this code was built as a refactor of what's in there now.
If Camille didn't intend that effect, I'd be happy to rework it.

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

I emailed her to find out.

+  // didn't know about), ignore it. Only check if swapped in because navigations
+  // while swapped out are turned into asynchronous navigations.

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

This comment is a little hard to follow.  Asynchronous navigations are generally
the reason we need the stale back/forward check.

Is it because swapped out RenderFrames won't ever commit a navigation without
asking the browser first?

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

That's what I meant, though I'm taking your word for it.

Rephrasing the comment.

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

Acknowledged.

+  bool is_history_navigation = params.commit_params.page_state.IsValid();
+  if (!render_view_->is_swapped_out() && is_history_navigation &&
+      render_view_->history_list_offset_ !=
+          params.current_history_list_offset) {

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

Yes, I think this will be sufficient to replace the list of page IDs.  I'm
trying to think of a way that the renderer could clear the forward history
without a net change to its history_list_offset_, and I haven't come up with
anything.  (Any back/forward navigation necessarily goes through the browser,
and any new navigation that would clear the forward history necessarily
increases the offset.)

One subtle case: we limit history size to 50.  The renderer could keep
committing new navigations and stay at offset 49.  However, if we were at offset
49 we couldn't have a forward history item, so I think we're safe.

I'm wondering if we can reduce false positives here (since in many cases, a
history navigation to something smaller than the current offset should be safe
even after an offset change).  Maybe we should do that in a followup CL, though,
since that's not always true (thanks to the history size limit).  Perhaps a TODO
would suffice?

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

I'm trying to get things right the first time. I can't think of a concrete case
where this would fire as a false positive, and none of the tests are failing.

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

Oh, here's an easy false positive, unless I'm misunderstanding:

1) Navigate to page A1, then A2.
2) Click Back in the browser process.
3) Before the IPC arrives at the renderer, the RenderFrame commits A2#foo.
4) The IPC arrives at the renderer, telling it to go to A1.

The new stale page ID detection logic would kick in and cause us to ignore it,
because the offset is now 2 but the offset in the IPC was 1.  However, it's
totally safe to load A1.  The browser will still recognize it as a history entry
when it commits.

(The staleness check only matters in cases that the browser won't recognize the
history entry when it commits, like a pruned forward navigation.)

Did you want to try to avoid some of these false positives in this CL?  If so,
we should chat about potential fixes to make sure they work.  (The goal would be
to have fewer cases where the user clicks back/forward and sees nothing happen.)

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

I'd have to talk to you about that. The pruning issue happens for both forwards
and backwards, in slightly different versions, and at that point we would need
to be clever. And I don't like being clever :( I like failing quickly and
simply. If we figured out a way for the renderer to say "hey I dropped your nav"
so that things wouldn't spin infinitely, would that make you happier about
dropping things on the floor in other weird edge cases where it would have been
safe?

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

Yes.  I may still be interested in improving the logic later, but that would
make me happy with it in the meantime.  (Seems especially important not to make
the spinner behavior worse given the current concern around
http://crbug.com/429399.)

+    return;
+  }
+
   bool is_reload =
       RenderViewImpl::IsReload(params.common_params.navigation_type);
   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, params.page_id, &is_reload,
+      &cache_policy)) {
     return;
   }
 
-  int pending_history_list_offset = params.pending_history_list_offset;

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

Do we ever still use pending_history_list_offset?  If not, let's remove it.

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

We do, when history navigations commit. See the !is_new_navigation branch of the
if in RenderFrameImpl::didCommitProvisionalLoad.

-  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 1314 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 1270 matching lines @@
     const GURL& stream_url,
     const CommonNavigationParams& common_params,
     const CommitNavigationParams& commit_params) {
   CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   bool is_reload = false;
   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, -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,
     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))
-    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);
 
   // If this is an attempt to reload while we are swapped out, we should not
@@ skipping 101 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