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

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 a7a8d1396ae0f0731399c375323694d8291e38cc..00aed12de332000adfb52e920c76a3ae01a02933 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -947,35 +947,33 @@ 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);
@@ -1013,7 +1011,7 @@ void RenderFrameImpl::OnNavigate(const FrameMsg_Navigate_Params& params) {
       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 =
@@ -2348,7 +2346,7 @@ void RenderFrameImpl::didCommitProvisionalLoad(
     // 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
@@ -2362,10 +2360,6 @@ void RenderFrameImpl::didCommitProvisionalLoad(
         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
@@ -2386,14 +2380,6 @@ void RenderFrameImpl::didCommitProvisionalLoad(
 
       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_);
     }
   }
 
@@ -3684,7 +3670,7 @@ void RenderFrameImpl::OnCommitNavigation(
       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;
   }
 
@@ -4106,8 +4092,6 @@ 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) {
@@ -4118,12 +4102,6 @@ bool RenderFrameImpl::PrepareRenderViewForNavigation(
   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;