Prevent non-error navigations from committing in the error page process.

This CL is fixing a case where a successful navigation to a regular web URL
can commit in the dedicated error page process. It is one of the two ways
found to cause renderer process termination described in
https://crbug.com/866549.

Bug: 866549
Change-Id: If7a49c47feef35b954c67e7f6bd2339641473bf7
Reviewed-on: https://chromium-review.googlesource.com/1165593
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583708}

chrome/browser/translate/translate_manager_render_view_host_unittest.cc

@@ -69,6 +69,7 @@
 #include "services/network/test/test_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/blink/public/web/web_context_menu_data.h"
+#include "ui/base/page_transition_types.h"
 #include "url/gurl.h"
 
 namespace {
@@ -423,8 +424,9 @@ class TranslateManagerRenderViewHostTest
     // Ensures it is really handled a reload.
     const content::LoadCommittedDetails& nav_details =
         nav_observer.load_committed_details();
-    EXPECT_TRUE(nav_details.entry != NULL);  // There was a navigation.
-    EXPECT_EQ(content::NAVIGATION_TYPE_EXISTING_PAGE, nav_details.type);
+    EXPECT_TRUE(nav_details.entry);  // There was a navigation.
+    EXPECT_TRUE(ui::PageTransitionCoreTypeIs(
+        ui::PAGE_TRANSITION_RELOAD, nav_details.entry->GetTransitionType()));
 
     // The TranslateManager class processes the navigation entry committed
     // notification in a posted task; process that task.

content/browser/frame_host/navigation_controller_impl.cc

@@ -1022,6 +1022,10 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
       return NAVIGATION_TYPE_NAV_IGNORE;
 
     // This is history.replaceState() or history.reload().
+    // TODO(nasko): With error page isolation, reloading an existing session
+    // history entry can result in change of SiteInstance. Check for such a case
+    // here and classify it as NEW_PAGE, as such navigations should be treated
+    // as new with replacement.
     return NAVIGATION_TYPE_EXISTING_PAGE;
   }
 

content/browser/frame_host/render_frame_host_manager.cc

