Solidify Entry discarding logic (NavigationHandle keeps its ID)

content/browser/frame_host/navigator_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/browser/frame_host/navigator_impl.h"
 
 #include <utility>
 
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
@@ skipping 134 matching lines @@
       return;
     }
 
     // This ensures that notifications about the end of the previous
     // navigation are sent before notifications about the start of the
     // new navigation.
     render_frame_host->SetNavigationHandle(scoped_ptr<NavigationHandleImpl>());
   }
 
   render_frame_host->SetNavigationHandle(NavigationHandleImpl::Create(
-      validated_url, render_frame_host->frame_tree_node(), navigation_start));
+      validated_url, render_frame_host->frame_tree_node(), navigation_start,
+      controller_->GetPendingEntry()));
 }
 
 void NavigatorImpl::DidFailProvisionalLoadWithError(
     RenderFrameHostImpl* render_frame_host,
     const FrameHostMsg_DidFailProvisionalLoadWithError_Params& params) {
   VLOG(1) << "Failed Provisional Load: " << params.url.possibly_invalid_spec()
           << ", error_code: " << params.error_code
           << ", error_description: " << params.error_description
           << ", showing_repost_interstitial: " <<
             params.showing_repost_interstitial
@@ skipping 41 matching lines @@
   //
   // However, we do preserve the pending entry in some cases, such as on the
   // initial navigation of an unmodified blank tab.  We also allow the delegate
   // to say when it's safe to leave aborted URLs in the omnibox, to let the user
   // edit the URL and try again.  This may be useful in cases that the committed
   // page cannot be attacker-controlled.  In these cases, we still allow the
   // view to clear the pending entry and typed URL if the user requests
   // (e.g., hitting Escape with focus in the address bar).
   //
   // Note: don't touch the transient entry, since an interstitial may exist.
+  NavigationHandleImpl* handle = render_frame_host->navigation_handle();
+  NavigationEntry* pending = controller_->GetPendingEntry();

[Charlie Reis, 2016/02/05 19:36:12]

nit: pending_entry

[Charlie Harrison, 2016/02/05 23:10:24]

Done.

+  bool pending_matches_fail =

[Charlie Reis, 2016/02/05 19:36:12]

nit: pending_entry_matches_fail_msg

[Charlie Harrison, 2016/02/05 23:10:23]

Done.

+      handle && pending && handle->get_entry_id() == pending->GetUniqueID();
   bool should_preserve_entry = controller_->IsUnmodifiedBlankTab() ||
       delegate_->ShouldPreserveAbortedURLs();
-  if (controller_->GetPendingEntry() != controller_->GetVisibleEntry() ||
-      !should_preserve_entry) {
+  if ((pending != controller_->GetVisibleEntry() || !should_preserve_entry) &&
+      pending_matches_fail) {

[Charlie Reis, 2016/02/05 19:36:12]

This is really hard to follow.  (It was already really hard to follow, and now I
think it's past the threshold that I can reason about it.)  :)

I think we're basically saying: do what we did before, but make sure we only
discard the pending entry here if the DidFail message matches the pending entry.

Maybe this would be more legible like:

  // Big comment about why we only care if the pending entry matches the IPC
  NavHandle handle...
  if (pending_entry_matches_fail_msg) {
    // We usually...
    bool should_preserve...
    if (pending_entry != ...) {
      Discard + Notify
    }
  }

[Charlie Harrison, 2016/02/05 23:10:23]

SGTM. My projections show that by next year, this method will read as a tidy
novella!

     controller_->DiscardPendingEntry(true);
 
     // Also force the UI to refresh.
     controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
   }
 
   if (delegate_)
     delegate_->DidFailProvisionalLoadWithError(render_frame_host, params);
 }
 
@@ skipping 535 matching lines @@
   }
 
   // In all other cases the current navigation, if any, is canceled and a new
   // NavigationRequest is created for the node.
   frame_tree_node->CreatedNavigationRequest(
       NavigationRequest::CreateRendererInitiated(
           frame_tree_node, common_params, begin_params, body,
           controller_->GetLastCommittedEntryIndex(),
           controller_->GetEntryCount()));
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
-  navigation_request->CreateNavigationHandle();
-
   if (frame_tree_node->IsMainFrame()) {
     // Renderer-initiated main-frame navigations that need to swap processes
     // will go to the browser via a OpenURL call, and then be handled by the
     // same code path as browser-initiated navigations. For renderer-initiated
     // main frame navigation that start via a BeginNavigation IPC, the
     // RenderFrameHost will not be swapped. Therefore it is safe to call
     // DidStartMainFrameNavigation with the SiteInstance from the current
     // RenderFrameHost.
     DidStartMainFrameNavigation(
         common_params.url,
         frame_tree_node->current_frame_host()->GetSiteInstance());
     navigation_data_.reset();
   }
 
+  navigation_request->CreateNavigationHandle(controller_->GetPendingEntry());

[Charlie Reis, 2016/02/05 19:36:12]

nit: Comment about why this must be after DidStartMainFrameNavigation, and no
blank line after.

[Charlie Harrison, 2016/02/05 23:10:23]

Done.

+
   navigation_request->BeginNavigation();
 }
 
 // PlzNavigate
 void NavigatorImpl::CommitNavigation(FrameTreeNode* frame_tree_node,
                                      ResourceResponse* response,
                                      scoped_ptr<StreamHandle> body) {
   CHECK(IsBrowserSideNavigationEnabled());
 
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
@@ skipping 132 matching lines @@
   bool should_dispatch_beforeunload =
       frame_tree_node->current_frame_host()->ShouldDispatchBeforeUnload();
   FrameMsg_Navigate_Type::Value navigation_type =
       GetNavigationType(controller_->GetBrowserContext(), entry, reload_type);
   frame_tree_node->CreatedNavigationRequest(
       NavigationRequest::CreateBrowserInitiated(
           frame_tree_node, dest_url, dest_referrer, frame_entry, entry,
           navigation_type, is_same_document_history_load, navigation_start,
           controller_));
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
-  navigation_request->CreateNavigationHandle();
+  navigation_request->CreateNavigationHandle(controller_->GetVisibleEntry());

[Charlie Reis, 2016/02/05 19:36:12]

Shouldn't this be GetPendingEntry()?  GetVisibleEntry() can return the last
committed entry in situations that we don't think it's safe to display the
pending one (e.g., renderer-initiated navigations).

[Charlie Harrison, 2016/02/05 23:10:23]

Yes. Good catch. In fact, here we can use the NavigationEntryImpl that was
passed into the method.

 
   // Have the current renderer execute its beforeunload event if needed. If it
   // is not needed (when beforeunload dispatch is not needed or this navigation
   // is synchronous and same-site) then NavigationRequest::BeginNavigation
   // should be directly called instead.
   if (should_dispatch_beforeunload &&
       ShouldMakeNetworkRequestForURL(
           navigation_request->common_params().url)) {
     navigation_request->SetWaitingForRendererResponse();
     frame_tree_node->current_frame_host()->DispatchBeforeUnload(true);
@@ skipping 83 matching lines @@
       entry->set_should_replace_entry(pending_entry->should_replace_entry());
       entry->SetRedirectChain(pending_entry->GetRedirectChain());
     }
     controller_->SetPendingEntry(std::move(entry));
     if (delegate_)
       delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
   }
 }
 
 }  // namespace content