Fix bugs with renderer-side debug URLs, like chrome://crash or javascript:.

These URLs should not cause a process swap when leaving pages like extensions
or view-source.  Also, they should be ignored if the renderer process is not
live.

BUG=335503
BUG=334214
TEST=chrome://kill works on extensions and is ignored on crashed tabs.

Review URL: https://codereview.chromium.org/151593004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@248442 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 97 insertions, 18 deletions

diff --git a/content/browser/frame_host/debug_urls.cc b/content/browser/frame_host/debug_urls.cc
index 3322e51..4e9d49d 100644
--- a/content/browser/frame_host/debug_urls.cc
+++ b/content/browser/frame_host/debug_urls.cc
@@ -80,4 +80,17 @@ bool HandleDebugURL(const GURL& url, PageTransition transition) {
   return false;
 }
 
+bool IsRendererDebugURL(const GURL& url) {
+  if (!url.is_valid())
+    return false;
+
+  if (url.SchemeIs(kJavaScriptScheme))
+    return true;
+
+  return url == GURL(kChromeUICrashURL) ||
+         url == GURL(kChromeUIKillURL) ||
+         url == GURL(kChromeUIHangURL) ||
+         url == GURL(kChromeUIShorthangURL);
+}
+
 }  // namespace content
diff --git a/content/browser/frame_host/debug_urls.h b/content/browser/frame_host/debug_urls.h
index 1985253..01530b4 100644
--- a/content/browser/frame_host/debug_urls.h
+++ b/content/browser/frame_host/debug_urls.h
@@ -15,6 +15,12 @@ namespace content {
 // handles it and returns true.
 bool HandleDebugURL(const GURL& url, PageTransition transition);
 
+// Returns whether the given url is either a debugging url handled in the
+// renderer process, such as one that crashes or hangs the renderer, or a
+// javascript: URL that operates on the current page in the renderer.  Such URLs
+// do not represent actual navigations and can be loaded in any SiteInstance.
+bool IsRendererDebugURL(const GURL& url);
+
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_FRAME_HOST_DEBUG_URLS_H_
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index c2ae947..b4f4cf1 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -642,6 +642,14 @@ void NavigationControllerImpl::LoadURLWithParams(const LoadURLParams& params) {
   if (HandleDebugURL(params.url, params.transition_type))
     return;
 
+  // Any renderer-side debug URLs or javascript: URLs should be ignored if the
+  // renderer process is not live.
+  if (IsRendererDebugURL(params.url)) {
+    // TODO(creis): Find the RVH for the correct frame.
+    if (!delegate_->GetRenderViewHost()->IsRenderViewLive())
+      return;
+  }
+
   // Checks based on params.load_type.
   switch (params.load_type) {
     case LOAD_TYPE_DEFAULT:
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 5738861..ade1c72 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -12,6 +12,7 @@
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/devtools/render_view_devtools_agent_host.h"
 #include "content/browser/frame_host/cross_process_frame_connector.h"
+#include "content/browser/frame_host/debug_urls.h"
 #include "content/browser/frame_host/interstitial_page_impl.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
@@ -602,6 +603,11 @@ bool RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
   const GURL& new_url = SiteInstanceImpl::GetEffectiveURL(browser_context,
                                                           new_entry->GetURL());
 
+  // Don't force a new BrowsingInstance for debug URLs that are handled in the
+  // renderer process, like javascript: or chrome://crash.
+  if (IsRendererDebugURL(new_url))
+    return false;
+
   // For security, we should transition between processes when one is a Web UI
   // page and one isn't.
   if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
@@ -781,10 +787,13 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForEntry(
       current_instance->GetSiteURL();
 
   // View-source URLs must use a new SiteInstance and BrowsingInstance.
+  // We don't need a swap when going from view-source to a debug URL like
+  // chrome://crash, however.
   // TODO(creis): Refactor this method so this duplicated code isn't needed.
   // See http://crbug.com/123007.
   if (current_entry &&
-      current_entry->IsViewSourceMode() != entry.IsViewSourceMode()) {
+      current_entry->IsViewSourceMode() != entry.IsViewSourceMode() &&
+      !IsRendererDebugURL(dest_url)) {
     return SiteInstance::CreateForURL(browser_context, dest_url);
   }
 
diff --git a/content/browser/frame_host/render_frame_host_manager_browsertest.cc b/content/browser/frame_host/render_frame_host_manager_browsertest.cc
index 23fec72..857cac8 100644
--- a/content/browser/frame_host/render_frame_host_manager_browsertest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_browsertest.cc
@@ -9,6 +9,7 @@
 #include "base/path_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
@@ -1355,4 +1356,57 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
   EXPECT_EQ(new_site_instance, shell()->web_contents()->GetSiteInstance());
 }
 
+// Ensure that renderer-side debug URLs do not cause a process swap, since they
+// are meant to run in the current page.  We had a bug where we expected a
+// BrowsingInstance swap to occur on pages like view-source and extensions,
+// which broke chrome://crash and javascript: URLs.
+// See http://crbug.com/335503.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, RendererDebugURLsDontSwap) {
+  ASSERT_TRUE(test_server()->Start());
+
+  GURL original_url(test_server()->GetURL("files/title2.html"));
+  GURL view_source_url(kViewSourceScheme + std::string(":") +
+                       original_url.spec());
+
+  NavigateToURL(shell(), view_source_url);
+
+  // Check that javascript: URLs work.
+  base::string16 expected_title = ASCIIToUTF16("msg");
+  TitleWatcher title_watcher(shell()->web_contents(), expected_title);
+  shell()->LoadURL(GURL("javascript:document.title='msg'"));
+  ASSERT_EQ(expected_title, title_watcher.WaitAndGetTitle());
+
+  // Crash the renderer of the view-source page.
+  RenderProcessHostWatcher crash_observer(
+      shell()->web_contents(),
+      RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+  NavigateToURL(shell(), GURL(kChromeUICrashURL));
+  crash_observer.Wait();
+}
+
+// Ensure that renderer-side debug URLs don't take effect on crashed renderers.
+// Otherwise, we might try to load an unprivileged about:blank page into a
+// WebUI-enabled RenderProcessHost, failing a safety check in InitRenderView.
+// See http://crbug.com/334214.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
+                       IgnoreRendererDebugURLsWhenCrashed) {
+  // Visit a WebUI page with bindings.
+  GURL webui_url = GURL(std::string(chrome::kChromeUIScheme) + "://" +
+                        std::string(kChromeUIGpuHost));
+  NavigateToURL(shell(), webui_url);
+  EXPECT_TRUE(ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
+                  shell()->web_contents()->GetRenderProcessHost()->GetID()));
+
+  // Crash the renderer of the WebUI page.
+  RenderProcessHostWatcher crash_observer(
+      shell()->web_contents(),
+      RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+  NavigateToURL(shell(), GURL(kChromeUICrashURL));
+  crash_observer.Wait();
+
+  // Load the crash URL again but don't wait for any action.  If it is not
+  // ignored this time, we will fail the WebUI CHECK in InitRenderView.
+  shell()->LoadURL(GURL(kChromeUICrashURL));
+}
+
 }  // namespace content
diff --git a/content/browser/loader/resource_dispatcher_host_browsertest.cc b/content/browser/loader/resource_dispatcher_host_browsertest.cc
index a2af83b..239da69 100644
--- a/content/browser/loader/resource_dispatcher_host_browsertest.cc
+++ b/content/browser/loader/resource_dispatcher_host_browsertest.cc
@@ -301,6 +301,9 @@ IN_PROC_BROWSER_TEST_F(ResourceDispatcherHostBrowserTest,
 // complete and isn't conducive to quick turnarounds. As we don't currently
 // strip the app on the build bots, this is bad times.
 IN_PROC_BROWSER_TEST_F(ResourceDispatcherHostBrowserTest, CrossSiteAfterCrash) {
+  // Make sure we have a live process before trying to kill it.
+  NavigateToURL(shell(), GURL("about:blank"));
+
   // Cause the renderer to crash.
   RenderProcessHostWatcher crash_observer(
       shell()->web_contents(),
diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc
index cf093d8..e5a275c 100644
--- a/content/browser/site_instance_impl.cc
+++ b/content/browser/site_instance_impl.cc
@@ -7,6 +7,7 @@
 #include "base/command_line.h"
 #include "content/browser/browsing_instance.h"
 #include "content/browser/child_process_security_policy_impl.h"
+#include "content/browser/frame_host/debug_urls.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/public/browser/content_browser_client.h"
@@ -18,21 +19,6 @@
 
 namespace content {
 
-static bool IsURLSameAsAnySiteInstance(const GURL& url) {
-  if (!url.is_valid())
-    return false;
-
-  // We treat javascript: as the same site as any URL since it is actually
-  // a modifier on existing pages.
-  if (url.SchemeIs(kJavaScriptScheme))
-    return true;
-
-  return url == GURL(kChromeUICrashURL) ||
-         url == GURL(kChromeUIKillURL) ||
-         url == GURL(kChromeUIHangURL) ||
-         url == GURL(kChromeUIShorthangURL);
-}
-
 const RenderProcessHostFactory*
     SiteInstanceImpl::g_render_process_host_factory_ = NULL;
 int32 SiteInstanceImpl::next_site_instance_id_ = 1;
@@ -210,7 +196,7 @@ bool SiteInstanceImpl::HasWrongProcessForURL(const GURL& url) {
 
   // If the URL to navigate to can be associated with any site instance,
   // we want to keep it in the same process.
-  if (IsURLSameAsAnySiteInstance(url))
+  if (IsRendererDebugURL(url))
     return false;
 
   // If the site URL is an extension (e.g., for hosted apps or WebUI) but the
@@ -259,7 +245,7 @@ bool SiteInstance::IsSameWebSite(BrowserContext* browser_context,
   // Some special URLs will match the site instance of any other URL. This is
   // done before checking both of them for validity, since we want these URLs
   // to have the same site instance as even an invalid one.
-  if (IsURLSameAsAnySiteInstance(url1) || IsURLSameAsAnySiteInstance(url2))
+  if (IsRendererDebugURL(url1) || IsRendererDebugURL(url2))
     return true;
 
   // If either URL is invalid, they aren't part of the same site.