Change NavigationEntry to use virtual URL in error pages for blocked navigations

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 1e0e55b71fc212027d87e08368f5815c83eb9acf..9352302e6d32cac26ae991693029441b11bfc513 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -162,6 +162,26 @@ bool ShouldTreatNavigationAsReload(const NavigationEntry* entry) {
   return false;
 }
 
+bool IsBlockedNavigation(net::Error error_code) {
+  switch (error_code) {

[Charlie Reis, 2017/05/18 22:41:46]

I'm nervous about hand picking these out of net_error_list.h.  Feels easy to
miss things (especially if error codes get added later, like
ERR_BLOCKED_BY_XSS_AUDITOR) or go too far (as we found with ERR_INVALID_URL
causing test failures).

Can we get someone from the network team to offer a suggestion?  (Is blocked vs
failed even a well-defined distinction between errors?  I hope so.)

[nasko, 2017/05/18 22:55:11]

I don't think that such a distinction exists actually, but definitely will ping
someone from the network team to discuss it with. The goal of this was to start
with something and drive discussion forward :).

+    case net::ERR_ACCESS_DENIED:
+    case net::ERR_NOT_IMPLEMENTED:
+    case net::ERR_BLOCKED_BY_CLIENT:
+    case net::ERR_BLOCKED_BY_ADMINISTRATOR:
+    case net::ERR_BLOCKED_BY_RESPONSE:
+    case net::ERR_BLOCKED_BY_XSS_AUDITOR:
+    case net::ERR_CLEARTEXT_NOT_PERMITTED:
+    case net::ERR_NETWORK_ACCESS_DENIED:  // Does this mean blocked?

[Charlie Reis, 2017/05/18 22:41:46]

I would probably remove this.  Network access denied doesn't sound like the URL
was the problem.  If network access was later granted, I would expect these URLs
to keep working.

In general, we should only be doing this change for cases that are definitely
dangerous.  Otherwise it's a functional regression if some temporary error
prevents you from getting back to the pages you had open before.

[nasko, 2017/05/18 22:55:11]

My goal was to pick the errors that aren't transient, but will always result in
the same error. This specific error was on the fence, hence the comment.
Removed.

+    case net::ERR_INVALID_URL:

[Charlie Reis, 2017/05/18 22:41:46]

Should we remove this to get the test to pass, or investigate why the test is
hitting it?

[nasko, 2017/05/18 22:55:11]

I don't think the test is doing what it is expecting to do. Let's discuss what
the goals of the test are and we can decide then. I think the failure while
related to this change, might be a bit unrelated.

[Charlie Reis, 2017/05/18 23:15:51]

Agreed-- let's look more closely at it tomorrow.

+    case net::ERR_DISALLOWED_URL_SCHEME:
+    case net::ERR_UNSAFE_REDIRECT:
+    case net::ERR_UNSAFE_PORT:
+      return true;
+    default:
+      return false;
+  }
+}
+
 }  // namespace
 
 // NavigationControllerImpl ----------------------------------------------------
@@ -933,6 +953,22 @@ bool NavigationControllerImpl::RendererDidNavigate(
     frame_entry->set_redirect_chain(params.redirects);
   }
 
+  // When a navigation in the main frame is blocked, it will display an error
+  // page. To avoid committing the blocked URL in back/forward/reload
+  // navigations, ensure the underlying URL is safe to load and preserve the
+  // UX by setting the blocked URL as the virtual URL on |active_entry|. To
+  // ensure the renderer process doesn't also navigate there, overwrite the
+  // PageState with a new one.

[Charlie Reis, 2017/05/18 22:41:46]

Sounds good.  Slight rephrase:

... To avoid loading the blocked URL on back/forward/reload navigations, do not
store it in the FrameNavigationEntry's URL or PageState. Instead, make it
visible to the user as the virtual URL. Store a safe URL (about:blank) as the
one to load if the entry is revisited.

[nasko, 2017/05/18 22:55:11]

Done.

+  if (!rfh->GetParent() &&
+      IsBlockedNavigation(navigation_handle->GetNetErrorCode())) {
+    DCHECK(params.url_is_unreachable);

[Charlie Reis, 2017/05/18 22:41:46]

Heh, I have some mixed feelings here.

We generally don't want a renderer IPC to crash the browser, but in a debug
build it's probably reasonable to catch bugs this way.

I'm mainly curious if this is going to get hit in practice-- hopefully not, but
I'm not sure how close the code for these lines up.

Let's keep it and see what happens.

[nasko, 2017/05/18 22:55:11]

I wanted it mainly for discovering potential issues in test coverage that we
have. I'm ok removing it, but we also should not be shipping DCHECKs to users
:).

+    active_entry->SetURL(GURL(url::kAboutBlankURL));
+    active_entry->SetVirtualURL(params.url);
+    if (frame_entry)

[Charlie Reis, 2017/05/18 22:41:46]

nit: Needs braces.

[nasko, 2017/05/18 22:55:11]

Done.

+      frame_entry->SetPageState(
+          PageState::CreateFromURL(active_entry->GetURL()));
+  }
+
   // Use histogram to track memory impact of redirect chain because it's now
   // not cleared for committed entries.
   size_t redirect_chain_size = 0;