Fix JS cookie access failing or hanging after network process crashes.

Bug: 887680
Change-Id: I249e640a559e550b879370f2f02d2437e52ac93b
Reviewed-on: https://chromium-review.googlesource.com/1236282
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Chong Zhang <chongz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593038}

content/browser/frame_host/render_frame_message_filter.cc

@@ -273,19 +273,6 @@ RenderFrameMessageFilter::RenderFrameMessageFilter(
       render_widget_helper_(render_widget_helper),
       incognito_(browser_context->IsOffTheRecord()),
       render_process_id_(render_process_id) {
-  network::mojom::CookieManagerPtr cookie_manager;
-  storage_partition->GetNetworkContext()->GetCookieManager(
-      mojo::MakeRequest(&cookie_manager));
-
-  // The PostTask below could finish before the constructor returns which would
-  // lead to this object being destructed prematurely.
-  AddRef();
-  base::ThreadTaskRunnerHandle::Get()->ReleaseSoon(FROM_HERE, this);
-
-  base::PostTaskWithTraits(
-      FROM_HERE, {BrowserThread::IO},
-      base::BindOnce(&RenderFrameMessageFilter::InitializeOnIO, this,
-                     cookie_manager.PassInterface()));
 }
 
 RenderFrameMessageFilter::~RenderFrameMessageFilter() {
@@ -293,9 +280,26 @@ RenderFrameMessageFilter::~RenderFrameMessageFilter() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 }
 
-void RenderFrameMessageFilter::InitializeOnIO(
-    network::mojom::CookieManagerPtrInfo cookie_manager) {
-  cookie_manager_.Bind(std::move(cookie_manager));
+network::mojom::CookieManagerPtr* RenderFrameMessageFilter::GetCookieManager() {
+  if (!cookie_manager_ || cookie_manager_.encountered_error()) {
+    base::PostTaskWithTraits(
+        FROM_HERE, {BrowserThread::UI},
+        base::BindOnce(&RenderFrameMessageFilter::InitializeCookieManager, this,
+                       mojo::MakeRequest(&cookie_manager_)));
+  }
+  return &cookie_manager_;
+}
+
+void RenderFrameMessageFilter::InitializeCookieManager(
+    network::mojom::CookieManagerRequest cookie_manager_request) {
+  RenderProcessHost* render_process_host =
+      RenderProcessHost::FromID(render_process_id_);
+  if (!render_process_host)
+    return;
+
+  render_process_host->GetStoragePartition()
+      ->GetNetworkContext()
+      ->GetCookieManager(std::move(cookie_manager_request));
 }
 
 bool RenderFrameMessageFilter::OnMessageReceived(const IPC::Message& message) {
@@ -558,9 +562,10 @@ void RenderFrameMessageFilter::SetCookie(int32_t render_frame_id,
   // this process' StoragePartition.
   if (base::FeatureList::IsEnabled(network::features::kNetworkService) &&
       context == request_context_->GetURLRequestContext()) {
-    cookie_manager_->SetCanonicalCookie(*cookie, url.SchemeIsCryptographic(),
-                                        !options.exclude_httponly(),
-                                        net::CookieStore::SetCookiesCallback());
+    (*GetCookieManager())
+        ->SetCanonicalCookie(*cookie, url.SchemeIsCryptographic(),
+                             !options.exclude_httponly(),
+                             net::CookieStore::SetCookiesCallback());
     return;
   }
 
@@ -607,11 +612,12 @@ void RenderFrameMessageFilter::GetCookies(int render_frame_id,
       context == request_context_->GetURLRequestContext()) {
     // TODO(jam): modify GetRequestContextForURL to work with network service.
     // Merge this with code path below for non-network service.
-    cookie_manager_->GetCookieList(
-        url, options,
-        base::BindOnce(&RenderFrameMessageFilter::CheckPolicyForCookies, this,
-                       render_frame_id, url, site_for_cookies,
-                       std::move(callback)));
+    (*GetCookieManager())
+        ->GetCookieList(
+            url, options,
+            base::BindOnce(&RenderFrameMessageFilter::CheckPolicyForCookies,
+                           this, render_frame_id, url, site_for_cookies,
+                           std::move(callback)));
     return;
   }
 

content/browser/frame_host/render_frame_message_filter.h

@@ -73,6 +73,8 @@ class CONTENT_EXPORT RenderFrameMessageFilter
   bool OnMessageReceived(const IPC::Message& message) override;
   void OnDestruct() const override;
 
+  network::mojom::CookieManagerPtr* GetCookieManager();
+
  protected:
   friend class TestSaveImageFromDataURL;
 
@@ -97,7 +99,8 @@ class CONTENT_EXPORT RenderFrameMessageFilter
 
   ~RenderFrameMessageFilter() override;
 
-  void InitializeOnIO(network::mojom::CookieManagerPtrInfo cookie_manager);
+  void InitializeCookieManager(
+      network::mojom::CookieManagerRequest cookie_manager_request);
 
   // |new_render_frame_id| and |devtools_frame_token| are out parameters.
   // Browser process defines them for the renderer process.

content/browser/network_service_restart_browsertest.cc

@@ -2,17 +2,19 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
+#include "content/browser/frame_host/render_frame_message_filter.h"
+#include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/browser/url_loader_factory_getter.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/network_service_instance.h"
-#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
@@ -665,4 +667,42 @@ IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest,
   EXPECT_FALSE(CheckContainsProcessID(network_usages, process_id2));
 }
 
+// Make sure cookie access doesn't hang or fail after a network process crash.
+IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest, Cookies) {
+  auto* web_contents = shell()->web_contents();
+  NavigateToURL(shell(), embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_TRUE(ExecuteScript(web_contents, "document.cookie = 'foo=bar';"));
+
+  std::string cookie;
+  EXPECT_TRUE(ExecuteScriptAndExtractString(
+      web_contents, "window.domAutomationController.send(document.cookie);",
+      &cookie));
+  EXPECT_EQ("foo=bar", cookie);
+
+  SimulateNetworkServiceCrash();
+
+  auto* process = web_contents->GetMainFrame()->GetProcess();
+  scoped_refptr<RenderFrameMessageFilter> filter(
+      static_cast<RenderProcessHostImpl*>(process)
+          ->render_frame_message_filter_for_testing());
+  // Need to use FlushAsyncForTesting instead of FlushForTesting because the IO
+  // thread doesn't support nested message loops.
+  base::RunLoop run_loop;
+  base::PostTaskWithTraits(FROM_HERE, {BrowserThread::IO},
+                           base::BindLambdaForTesting([&]() {
+                             filter->GetCookieManager()->FlushAsyncForTesting(
+                                 run_loop.QuitClosure());
+                           }));
+
+  // content_shell uses in-memory cookie database, so the value saved earlier
+  // won't persist across crashes. What matters is that new access works.
+  EXPECT_TRUE(ExecuteScript(web_contents, "document.cookie = 'foo=bar';"));
+
+  // This will hang without the fix.
+  EXPECT_TRUE(ExecuteScriptAndExtractString(
+      web_contents, "window.domAutomationController.send(document.cookie);",
+      &cookie));
+  EXPECT_EQ("foo=bar", cookie);
+}
+
 }  // namespace content