Fix webview crash loop

In the NOTIFICATION_RENDERER_PROCESS_CLOSED call, some webview apps can
synchronously create a new WebContents and load a page into it. This
breaks content's internal tracking of IPC state which causes the new
renderer to crash again, causing a crash loop.

The root cause is RFHI::OnRenderProcessGone resets the mojo connection
tracking in the new WebContents, which is wrong because the new
WebContents/RFHI was never connected to the old renderer process.

Fix this by somewhat reducing reentrancy by moving
NOTIFICATION_RENDERER_PROCESS_CLOSED after RFHI::OnRenderProcessGone
to avoid this situation.

Bug: 946758
Change-Id: I7be5a66631688dfb9ed77ef30b518e3149c09af2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1545700
Commit-Queue: Bo <boliu@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#646245}

content/browser/navigation_browsertest.cc

@@ -29,6 +29,7 @@
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/download_manager_delegate.h"
 #include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_features.h"
@@ -1791,4 +1792,86 @@ IN_PROC_BROWSER_TEST_F(NavigationBaseBrowserTest,
       base::ContainsKey(response_2.http_request()->headers, "header_name"));
 }
 
+struct NewWebContentsData {
+  NewWebContentsData() = default;
+  NewWebContentsData(NewWebContentsData&& other)
+      : new_web_contents(std::move(other.new_web_contents)),
+        manager(std::move(other.manager)) {}
+
+  std::unique_ptr<WebContents> new_web_contents;
+  std::unique_ptr<TestNavigationManager> manager;
+};
+
+class CreateWebContentsOnCrashObserver : public NotificationObserver {
+ public:
+  CreateWebContentsOnCrashObserver(const GURL& url,
+                                   WebContents* first_web_contents)
+      : url_(url), first_web_contents_(first_web_contents) {}
+
+  void Observe(int type,
+               const NotificationSource& source,
+               const NotificationDetails& details) override {
+    EXPECT_EQ(content::NOTIFICATION_RENDERER_PROCESS_CLOSED, type);
+
+    // Only do this once in the test.
+    if (observed_)
+      return;
+    observed_ = true;
+
+    WebContents::CreateParams new_contents_params(
+        first_web_contents_->GetBrowserContext(),
+        first_web_contents_->GetSiteInstance());
+    data_.new_web_contents = WebContents::Create(new_contents_params);
+    data_.manager = std::make_unique<TestNavigationManager>(
+        data_.new_web_contents.get(), url_);
+    NavigationController::LoadURLParams load_params(url_);
+    data_.new_web_contents->GetController().LoadURLWithParams(load_params);
+  }
+
+  NewWebContentsData TakeNewWebContentsData() { return std::move(data_); }
+
+ private:
+  NewWebContentsData data_;
+  bool observed_ = false;
+
+  GURL url_;
+  WebContents* first_web_contents_;
+};
+
+// This test simulates android webview's behavior in apps that handle
+// renderer crashes by synchronously creating a new WebContents and loads
+// the same page again. This reenters into content code.
+IN_PROC_BROWSER_TEST_F(NavigationBrowserTest, WebViewRendererKillReload) {
+  // Webview is limited to one renderer.
+  RenderProcessHost::SetMaxRendererProcessCount(1u);
+
+  // Load a page into first webview.
+  auto* web_contents = shell()->web_contents();
+  GURL url(embedded_test_server()->GetURL("/simple_links.html"));
+  {
+    TestNavigationObserver observer(web_contents);
+    EXPECT_TRUE(NavigateToURL(web_contents, url));
+    EXPECT_EQ(url, observer.last_navigation_url());
+  }
+
+  // Install a crash observer that synchronously creates and loads a new
+  // WebContents. Then crash the renderer which triggers the observer.
+  CreateWebContentsOnCrashObserver crash_observer(url, web_contents);
+  content::NotificationRegistrar notification_registrar;
+  notification_registrar.Add(&crash_observer,
+                             content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                             content::NotificationService::AllSources());
+  NavigateToURLBlockUntilNavigationsComplete(web_contents, GURL("chrome:crash"),
+                                             1);
+
+  // Wait for navigation in new WebContents to finish.
+  NewWebContentsData data = crash_observer.TakeNewWebContentsData();
+  data.manager->WaitForNavigationFinished();
+
+  // Test passes if renderer is still alive.
+  EXPECT_TRUE(ExecJs(data.new_web_contents.get(), "true;"));
+  EXPECT_TRUE(data.new_web_contents->GetMainFrame()->IsRenderFrameLive());
+  EXPECT_EQ(url, data.new_web_contents->GetMainFrame()->GetLastCommittedURL());
+}
+
 }  // namespace content

content/browser/renderer_host/render_process_host_impl.cc

@@ -4163,12 +4163,8 @@ void RenderProcessHostImpl::ProcessDied(
     info.exit_code = shutdown_exit_code_;
 
   within_process_died_observer_ = true;
-  NotificationService::current()->Notify(
-      NOTIFICATION_RENDERER_PROCESS_CLOSED, Source<RenderProcessHost>(this),
-      Details<ChildProcessTerminationInfo>(&info));
   for (auto& observer : observers_)
     observer.RenderProcessExited(this, info);
-  within_process_died_observer_ = false;
 
   base::IDMap<IPC::Listener*>::iterator iter(&listeners_);
   while (!iter.IsAtEnd()) {
@@ -4177,6 +4173,11 @@ void RenderProcessHostImpl::ProcessDied(
     iter.Advance();
   }
 
+  NotificationService::current()->Notify(
+      NOTIFICATION_RENDERER_PROCESS_CLOSED, Source<RenderProcessHost>(this),
+      Details<ChildProcessTerminationInfo>(&info));
+  within_process_died_observer_ = false;
+
   RemoveUserData(kSessionStorageHolderKey);
 
   // Initialize a new ChannelProxy in case this host is re-used for a new