AW: implement setCookie for the network service

This implements CookieManager#setCookie for the network service code
path, plumbing SetCanonicalCookie through AwCookieManagerWrapper.

Along with this change, this re-implements #setCookie for the legacy
code path, to use SetCanonicalCookieAsync instead of
SetCookieWithOptionsAsync (to share code between the new and old code
paths).

This splits apart a test, which I found to be useful when debugging
(since the "good URL" and "bad URL" case are actually quite different).

This removes a bunch of tests from the test filter, and adds in new
breakages (mainly due to #hasCookies() not being implemented yet).

This also updates some TODOs, to point at finer-grained crbugs I've
filed since the last CL.

Bug: 933460, 931641, 902641, 933462, 893580
Test: run_webview_instrumentation_test_apk -f=CookieManagerTest#* \
Test: --enable-features=NetworkService,NetworkServiceInProcess
Cq-Include-Trybots: master.tryserver.chromium.android:android_mojo
Change-Id: I3f29a209aa9a442d273b8673c01848801f298ee8
Reviewed-on: https://chromium-review.googlesource.com/c/1478470Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634460}

android_webview/browser/cookie_manager.cc

@@ -30,6 +30,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/time/time.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/cookie_store_factory.h"
 #include "jni/AwCookieManager_jni.h"
@@ -95,6 +96,38 @@ class BoolCookieCallbackHolder {
 
 namespace {
 
+// Copied from net/cookies/cookie_util.h for consistency and backwards
+// compatibility.
+const int kVlogSetCookies = 7;
+
+// TODO(ntfschr): see if we can turn this into OnceCallback.
+// http://crbug.com/932535.
+void MaybeRunCookieCallback(base::RepeatingCallback<void(bool)> callback,
+                            const bool& result) {
+  if (callback)
+    std::move(callback).Run(result);
+}
+
+GURL MaybeFixUpSchemeForSecureCookie(const GURL& host,
+                                     const std::string& value) {
+  // Log message for catching strict secure cookies related bugs.
+  // TODO(sgurun) temporary. Add UMA stats to monitor, and remove afterwards.
+  // http://crbug.com/933981.
+  if (host.is_valid() &&
+      (!host.has_scheme() || host.SchemeIs(url::kHttpScheme))) {
+    net::ParsedCookie parsed_cookie(value);
+    if (parsed_cookie.IsValid() && parsed_cookie.IsSecure()) {
+      LOG(WARNING) << "Strict Secure Cookie policy does not allow setting a "
+                      "secure cookie for "
+                   << host.spec();
+      GURL::Replacements replace_host;
+      replace_host.SetSchemeStr("https");
+      return host.ReplaceComponents(replace_host);
+    }
+  }
+  return host;
+}
+
 // Construct a closure which signals a waitable event if and when the closure
 // is called the waitable event must still exist.
 static base::RepeatingClosure SignalEventClosure(WaitableEvent* completion) {
@@ -308,26 +341,63 @@ void CookieManager::SetCookieHelper(
   net::CookieOptions options;
   options.set_include_httponly();
 
-  // Log message for catching strict secure cookies related bugs.
-  // TODO(sgurun) temporary. Add UMA stats to monitor, and remove afterwards.
-  if (host.is_valid() &&
-      (!host.has_scheme() || host.SchemeIs(url::kHttpScheme))) {
-    net::ParsedCookie parsed_cookie(value);
-    if (parsed_cookie.IsValid() && parsed_cookie.IsSecure()) {
-      LOG(WARNING) << "Strict Secure Cookie policy does not allow setting a "
-                      "secure cookie for "
-                   << host.spec();
-      GURL::Replacements replace_host;
-      replace_host.SetSchemeStr("https");
-      GURL new_host = host.ReplaceComponents(replace_host);
-      GetCookieStore()->SetCookieWithOptionsAsync(new_host, value, options,
-                                                  StatusToBool(callback));
+  const GURL& new_host = MaybeFixUpSchemeForSecureCookie(host, value);
+
+  // The below is copied from net::CookieMonster::SetCookieWithOptions. We do
+  // this because we have strict requirements to keep behavior identical across
+  // WebView versions.
+
+  // If this scheme is not cookieable, then we should not set a cookie for it.
+  // Instead, invoke the callback and indicate failure.
+  if (!HasCookieableScheme(new_host)) {
+    MaybeRunCookieCallback(std::move(callback), false);
+    return;
+  }
+
+  VLOG(kVlogSetCookies) << "SetCookie() line: " << value;
+
+  net::CanonicalCookie::CookieInclusionStatus status;
+
+  std::unique_ptr<net::CanonicalCookie> cc(net::CanonicalCookie::Create(
+      new_host, value, base::Time::Now(), options, &status));
+
+  if (status != net::CanonicalCookie::CookieInclusionStatus::INCLUDE) {
+    DCHECK(!cc);
+    VLOG(kVlogSetCookies) << "WARNING: Failed to allocate CanonicalCookie";
+    MaybeRunCookieCallback(std::move(callback), false);
     return;
   }
+
+  DCHECK(cc);
+
+  // Note: CookieStore and network::CookieManager have different signatures: one
+  // accepts a boolean callback while the other (recently) changed to accept a
+  // CookieInclusionStatus callback. WebView only cares about boolean success,
+  // which is why we use StatusToBool. This is temporary technical debt until we
+  // fully launch the Network Service code path.
+  if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
+    // *cc.get() is safe, because network::CookieManager::SetCanonicalCookie
+    // will make a copy before our smart pointer goes out of scope.
+    GetCookieManagerWrapper()->SetCanonicalCookie(
+        *cc.get(), new_host.SchemeIsCryptographic(),
+        !options.exclude_httponly(), std::move(callback));
+  } else {
+    GetCookieStore()->SetCanonicalCookieAsync(
+        std::move(cc), new_host.SchemeIsCryptographic(),
+        !options.exclude_httponly(), StatusToBool(callback));
   }
+}
 
-  GetCookieStore()->SetCookieWithOptionsAsync(host, value, options,
-                                              StatusToBool(callback));
+bool CookieManager::HasCookieableScheme(const GURL& host) {
+  for (int i = 0; i < net::CookieMonster::kDefaultCookieableSchemesCount; ++i) {
+    if (host.SchemeIs(net::CookieMonster::kDefaultCookieableSchemes[i])) {
+      return true;
+    }
+  }
+  if (host.SchemeIsFile() && AllowFileSchemeCookies()) {
+    return true;
+  }
+  return false;
 }
 
 std::string CookieManager::GetCookie(const GURL& host) {

android_webview/browser/cookie_manager.h

@@ -113,6 +113,9 @@ class CookieManager {
                            bool* result,
                            const net::CookieList& cookies,
                            const net::CookieStatusList& excluded_cookies);
+  // Determine if cookies can be set for |host|, based on its scheme. This is
+  // based on net::CookieMonster::HasCookieableScheme.
+  bool HasCookieableScheme(const GURL& host);
 
   // This protects the following two bools, as they're used on multiple threads.
   base::Lock accept_file_scheme_cookies_lock_;

android_webview/browser/net_network_service/aw_cookie_manager_wrapper.cc

@@ -26,8 +26,19 @@ void AwCookieManagerWrapper::GetCookieList(
     const net::CookieOptions& cookie_options,
     GetCookieListCallback callback) {
   // TODO(ntfschr): handle the case where content layer isn't initialized yet
-  // (http://crbug.com/902641).
+  // (http://crbug.com/933461).
   cookie_manager_->GetCookieList(url, cookie_options, std::move(callback));
 }
 
+void AwCookieManagerWrapper::SetCanonicalCookie(
+    const net::CanonicalCookie& cc,
+    bool secure_source,
+    bool modify_http_only,
+    SetCanonicalCookieCallback callback) {
+  // TODO(ntfschr): handle the case where content layer isn't initialized yet
+  // (http://crbug.com/933461).
+  cookie_manager_->SetCanonicalCookie(cc, secure_source, modify_http_only,
+                                      std::move(callback));
+}
+
 }  // namespace android_webview

android_webview/browser/net_network_service/aw_cookie_manager_wrapper.h

@@ -11,9 +11,6 @@
 
 namespace android_webview {
 
-using GetCookieListCallback =
-    base::OnceCallback<void(const std::vector<net::CanonicalCookie>&)>;
-
 // AwCookieManagerWrapper is a thin wrapper around
 // network::mojom::CookieManager. This lives on the CookieStore TaskRunner. This
 // class's main responsibility is to support the CookieManager APIs before it
@@ -24,17 +21,30 @@ class AwCookieManagerWrapper {
   AwCookieManagerWrapper();
   ~AwCookieManagerWrapper();
 
+  // We redefine these type aliases for consistency and readability. These are
+  // originally defined by generated mojo code in
+  // out/<folder>/gen/services/network/public/mojom/cookie_manager.mojom.h.
+  using GetCookieListCallback =
+      network::mojom::CookieManager::GetCookieListCallback;
+  using SetCanonicalCookieCallback =
+      network::mojom::CookieManager::SetCanonicalCookieCallback;
+
   // Called when content layer starts up, to pass in a NetworkContextPtr for us
   // to use for Cookies APIs.
   void SetMojoCookieManager(
       network::mojom::CookieManagerPtrInfo cookie_manager_info);
 
   // Thin wrappers around network::mojom::CookieManager APIs.
-  // TODO(ntfschr): implement the other APIs we need (http://crbug.com/902641).
+  // TODO(ntfschr): implement the other APIs we need (http://crbug.com/933462).
   void GetCookieList(const GURL& url,
                      const net::CookieOptions& cookie_options,
                      GetCookieListCallback callback);
 
+  void SetCanonicalCookie(const net::CanonicalCookie& cc,
+                          bool secure_source,
+                          bool modify_http_only,
+                          SetCanonicalCookieCallback);
+
  private:
   // A CookieManagerPtr which is cloned from the NetworkContext's
   // CookieManagerPtr (but, lives on this thread).

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

@@ -299,10 +299,9 @@ public class CookieManagerTest {
     @Test
     @MediumTest
     @Feature({"AndroidWebView", "Privacy"})
-    public void testSetCookieCallback() throws Throwable {
+    public void testSetCookieCallback_goodUrl() throws Throwable {
         final String url = "http://www.example.com";
         final String cookie = "name=test";
-        final String brokenUrl = "foo";
 
         final TestCallback<Boolean> callback = new TestCallback<Boolean>();
         int callCount = callback.getOnResultHelper().getCallCount();
@@ -311,13 +310,23 @@ public class CookieManagerTest {
         callback.getOnResultHelper().waitForCallback(callCount);
         Assert.assertTrue(callback.getValue());
         Assert.assertEquals(cookie, mCookieManager.getCookie(url));
+    }
 
-        callCount = callback.getOnResultHelper().getCallCount();
+    @Test
+    @MediumTest
+    @Feature({"AndroidWebView", "Privacy"})
+    public void testSetCookieCallback_badUrl() throws Throwable {
+        final String cookie = "name=test";
+        final String brokenUrl = "foo";
+
+        final TestCallback<Boolean> callback = new TestCallback<Boolean>();
+        int callCount = callback.getOnResultHelper().getCallCount();
 
         setCookieOnUiThread(brokenUrl, cookie, callback);
         callback.getOnResultHelper().waitForCallback(callCount);
-        Assert.assertFalse(callback.getValue());
-        Assert.assertEquals(null, mCookieManager.getCookie(brokenUrl));
+        Assert.assertFalse("Cookie should not be set for bad URLs", callback.getValue());
+        Assert.assertNull("getCookie should be null if cookie hasn't been set",
+                mCookieManager.getCookie(brokenUrl));
     }
 
     @Test
@@ -333,6 +342,7 @@ public class CookieManagerTest {
         mCookieManager.setCookie(url, cookie, null);
 
         AwActivityTestRule.pollInstrumentationThread(() -> mCookieManager.hasCookies());
+        Assert.assertEquals(cookie, mCookieManager.getCookie(url));
     }
 
     @Test

testing/buildbot/filters/mojo.fyi.network_webview_instrumentation_test_apk.filter

@@ -20,35 +20,29 @@
 -org.chromium.android_webview.test.AwContentsClientShouldInterceptRequestTest.testLoadDataWithBaseUrlTriggersShouldInterceptRequest
 
 # https://crbug.com/893575
--org.chromium.android_webview.test.CookieManagerStartupTest.testShouldInterceptRequestDeadlock
 -org.chromium.android_webview.test.CookieManagerStartupTest.testStartup
 
 # https://crbug.com/893576
 -org.chromium.android_webview.test.CookieManagerTest.testAcceptCookie_falseWontSetCookies
 -org.chromium.android_webview.test.CookieManagerTest.testAcceptFileSchemeCookies
--org.chromium.android_webview.test.CookieManagerTest.testEmbedderCanSeeRestrictedCookies
--org.chromium.android_webview.test.CookieManagerTest.testRejectFileSchemeCookies
 -org.chromium.android_webview.test.CookieManagerTest.testThirdPartyCookie
--org.chromium.android_webview.test.CookieManagerTest.testThirdPartyCookieForWebSocketEnabledCase
 -org.chromium.android_webview.test.CookieManagerTest.testThirdPartyCookiesArePerWebview
--org.chromium.android_webview.test.CookieManagerTest.testThirdPartyJavascriptCookie
 
 # https://crbug.com/931641
 -org.chromium.android_webview.test.CookieManagerTest.testRemoveSessionCookiesNullCallback
--org.chromium.android_webview.test.CookieManagerTest.testSetSecureCookieForHttpUrl
--org.chromium.android_webview.test.CookieManagerTest.testSetCookie
 -org.chromium.android_webview.test.CookieManagerTest.testThirdPartyCookieForWebSocketDisabledCase
--org.chromium.android_webview.test.CookieManagerTest.testSetCookieWithDomainForUrlWithTrailingSemicolonInCookie
--org.chromium.android_webview.test.CookieManagerTest.testSetCookieWithDomainForUrl
 -org.chromium.android_webview.test.CookieManagerTest.testRemoveSessionCookies
--org.chromium.android_webview.test.CookieManagerTest.testSetCookieCallback
 -org.chromium.android_webview.test.CookieManagerTest.testRemoveSessionCookiesCallback
--org.chromium.android_webview.test.CookieManagerTest.testSetCookieWithDomainForUrlAndExistingDomainAttribute
--org.chromium.android_webview.test.CookieManagerTest.testCookieExpiration
+
+# https://crbug.com/933629
+-org.chromium.android_webview.test.CookieManagerTest.testCookiesExpire
+-org.chromium.android_webview.test.CookieManagerTest.testHasCookie
+-org.chromium.android_webview.test.CookieManagerTest.testRemoveAllCookies
+-org.chromium.android_webview.test.CookieManagerTest.testRemoveAllCookiesCallback
+-org.chromium.android_webview.test.CookieManagerTest.testSetCookieNullCallback
 
 # https://crbug.com/893580
 -org.chromium.android_webview.test.LoadDataWithBaseUrlTest.testLoadDataWithBaseUrlAccessingFile
--org.chromium.android_webview.test.LoadDataWithBaseUrlTest.testThirdPartyCookieInIframe
 
 # https://crbug.com/893582
 -org.chromium.android_webview.test.SafeBrowsingTest.testSafeBrowsingDontProceedCausesNetworkErrorForMainFrame