Classify navigations without page id in parallel to the existing classifier.

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 8915dea7ec91fbd213aceb3082888316ef559384..c2b0bc967f3cc6f56955897211c3f667179a4cdf 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1117,6 +1117,10 @@ void RenderFrameImpl::OnNavigate(
   } else if (is_history_navigation) {
     // We must know the page ID of the page we are navigating back to.
     DCHECK_NE(request_params.page_id, -1);
+    // We must know the nav entry ID of the page we are navigating back to,
+    // which should be the case because history navigations are routed via the
+    // browser.
+    DCHECK_NE(0, request_params.nav_entry_id);

[clamy, 2015/04/10 12:05:44]

Considering that we are in an IPC handler for a browser-initiated navigation,
shouldn't we always have created an entry for the navigation, and so have
request_params.nav_entry_id not be 0? I guess the only problem would be some
RenderViewTests which call Onnavigate directly, but the RequestNavigationParams
we pass there could be modified to have a non-null nav_entry_id.

[Avi (use Gerrit), 2015/04/13 22:42:48]

I was auditing the code for uses of page id, and adding the equivalent in the
new scheme. Since the old code DCHECKed on page id, I added the equivalent for
nav_entry_id. Would you prefer I drop it?

[clamy, 2015/04/14 15:44:47]

No I was suggesting having the DCHECK for all navigations, not just history
ones.

[Avi (use Gerrit), 2015/04/15 15:10:16]

There are lots of parameters that are required for proper functioning, though,
and it's weird to check just this one parameter of request_params.

In fact, I'm not understanding what the usefulness is of checking the page id
here and below. I know the historic origins (they were added by Darin in 2009 in
http://codereview.chromium.org/113758 as a sanity check and strengthened by
Charlie in 2011 in https://codereview.chromium.org/8817006) but either we should
do a complete sanity DCHECK of all parameters or not.

I'd actually prefer to remove them.

     scoped_ptr<HistoryEntry> entry =
         PageStateToHistoryEntry(request_params.page_state);
     if (entry) {
@@ -1164,6 +1168,10 @@ void RenderFrameImpl::OnNavigate(
 
     // A session history navigation should have been accompanied by state.
     CHECK_EQ(request_params.page_id, -1);
+    // FYIREMOVEME(avi): Page id being -1 will happen with browser-initiated new
+    // (non-history) navigations, and that's the test here. Nav entry unique
+    // ids, however, are always provided with browser-initiated navigations,
+    // history or new, so this test isn't valid for nav entry unique ids.
 
     // Record this before starting the load, we need a lower bound of this time
     // to sanitize the navigationStart override set below.
@@ -2497,8 +2505,6 @@ void RenderFrameImpl::didFailProvisionalLoad(
   WebDataSource* ds = frame->provisionalDataSource();
   DCHECK(ds);
 
-  const WebURLRequest& failed_request = ds->request();
-
   // Notify the browser that we failed a provisional load with an error.
   //
   // Note: It is important this notification occur before DidStopLoading so the
@@ -2510,12 +2516,19 @@ void RenderFrameImpl::didFailProvisionalLoad(
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_,
                     DidFailProvisionalLoad(error));
 
+  DocumentState* document_state = DocumentState::FromDataSource(ds);
+  NavigationStateImpl* navigation_state =
+      static_cast<NavigationStateImpl*>(document_state->navigation_state());
+
+  const WebURLRequest& failed_request = ds->request();
+
   bool show_repost_interstitial =
       (error.reason == net::ERR_CACHE_MISS &&
        EqualsASCII(failed_request.httpMethod(), "POST"));
 
   FrameHostMsg_DidFailProvisionalLoadWithError_Params params;
   params.error_code = error.reason;
+  params.nav_entry_id = navigation_state->request_params().nav_entry_id;
   GetContentClient()->renderer()->GetNavigationErrorStrings(
       render_view_.get(),
       frame,
@@ -2553,10 +2566,6 @@ void RenderFrameImpl::didFailProvisionalLoad(
   // Make sure we never show errors in view source mode.
   frame->enableViewSourceMode(false);
 
-  DocumentState* document_state = DocumentState::FromDataSource(ds);
-  NavigationStateImpl* navigation_state =
-      static_cast<NavigationStateImpl*>(document_state->navigation_state());
-
   // If this is a failed back/forward/reload navigation, then we need to do a
   // 'replace' load.  This is necessary to avoid messing up session history.
   // Otherwise, we do a normal load, which simulates a 'go' navigation as far
@@ -2618,6 +2627,7 @@ void RenderFrameImpl::didCommitProvisionalLoad(
   }
   internal_data->set_use_error_page(false);
 
+  bool successful_history_nav = 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.
@@ -2643,19 +2653,13 @@ void RenderFrameImpl::didCommitProvisionalLoad(
           render_view_->history_list_offset_ + 1;
     }
   } 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->request_params().page_id != -1 &&
-        navigation_state->request_params().page_id != render_view_->page_id_) {
+    // FYIREMOVEME(avi): This used to check if the page id matched because
+    // UpdateSessionHistory was called here, and we didn't want to call it twice
+    // on the same load. After r271220, when it was moved elsewhere, we don't
+    // have that worry.
+    if (navigation_state->request_params().page_id != -1) {
       // This is a successful session history navigation!
+      successful_history_nav = true;
       render_view_->page_id_ = navigation_state->request_params().page_id;
 
       render_view_->history_list_offset_ =
@@ -2663,6 +2667,13 @@ void RenderFrameImpl::didCommitProvisionalLoad(
     }
   }
 
+  if (commit_type == blink::WebBackForwardCommit) {
+    // Checking the commit type is planned to be the replacement for the if()

[Charlie Reis, 2015/04/10 23:54:21]

Would it be better to just have
DCHECK_EQ(successful_history_nav, commit_type == blink::WebBackForwardCommit) ?

The current if (A) DCHECK(B) wouldn't catch cases that were successful history
navs before but weren't WebBackForwardCommits.

[Avi (use Gerrit), 2015/04/13 22:42:49]

Good idea.

[Avi (use Gerrit), 2015/04/15 20:28:25]

And this breaks a lot. I need to dig pretty hard on this now.

+    // block containing the comment "This is a successful session history
+    // navigation!" Make sure that it fires correspondingly.
+    DCHECK(successful_history_nav);
+  }
+
   bool sent = Send(
       new FrameHostMsg_DidAssignPageId(routing_id_, render_view_->page_id_));
   CHECK(sent);  // http://crbug.com/407376
@@ -3809,8 +3820,10 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(
   params.http_status_code = response.httpStatusCode();
   params.url_is_unreachable = ds->hasUnreachableURL();
   params.is_post = false;
+  params.did_create_new_entry = commit_type == blink::WebStandardCommit;
   params.post_id = -1;
   params.page_id = render_view_->page_id_;
+  params.nav_entry_id = navigation_state->request_params().nav_entry_id;
   // We need to track the RenderViewHost routing_id because of downstream
   // dependencies (crbug.com/392171 DownloadRequestHandle, SaveFileManager,
   // ResourceDispatcherHostImpl, MediaStreamUIProxy,