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

content/browser/frame_host/navigation_controller_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.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 144 matching lines @@
     return true;
   }
 
   if (ui::PageTransitionCoreTypeIs(entry->GetTransitionType(),
                                    ui::PAGE_TRANSITION_LINK)) {
     return true;
   }
   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 ----------------------------------------------------
 
 const size_t kMaxEntryCountForTestingNotSet = static_cast<size_t>(-1);
 
 // static
 size_t NavigationControllerImpl::max_entry_count_for_testing_ =
     kMaxEntryCountForTestingNotSet;
 
@@ skipping 703 matching lines @@
     case NAVIGATION_TYPE_NEW_SUBFRAME:
       RendererDidNavigateNewSubframe(rfh, params, details->is_same_document,
                                      details->did_replace_entry);
       break;
     case NAVIGATION_TYPE_AUTO_SUBFRAME:
       if (!RendererDidNavigateAutoSubframe(rfh, params)) {
         // We don't send a notification about auto-subframe PageState during
         // UpdateStateForFrame, since it looks like nothing has changed.  Send
         // it here at commit time instead.
         NotifyEntryChanged(GetLastCommittedEntry());
         return false;

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

If we later extend this fix to subframes, we'll need to be careful about this
case, which happens on the first navigation of a subframe and doesn't reach the
code below.

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

Noted! Should we add a TODO/Note?

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

Yes, maybe down on the comment you're adding for the block below, about doing
something similar for subframe navigations (including AUTO_SUBFRAME).

[nasko, 2017/05/19 18:30:52]

Done.

       }
       break;
     case NAVIGATION_TYPE_NAV_IGNORE:
       // If a pending navigation was in progress, this canceled it.  We should
       // discard it and make sure it is removed from the URL bar.  After that,
       // there is nothing we can do with this navigation, so we just return to
       // the caller that nothing has happened.
       if (pending_entry_) {
         DiscardNonCommittedEntries();
         delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
@@ skipping 27 matching lines @@
   FrameNavigationEntry* frame_entry =
       active_entry->GetFrameEntry(rfh->frame_tree_node());
   // Update the frame-specific PageState and RedirectChain
   // We may not find a frame_entry in some cases; ignore the PageState if so.
   // TODO(creis): Remove the "if" once https://crbug.com/522193 is fixed.
   if (frame_entry) {
     frame_entry->SetPageState(params.page_state);
     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;
   for (size_t i = 0; i < params.redirects.size(); ++i) {
     redirect_chain_size += params.redirects[i].spec().length();
   }
   UMA_HISTOGRAM_COUNTS("Navigation.RedirectChainSize", redirect_chain_size);
 
   // Once it is committed, we no longer need to track several pieces of state on
   // the entry.
@@ skipping 1247 matching lines @@
   DCHECK(pending_entry_index_ == -1 ||
          pending_entry_ == GetEntryAtIndex(pending_entry_index_));
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content