@@ -1138,22 +1138,35 @@ RenderFrameHostManager::DetermineSiteInstanceForURL(
   // If the entry has an instance already we should use it, unless it is no
   // longer suitable.
   if (dest_instance) {
-    SiteInstanceImpl* dest_instance_impl =
-        static_cast<SiteInstanceImpl*>(dest_instance);
-    // TODO(nasko,creis): The check whether data: or about: URLs are allowed
-    // to commit in the current process should be in HasWrongProcessForURL.
-    // However, making this change has further implications and needs more
-    // investigation of what behavior changes. For now, use a conservative
-    // approach and explicitly check before calling HasWrongProcessForURL.
-    if (IsDataOrAbout(dest_url) ||
-        !dest_instance_impl->HasWrongProcessForURL(dest_url)) {
-      // If we are forcing a swap, this should be in a different
-      // BrowsingInstance.
-      if (force_browsing_instance_swap) {
-        CHECK(!dest_instance->IsRelatedSiteInstance(
-            render_frame_host_->GetSiteInstance()));
+    // When error page isolation is enabled, don't reuse |dest_instance| if it's
+    // an error page SiteInstance, but the navigation will no longer fail.
+    // Similarly, don't reuse |dest_instance| if it's not an error page
+    // SiteInstance but the navigation will fail and actually need an error page
+    // SiteInstance.
+    // Note: The later call to HasWrongProcessForURL does not have context about
+    // error page navigaions, so we cannot rely on it to return correct value
+    // when error pages are involved.
+    if (!SiteIsolationPolicy::IsErrorPageIsolationEnabled(
+            frame_tree_node_->IsMainFrame()) ||
+        ((dest_instance->GetSiteURL() == GURL(kUnreachableWebDataURL)) ==
+         is_failure)) {
+      // TODO(nasko,creis): The check whether data: or about: URLs are allowed
+      // to commit in the current process should be in HasWrongProcessForURL.
+      // However, making this change has further implications and needs more
+      // investigation of what behavior changes. For now, use a conservative
+      // approach and explicitly check before calling HasWrongProcessForURL.
+      SiteInstanceImpl* dest_instance_impl =
+          static_cast<SiteInstanceImpl*>(dest_instance);
+      if (IsDataOrAbout(dest_url) ||
+          !dest_instance_impl->HasWrongProcessForURL(dest_url)) {
+        // If we are forcing a swap, this should be in a different
+        // BrowsingInstance.
+        if (force_browsing_instance_swap) {
+          CHECK(!dest_instance->IsRelatedSiteInstance(
+              render_frame_host_->GetSiteInstance()));
+        }
+        return SiteInstanceDescriptor(dest_instance);
       }
-      return SiteInstanceDescriptor(dest_instance);
     }
   }
 

content/browser/frame_host/render_frame_host_manager_browsertest.cc

@@ -4312,45 +4312,87 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, ErrorPageNavigationReload) {
     return;
 
   StartEmbeddedServer();
-  GURL url(embedded_test_server()->GetURL("/title1.html"));
+  GURL start_url(embedded_test_server()->GetURL("/title1.html"));
   GURL error_url(embedded_test_server()->GetURL("/empty.html"));
-  std::unique_ptr<URLLoaderInterceptor> url_interceptor =
-      SetupRequestFailForURL(error_url);
+  GURL end_url(embedded_test_server()->GetURL("/title2.html"));
   NavigationControllerImpl& nav_controller =
       static_cast<NavigationControllerImpl&>(
           shell()->web_contents()->GetController());
   auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
 
-  // Start with a successful navigation to a document and verify there is
-  // only one entry in session history.
-  EXPECT_TRUE(NavigateToURL(shell(), url));
+  // Build session history with three entries, where the middle one will be
+  // tested for successful and failed reloads. This allows checking whether
+  // reload accidentally clears the forward session history if it is
+  // incorrectly classified.
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+  EXPECT_TRUE(NavigateToURL(shell(), error_url));
+  EXPECT_TRUE(NavigateToURL(shell(), end_url));
+  {
+    TestNavigationObserver back_observer(shell()->web_contents());
+    shell()->web_contents()->GetController().GoBack();
+    back_observer.Wait();
+    EXPECT_TRUE(back_observer.last_navigation_succeeded());
+  }
+  EXPECT_EQ(3, nav_controller.GetEntryCount());
+  EXPECT_EQ(1, nav_controller.GetLastCommittedEntryIndex());
+  EXPECT_EQ(error_url, shell()->web_contents()->GetLastCommittedURL());
+
   scoped_refptr<SiteInstance> success_site_instance =
       shell()->web_contents()->GetMainFrame()->GetSiteInstance();
-  EXPECT_EQ(1, nav_controller.GetEntryCount());
 
-  // Navigate to an url resulting in an error page and ensure a new entry
-  // was added to session history.
-  EXPECT_FALSE(NavigateToURL(shell(), error_url));
-  EXPECT_EQ(2, nav_controller.GetEntryCount());
+  // Install an interceptor which will cause network failure for |error_url|,
+  // reload the existing entry and verify.
+  std::unique_ptr<URLLoaderInterceptor> url_interceptor =
+      SetupRequestFailForURL(error_url);
+  {
+    TestNavigationObserver reload_observer(shell()->web_contents());
+    shell()->web_contents()->GetController().Reload(ReloadType::NORMAL, false);
+    reload_observer.Wait();
+    EXPECT_FALSE(reload_observer.last_navigation_succeeded());
+    // TODO(nasko): Investigate making a failing reload of a successful
+    // navigation be classified as NEW_PAGE instead, since with error page
+    // isolation it involves a SiteInstance swap.
+    EXPECT_EQ(NavigationType::NAVIGATION_TYPE_EXISTING_PAGE,
+              reload_observer.last_navigation_type());
+  }
+  EXPECT_EQ(3, nav_controller.GetEntryCount());
+  EXPECT_EQ(1, nav_controller.GetLastCommittedEntryIndex());
   EXPECT_EQ(
       GURL(kUnreachableWebDataURL),
       shell()->web_contents()->GetMainFrame()->GetSiteInstance()->GetSiteURL());
-  EXPECT_EQ(
-      GURL(kUnreachableWebDataURL),
-      policy->GetOriginLock(
-          shell()->web_contents()->GetSiteInstance()->GetProcess()->GetID()));
+  int process_id =
+      shell()->web_contents()->GetMainFrame()->GetProcess()->GetID();
+  EXPECT_EQ(GURL(kUnreachableWebDataURL), policy->GetOriginLock(process_id));
+
+  // Reload while it will still fail to ensure it stays in the same process.
+  {
+    TestNavigationObserver reload_observer(shell()->web_contents());
+    shell()->web_contents()->GetController().Reload(ReloadType::NORMAL, false);
+    reload_observer.Wait();
+    EXPECT_FALSE(reload_observer.last_navigation_succeeded());
+    EXPECT_EQ(NavigationType::NAVIGATION_TYPE_EXISTING_PAGE,
+              reload_observer.last_navigation_type());
+  }
+  EXPECT_EQ(process_id,
+            shell()->web_contents()->GetMainFrame()->GetProcess()->GetID());
 
   // Reload the error page after clearing the error condition, such that the
   // navigation is successful and verify that no new entry was added to
-  // session history.
+  // session history and forward history is not pruned.
   url_interceptor.reset();
   {
     TestNavigationObserver reload_observer(shell()->web_contents());
     shell()->web_contents()->GetController().Reload(ReloadType::NORMAL, false);
     reload_observer.Wait();
     EXPECT_TRUE(reload_observer.last_navigation_succeeded());
+    // The successful reload should be classified as a NEW_PAGE navigation
+    // with replacement, since it needs to stay at the same entry in session
+    // history, but needs a new entry because of the change in SiteInstance.
+    EXPECT_EQ(NavigationType::NAVIGATION_TYPE_NEW_PAGE,
+              reload_observer.last_navigation_type());
   }
-  EXPECT_EQ(2, nav_controller.GetEntryCount());
+  EXPECT_EQ(3, nav_controller.GetEntryCount());
+  EXPECT_EQ(1, nav_controller.GetLastCommittedEntryIndex());
   EXPECT_NE(
       GURL(kUnreachableWebDataURL),
       shell()->web_contents()->GetMainFrame()->GetSiteInstance()->GetSiteURL());
@@ -4362,8 +4404,19 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, ErrorPageNavigationReload) {
   // Test the same scenario as above, but this time initiated by the
   // renderer process.
   url_interceptor = SetupRequestFailForURL(error_url);
-  EXPECT_FALSE(NavigateToURL(shell(), error_url));
-  EXPECT_EQ(2, nav_controller.GetEntryCount());
+  {
+    TestNavigationObserver reload_observer(shell()->web_contents());
+    shell()->web_contents()->GetController().Reload(ReloadType::NORMAL, false);
+    reload_observer.Wait();
+    EXPECT_FALSE(reload_observer.last_navigation_succeeded());
+    // TODO(nasko): Investigate making a failing reload of a successful
+    // navigation be classified as NEW_PAGE instead, since with error page
+    // isolation it involves a SiteInstance swap.
+    EXPECT_EQ(NavigationType::NAVIGATION_TYPE_EXISTING_PAGE,
+              reload_observer.last_navigation_type());
+  }
+  EXPECT_EQ(3, nav_controller.GetEntryCount());
+  EXPECT_EQ(1, nav_controller.GetLastCommittedEntryIndex());
   EXPECT_EQ(
       GURL(kUnreachableWebDataURL),
       shell()->web_contents()->GetMainFrame()->GetSiteInstance()->GetSiteURL());
@@ -4378,8 +4431,13 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, ErrorPageNavigationReload) {
     EXPECT_TRUE(ExecuteScript(shell(), "location.reload();"));
     reload_observer.Wait();
     EXPECT_TRUE(reload_observer.last_navigation_succeeded());
+    // TODO(nasko): Investigate making renderer initiated reloads that change
+    // SiteInstance be classified as NEW_PAGE as well.
+    EXPECT_EQ(NavigationType::NAVIGATION_TYPE_EXISTING_PAGE,
+              reload_observer.last_navigation_type());
   }
-  EXPECT_EQ(2, nav_controller.GetEntryCount());
+  EXPECT_EQ(3, nav_controller.GetEntryCount());
+  EXPECT_EQ(1, nav_controller.GetLastCommittedEntryIndex());
   EXPECT_NE(
       GURL(kUnreachableWebDataURL),
       shell()->web_contents()->GetMainFrame()->GetSiteInstance()->GetSiteURL());
@@ -4459,6 +4517,64 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
       shell()->web_contents()->GetMainFrame()->GetSiteInstance()->GetSiteURL());
 }
 
