Fixes a deadlock between shouldInterceptRequest() and getCookie()

In the WebView shouldInterceptRequest is called on the IO thread.
The CookieManager used to post cookie monster tasks to a thread
which was sometimes the IO thread (if the WebView was started 
before the CookieManager) but sometimes a special thread (if the
CookieManager was started before the WebView).

getCookie is synchronous and blocks waiting for the result after
posting its task so if getCookie was called from
shouldInterceptRequest and the cookie monster was on the IO thread
then it deadlocked.

We fix this by always starting the special thread for the cookie
monster.

Android Issue: https://code.google.com/p/android/issues/detail?id=65786

BUG=374203

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

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

android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java

@@ -11,28 +11,25 @@ import android.test.suitebuilder.annotation.SmallTest;
 import org.chromium.android_webview.AwBrowserProcess;
 import org.chromium.android_webview.AwContents;
 import org.chromium.android_webview.AwCookieManager;
+import org.chromium.android_webview.InterceptedRequestData;
 import org.chromium.android_webview.test.util.CommonResources;
 import org.chromium.android_webview.test.util.CookieUtils;
 import org.chromium.base.test.util.Feature;
 import org.chromium.content.app.ContentMain;
 import org.chromium.net.test.util.TestWebServer;
 
+
 /**
- * Test for the CookieManager in the case where it's used before Chromium is started.
+ * Tests for CookieManager/Chromium startup ordering weirdness.
  */
 public class CookieManagerStartupTest extends AwTestBase {
 
-    private AwCookieManager mCookieManager;
     private TestAwContentsClient mContentsClient;
     private AwContents mAwContents;
 
     @Override
     protected void setUp() throws Exception {
         super.setUp();
-
-        mCookieManager = new AwCookieManager();
-        assertNotNull(mCookieManager);
-
         ContentMain.initApplicationContext(getActivity().getApplicationContext());
     }
 
@@ -42,6 +39,10 @@ public class CookieManagerStartupTest extends AwTestBase {
     }
 
     private void startChromium() throws Exception {
+        startChromiumWithClient(new TestAwContentsClient());
+    }
+
+    private void startChromiumWithClient(TestAwContentsClient contentsClient) throws Exception {
         final Context context = getActivity();
         getInstrumentation().runOnMainSync(new Runnable() {
             @Override
@@ -50,7 +51,7 @@ public class CookieManagerStartupTest extends AwTestBase {
             }
         });
 
-        mContentsClient = new TestAwContentsClient();
+        mContentsClient = contentsClient;
         final AwTestContainerView testContainerView =
                 createAwTestContainerViewOnMainSync(mContentsClient);
         mAwContents = testContainerView.getAwContents();
@@ -66,12 +67,16 @@ public class CookieManagerStartupTest extends AwTestBase {
             String path = "/cookie_test.html";
             String url = webServer.setResponse(path, CommonResources.ABOUT_HTML, null);
 
-            CookieUtils.clearCookies(this, mCookieManager);
+            AwCookieManager cookieManager = new AwCookieManager();
+            assertNotNull(cookieManager);
+
+            CookieUtils.clearCookies(this, cookieManager);
+            assertFalse(cookieManager.hasCookies());
 
-            mCookieManager.setAcceptCookie(true);
-            assertTrue(mCookieManager.acceptCookie());
+            cookieManager.setAcceptCookie(true);
+            assertTrue(cookieManager.acceptCookie());
 
-            mCookieManager.setCookie(url, "count=41");
+            cookieManager.setCookie(url, "count=41");
 
             startChromium();
             loadUrlSync(mAwContents, mContentsClient.getOnPageFinishedHelper(), url);
@@ -80,7 +85,7 @@ public class CookieManagerStartupTest extends AwTestBase {
                     mContentsClient,
                     "var c=document.cookie.split('=');document.cookie=c[0]+'='+(1+(+c[1]));");
 
-            assertEquals("count=42", mCookieManager.getCookie(url));
+            assertEquals("count=42", cookieManager.getCookie(url));
         } finally {
             if (webServer != null) webServer.shutdown();
         }
@@ -89,20 +94,38 @@ public class CookieManagerStartupTest extends AwTestBase {
     @SmallTest
     @Feature({"AndroidWebView", "Privacy"})
     public void testAllowFileSchemeCookies() throws Throwable {
-        assertFalse(mCookieManager.allowFileSchemeCookies());
-        mCookieManager.setAcceptFileSchemeCookies(true);
-        assertTrue(mCookieManager.allowFileSchemeCookies());
-        mCookieManager.setAcceptFileSchemeCookies(false);
-        assertFalse(mCookieManager.allowFileSchemeCookies());
+        AwCookieManager cookieManager = new AwCookieManager();
+        assertFalse(cookieManager.allowFileSchemeCookies());
+        cookieManager.setAcceptFileSchemeCookies(true);
+        assertTrue(cookieManager.allowFileSchemeCookies());
+        cookieManager.setAcceptFileSchemeCookies(false);
+        assertFalse(cookieManager.allowFileSchemeCookies());
     }
 
     @SmallTest
     @Feature({"AndroidWebView", "Privacy"})
     public void testAllowCookies() throws Throwable {
-        assertTrue(mCookieManager.acceptCookie());
-        mCookieManager.setAcceptCookie(false);
-        assertFalse(mCookieManager.acceptCookie());
-        mCookieManager.setAcceptCookie(true);
-        assertTrue(mCookieManager.acceptCookie());
+        AwCookieManager cookieManager = new AwCookieManager();
+        assertTrue(cookieManager.acceptCookie());
+        cookieManager.setAcceptCookie(false);
+        assertFalse(cookieManager.acceptCookie());
+        cookieManager.setAcceptCookie(true);
+        assertTrue(cookieManager.acceptCookie());
+    }
+
+    // https://code.google.com/p/chromium/issues/detail?id=374203
+    @MediumTest
+    @Feature({"AndroidWebView"})
+    public void testShouldInterceptRequestDeadlock() throws Throwable {
+        String url = "http://www.example.com";
+        TestAwContentsClient contentsClient = new TestAwContentsClient() {
+            @Override
+            public InterceptedRequestData shouldInterceptRequest(String url) {
+                (new AwCookieManager()).getCookie("www.example.com");
+                return null;
+            }
+        };
+        startChromiumWithClient(contentsClient);
+        loadUrlSync(mAwContents, contentsClient.getOnPageFinishedHelper(), url);
     }
 }

android_webview/native/cookie_manager.cc

@@ -145,8 +145,7 @@ class CookieManager {
  public:
   static CookieManager* GetInstance();
 
-  scoped_refptr<net::CookieStore> CreateBrowserThreadCookieStore(
-      AwBrowserContext* browser_context);
+  scoped_refptr<net::CookieStore> GetCookieStore();
 
   void SetAcceptCookie(bool accept);
   bool AcceptCookie();
@@ -214,8 +213,6 @@ class CookieManager {
   scoped_refptr<base::MessageLoopProxy> cookie_monster_proxy_;
   base::Lock cookie_monster_lock_;
 
-  // Both these threads are normally NULL. They only exist if CookieManager was
-  // accessed before Chromium was started.
   scoped_ptr<base::Thread> cookie_monster_client_thread_;
   scoped_ptr<base::Thread> cookie_monster_backend_thread_;
 
@@ -327,33 +324,9 @@ void CookieManager::ExecCookieTask(const base::Closure& task) {
   cookie_monster_proxy_->PostTask(FROM_HERE, task);
 }
 
-scoped_refptr<net::CookieStore> CookieManager::CreateBrowserThreadCookieStore(
-    AwBrowserContext* browser_context) {
+scoped_refptr<net::CookieStore> CookieManager::GetCookieStore() {
   base::AutoLock lock(cookie_monster_lock_);
-
-  if (cookie_monster_client_thread_) {
-    // We created a cookie monster already on its own threads; we'll just keep
-    // using it rather than creating one on the normal Chromium threads.
-    // CookieMonster is threadsafe, so this is fine.
-    return cookie_monster_;
-  }
-
-  // Go ahead and create the cookie monster using the normal Chromium threads.
-  DCHECK(!cookie_monster_.get());
-  DCHECK(BrowserThread::IsMessageLoopValid(BrowserThread::IO));
-
-  FilePath user_data_dir;
-  GetUserDataDir(&user_data_dir);
-  DCHECK(browser_context->GetPath() == user_data_dir);
-
-  cookie_monster_proxy_ =
-      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO);
-  scoped_refptr<base::SequencedTaskRunner> background_task_runner =
-      BrowserThread::GetBlockingPool()->GetSequencedTaskRunner(
-          BrowserThread::GetBlockingPool()->GetSequenceToken());
-  CreateCookieMonster(user_data_dir,
-                      cookie_monster_proxy_,
-                      background_task_runner);
+  EnsureCookieMonsterExistsLocked();
   return cookie_monster_;
 }
 
@@ -649,8 +622,7 @@ static void SetAcceptFileSchemeCookies(JNIEnv* env, jobject obj,
 
 scoped_refptr<net::CookieStore> CreateCookieStore(
     AwBrowserContext* browser_context) {
-  return CookieManager::GetInstance()->CreateBrowserThreadCookieStore(
-      browser_context);
+  return CookieManager::GetInstance()->GetCookieStore();
 }
 
 bool RegisterCookieManager(JNIEnv* env) {