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

chrome/browser/extensions/navigation_observer.cc

 // Copyright (c) 2012 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 "chrome/browser/extensions/navigation_observer.h"
 
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/navigation_controller.h"
@@ skipping 51 matching lines @@
 void NavigationObserver::PromptToEnableExtensionIfNecessary(
     NavigationController* nav_controller) {
   // Bail out if we're already running a prompt.
   if (!in_progress_prompt_extension_id_.empty())
     return;
 
   NavigationEntry* nav_entry = nav_controller->GetVisibleEntry();
   if (!nav_entry)
     return;
 
-  const GURL& url = nav_entry->GetURL();
+  // With a disabled extension, the navigation is blocked, which results in
+  // a NavigationEntry with "about:blank" URL. In this case, the virtual
+  // URL is the real URL the user navigated to, so if it is non-empty, use
+  // it instead.
+  const GURL& url = nav_entry->GetVirtualURL().is_empty()

[Charlie Reis, 2017/05/24 21:50:52]

This condition makes me nervous.  If GetURL is an extension URL and
GetVirtualURL is not, then we're potentially breaking the code below.  I know
most component extensions won't be disabled, but I'm not 100% sure that no
extension that can be disabled has a non-extension virtual URL.

Maybe we should only use the virtual URL (instead of GetURL) if the virtual URL
is an extension and GetURL() is about:blank, to limit to this case?

And/or your idea of checking if nav_entry has an error in GetPageType?

[nasko, 2017/05/24 22:16:41]

Fixed by combining all 3 checks.

+                        ? nav_entry->GetURL()
+                        : nav_entry->GetVirtualURL();
+
   // NOTE: We only consider chrome-extension:// urls, and deliberately don't
   // consider hosted app urls. This is because it's really annoying to visit the
   // site associated with a hosted app (like calendar.google.com or
   // drive.google.com) and have it repeatedly prompt you to re-enable an item.
   // Visiting a chrome-extension:// url is a much stronger signal, and, without
   // the item enabled, we won't show anything.
   // TODO(devlin): While true, I still wonder how useful this is. We should get
   // metrics.
   if (!url.SchemeIs(kExtensionScheme))
     return;
@@ skipping 79 matching lines @@
       in_progress_prompt_extension_id_ != extension->id()) {
     return;
   }
 
   in_progress_prompt_extension_id_ = std::string();
   in_progress_prompt_navigation_controller_ = nullptr;
   extension_install_prompt_.reset();
 }
 
 }  // namespace extensions