+// Test to verify that when an error page is hit and its process is terminated,
+// a successful reload correctly commits in a different process.
+// See https://crbug.com/866549.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
+                       ErrorPageNavigationReloadWithTerminatedProcess) {
+  // This test is only valid if error page isolation is enabled.
+  if (!SiteIsolationPolicy::IsErrorPageIsolationEnabled(true))
+    return;
+
+  StartEmbeddedServer();
+  GURL url(embedded_test_server()->GetURL("/title1.html"));
+  GURL error_url(embedded_test_server()->GetURL("/empty.html"));
+  std::unique_ptr<URLLoaderInterceptor> url_interceptor =
+      SetupRequestFailForURL(error_url);
+  WebContents* web_contents = shell()->web_contents();
+  NavigationControllerImpl& nav_controller =
+      static_cast<NavigationControllerImpl&>(web_contents->GetController());
+
+  // Start with a successful navigation to a document.
+  EXPECT_TRUE(NavigateToURL(shell(), url));
+  scoped_refptr<SiteInstance> success_site_instance =
+      web_contents->GetMainFrame()->GetSiteInstance();
+  EXPECT_EQ(1, nav_controller.GetEntryCount());
+
+  // Navigate to an url resulting in an error page.
+  EXPECT_FALSE(NavigateToURL(shell(), error_url));
+  EXPECT_EQ(GURL(kUnreachableWebDataURL),
+            web_contents->GetMainFrame()->GetSiteInstance()->GetSiteURL());
+  EXPECT_EQ(GURL(kUnreachableWebDataURL),
+            ChildProcessSecurityPolicyImpl::GetInstance()->GetOriginLock(
+                web_contents->GetSiteInstance()->GetProcess()->GetID()));
+  EXPECT_EQ(2, nav_controller.GetEntryCount());
+
+  // Terminate the renderer process.
+  {
+    RenderProcessHostWatcher termination_observer(
+        web_contents->GetMainFrame()->GetProcess(),
+        RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+    web_contents->GetMainFrame()->GetProcess()->Shutdown(0);
+    termination_observer.Wait();
+  }
+
+  // Clear the interceptor so the navigation will succeed on reload.
+  url_interceptor.reset();
+
+  // Reload the URL and execute a Javascript statement to verify that the
+  // renderer process is still around and responsive.
+  TestNavigationObserver reload_observer(shell()->web_contents());
+  nav_controller.Reload(ReloadType::NORMAL, false);
+  reload_observer.Wait();
+  EXPECT_TRUE(reload_observer.last_navigation_succeeded());
+
+  std::string result;
+  EXPECT_TRUE(ExecuteScriptAndExtractString(
+      shell(), "window.domAutomationController.send(location.href);", &result));
+  EXPECT_EQ(error_url.spec(), result);
+}
+
 // A NavigationThrottle implementation that blocks all outgoing navigation
 // requests for a specific WebContents. It is used to block navigations to
 // WebUI URLs in the following test.

content/public/test/navigation_simulator.cc

@@ -512,6 +512,7 @@ void NavigationSimulator::Commit() {
   params.origin = url::Origin::Create(navigation_url_);
   params.referrer = referrer_;
   params.transition = transition_;
+  params.redirects.push_back(navigation_url_);
   params.should_update_history = true;
   params.did_create_new_entry = DidCreateNewEntry();
   params.gesture =

content/public/test/test_navigation_observer.cc

@@ -5,8 +5,8 @@
 #include "content/public/test/test_navigation_observer.h"
 
 #include "base/bind.h"
+#include "content/browser/frame_host/navigation_handle_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
-#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents_observer.h"
 
 namespace content {
@@ -186,6 +186,8 @@ void TestNavigationObserver::OnDidFinishNavigation(
   last_navigation_url_ = navigation_handle->GetURL();
   last_navigation_succeeded_ = !navigation_handle->IsErrorPage();
   last_net_error_code_ = navigation_handle->GetNetErrorCode();
+  last_navigation_type_ =
+      static_cast<NavigationHandleImpl*>(navigation_handle)->navigation_type();
 
   if (wait_event_ == WaitEvent::kNavigationFinished)
     EventTriggered();

content/public/test/test_navigation_observer.h

@@ -10,6 +10,7 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "content/public/browser/navigation_type.h"
 #include "content/public/test/test_utils.h"
 #include "net/base/net_errors.h"
 #include "url/gurl.h"
@@ -68,6 +69,8 @@ class TestNavigationObserver {
 
   net::Error last_net_error_code() const { return last_net_error_code_; }
 
+  NavigationType last_navigation_type() const { return last_navigation_type_; }
+
  protected:
   // Register this TestNavigationObserver as an observer of the |web_contents|.
   void RegisterAsObserver(WebContents* web_contents);
@@ -123,6 +126,9 @@ class TestNavigationObserver {
   // The net error code of the last navigation.
   net::Error last_net_error_code_;
 
+  // The NavigationType of the last navigation.
+  NavigationType last_navigation_type_;
+
   // The MessageLoopRunner used to spin the message loop.
   scoped_refptr<MessageLoopRunner> message_loop_runner_;