Reland "Introduce the ReduceCookiesIPC experiment"

This is a reland of commit 05fbc19c18

Test flakiness that caused revert is expected to be fixed by eliminating a race between cookie change notifications being sent and unblocking the Renderer with https://chromium-review.googlesource.com/c/chromium/src/+/4766038

The change also includes a fix for crbug.com/1468909#c14 where caching a null string resulted in invalid results. Neither cached nor returned string can now be null.

Original change's description:
> Introduce the ReduceCookiesIPC experiment
>
> BUG: 1393050
> Change-Id: I29c9120792727bf0a6a3e69271aaa12605bb2afb
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4664207
> Reviewed-by: Maks Orlovich <morlovich@chromium.org>
> Reviewed-by: Peter Beverloo <peter@chromium.org>
> Reviewed-by: Mark Foltz <mfoltz@chromium.org>
> Commit-Queue: Olivier Li <olivierli@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1176270}

Bug: 1393050
Change-Id: I05927d28ffb595cb08d93ce54c2ab5de25223956
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4771003
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Mark Foltz <mfoltz@chromium.org>
Commit-Queue: Olivier Li <olivierli@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1184786}

android_webview/browser/network_service/aw_proxying_restricted_cookie_manager.cc

@@ -16,6 +16,7 @@
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
+#include "services/network/public/mojom/restricted_cookie_manager.mojom-forward.h"
 #include "url/gurl.h"
 
 namespace android_webview {
@@ -169,7 +170,8 @@ void AwProxyingRestrictedCookieManager::GetCookiesString(
         url, site_for_cookies, top_frame_origin, has_storage_access,
         /*get_version_shared_memory=*/false, std::move(callback));
   } else {
-    std::move(callback).Run(base::ReadOnlySharedMemoryRegion(), "");
+    std::move(callback).Run(network::mojom::kInvalidCookieVersion,
+                            base::ReadOnlySharedMemoryRegion(), "");
   }
 }
 

content/browser/media/android/media_resource_getter_impl.cc

@@ -90,8 +90,13 @@ void ReturnResultOnUIThread(
 void ReturnResultOnUIThreadAndClosePipe(
     mojo::Remote<network::mojom::RestrictedCookieManager> pipe,
     base::OnceCallback<void(const std::string&)> callback,
+    uint64_t version,
     base::ReadOnlySharedMemoryRegion shared_memory_region,
     const std::string& result) {
+  // Clients of GetCookiesString() are free to use |shared_memory_region| and
+  // |result| to avoid IPCs when possible. This class has not proven to be a
+  // high enough source of IPC traffic to warrant wiring this up. Using them
+  // is completely optional so they are simply dropped here.
   GetUIThreadTaskRunner({})->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), result));
 }

services/network/cookie_manager.cc

@@ -73,6 +73,11 @@ CookieManager::CookieManager(
 }
 
 CookieManager::~CookieManager() {
+  // The cookie manager will go away which means potentially clearing cookies if
+  // policy calls for it. This can be important for background mode for which
+  // renderers might stay active.
+  OnSettingsWillChange();
+
   if (session_cleanup_cookie_store_) {
     session_cleanup_cookie_store_->DeleteSessionCookies(
         cookie_settings_.CreateDeleteCookieOnExitPredicate());

services/network/public/mojom/restricted_cookie_manager.mojom

@@ -134,12 +134,14 @@ interface RestrictedCookieManager {
   // The number is read from renderers and incremented monotonically from the
   // network process when cookie state changes. It can be null if
   // |get_version_shared_memory| was false or if initialization of the region
-  // failed.
+  // failed. |version| contains the cookie version at the time of returning the
+  // string.
   [Sync]
   GetCookiesString(url.mojom.Url url,
                    SiteForCookies site_for_cookies,
                    url.mojom.Origin top_frame_origin,
                    bool has_storage_access, bool get_version_shared_memory) => (
+                   uint64 version,
                    mojo_base.mojom.ReadOnlySharedMemoryRegion? version_buffer,
                    string cookies);
 

services/network/restricted_cookie_manager.cc

@@ -4,6 +4,7 @@
 
 #include "services/network/restricted_cookie_manager.h"
 
+#include <cstdint>
 #include <memory>
 #include <utility>
 #include <vector>
@@ -377,6 +378,7 @@ RestrictedCookieManager::~RestrictedCookieManager() {
 }
 
 void RestrictedCookieManager::IncrementSharedVersion() {
+  CHECK(mapped_region_.IsValid());
   // Relaxed memory order since only the version is stored within the region
   // and as such is the only data shared between processes. There is no
   // re-ordering to worry about.
@@ -384,6 +386,15 @@ void RestrictedCookieManager::IncrementSharedVersion() {
       1, std::memory_order_relaxed);
 }
 
+uint64_t RestrictedCookieManager::GetSharedVersion() {
+  CHECK(mapped_region_.IsValid());
+  // Relaxed memory order since only the version is stored within the region
+  // and as such is the only data shared between processes. There is no
+  // re-ordering to worry about.
+  return mapped_region_.mapping.GetMemoryAs<SharedVersionType>()->load(
+      std::memory_order_relaxed);
+}
+
 void RestrictedCookieManager::OnCookieSettingsChanged() {
   // Cookie settings changes can change cookie values as seen by content.
   // Increment the shared version to make sure it issues a full cookie string
@@ -688,11 +699,6 @@ void RestrictedCookieManager::SetCanonicalCookie(
     return;
   }
 
-  // The cookie is about to change, increment version. This is best effort
-  // change detection. Remove this line once proper change listening is set
-  // up as part of crbug.com/1393050.
-  IncrementSharedVersion();
-
   net::CanonicalCookie cookie_copy = *sanitized_cookie;
   net::CookieOptions options =
       MakeOptionsForSet(role_, url, site_for_cookies, cookie_settings());
@@ -775,6 +781,12 @@ void RestrictedCookieManager::SetCookieFromString(
     SetCookieFromStringCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+  // The cookie is about to be set. Proactively increment the version so it's
+  // instantly reflected. This ensures that changes a reflected before the
+  // optimistic callback invocation further down that unblocks the caller before
+  // the cookie is actually set.
+  IncrementSharedVersion();
+
   bool site_for_cookies_ok =
       BoundSiteForCookies().IsEquivalent(site_for_cookies);
   bool top_frame_origin_ok = top_frame_origin == BoundTopFrameOrigin();
@@ -835,10 +847,32 @@ void RestrictedCookieManager::GetCookiesString(
   base::ReadOnlySharedMemoryRegion shared_memory_region;
   if (get_version_shared_memory) {
     shared_memory_region = mapped_region_.region.Duplicate();
+
+    // Clients can change their URL. If that happens the subscription needs to
+    // mirror that to get the correct updates.
+    bool new_url = cookie_store_subscription_ && change_subscribed_url_ != url;
+
+    if (!cookie_store_subscription_ || new_url) {
+      change_subscribed_url_ = url;
+      cookie_store_subscription_ =
+          cookie_store_->GetChangeDispatcher().AddCallbackForUrl(
+              url, cookie_partition_key_,
+              base::IgnoreArgs<const net::CookieChangeInfo&>(
+                  base::BindRepeating(
+                      &RestrictedCookieManager::IncrementSharedVersion,
+                      base::Unretained(this))));
+    }
   }
 
-  auto bound_callback =
-      base::BindOnce(std::move(callback), std::move(shared_memory_region));
+  // Bind the current shared cookie version to |callback| to be returned once
+  // the cookie string is retrieved. At that point the cookie version might have
+  // been incremented by actions that happened in the meantime. Returning a
+  // slightly stale version like this is still correct since it's a best effort
+  // mechanism to avoid unnecessary IPCs. When the version is stale an
+  // additional IPC will take place which is the way it would always be if there
+  // was not shared memory versioning.
+  auto bound_callback = base::BindOnce(std::move(callback), GetSharedVersion(),
+                                       std::move(shared_memory_region));
 
   // Match everything.
   auto match_options = mojom::CookieManagerGetOptions::New();

services/network/restricted_cookie_manager.h

@@ -189,6 +189,11 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) RestrictedCookieManager
   // cookies the other side could have cached.
   void IncrementSharedVersion();
 
+  // Returns the cookie version shared with clients to determine whether a
+  // cookie string has changed since the last request and a new request needs to
+  // be issued.
+  uint64_t GetSharedVersion();
+
   // The state associated with a CookieChangeListener.
   class Listener;
 
@@ -278,6 +283,9 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) RestrictedCookieManager
 
   url::Origin origin_;
 
+  std::unique_ptr<net::CookieChangeSubscription> cookie_store_subscription_;
+  GURL change_subscribed_url_;
+
   // Holds the browser-provided site_for_cookies and top_frame_origin to which
   // this RestrictedCookieManager is bound. (The frame_origin field is not used
   // directly, but must match the `origin_` if the RCM role is SCRIPT.)

services/network/restricted_cookie_manager_unittest.cc

@@ -542,6 +542,42 @@ TEST_P(RestrictedCookieManagerTest,
   }
 }
 
+// TODO(crbug.com/1393050): Add test cases that modify the cookies through
+// net::CookieStore::SetCanonicalCookie and/or modify the subscription URL.
+TEST_P(RestrictedCookieManagerTest, CookieVersion) {
+  std::string cookies_out;
+  base::ReadOnlySharedMemoryRegion mapped_region;
+  uint64_t version;
+
+  EXPECT_TRUE(backend()->GetCookiesString(
+      kDefaultUrlWithPath, kDefaultSiteForCookies, kDefaultOrigin,
+      /*has_storage_access=*/false, /*get_version_shared_memory=*/false,
+      &version, &mapped_region, &cookies_out));
+  // Version is at initial value on first query.
+  EXPECT_EQ(version, mojom::kInitialCookieVersion);
+
+  EXPECT_TRUE(backend()->GetCookiesString(
+      kDefaultUrlWithPath, kDefaultSiteForCookies, kDefaultOrigin,
+      /*has_storage_access=*/false, /*get_version_shared_memory=*/false,
+      &version, &mapped_region, &cookies_out));
+  // Version is still at initial value since nothing modified the cookie.
+  EXPECT_EQ(version, mojom::kInitialCookieVersion);
+
+  bool site_for_cookies_ok = false;
+  bool top_frame_origin_ok = false;
+  EXPECT_TRUE(backend()->SetCookieFromString(
+      kDefaultUrlWithPath, kDefaultSiteForCookies, kDefaultOrigin,
+      /*has_storage_access=*/false, "new-name=new-value;path=/",
+      &site_for_cookies_ok, &top_frame_origin_ok));
+
+  EXPECT_TRUE(backend()->GetCookiesString(
+      kDefaultUrlWithPath, kDefaultSiteForCookies, kDefaultOrigin,
+      /*has_storage_access=*/false, /*get_version_shared_memory=*/false,
+      &version, &mapped_region, &cookies_out));
+  // Version has been incremented by the set operation.
+  EXPECT_NE(version, mojom::kInitialCookieVersion);
+}
+
 TEST_P(RestrictedCookieManagerTest, GetAllForUrlBlankFilter) {
   SetSessionCookie("cookie-name", "cookie-value", "example.com", "/");
   SetSessionCookie("cookie-name-2", "cookie-value-2", "example.com", "/");
@@ -562,11 +598,12 @@ TEST_P(RestrictedCookieManagerTest, GetAllForUrlBlankFilter) {
   // Can also use the document.cookie-style API to get the same info.
   std::string cookies_out;
   base::ReadOnlySharedMemoryRegion mapped_region;
+  uint64_t version;
 
   EXPECT_TRUE(backend()->GetCookiesString(
       kDefaultUrlWithPath, kDefaultSiteForCookies, kDefaultOrigin,
       /*has_storage_access=*/false, /*get_version_shared_memory=*/false,
-      &mapped_region, &cookies_out));
+      &version, &mapped_region, &cookies_out));
   EXPECT_FALSE(mapped_region.IsValid());
   EXPECT_EQ("cookie-name=cookie-value; cookie-name-2=cookie-value-2",
             cookies_out);
@@ -575,7 +612,7 @@ TEST_P(RestrictedCookieManagerTest, GetAllForUrlBlankFilter) {
   EXPECT_TRUE(backend()->GetCookiesString(
       kDefaultUrlWithPath, kDefaultSiteForCookies, kDefaultOrigin,
       /*has_storage_access=*/false, /*get_version_shared_memory=*/true,
-      &mapped_region, &cookies_out));
+      &version, &mapped_region, &cookies_out));
   EXPECT_TRUE(mapped_region.IsValid());
   EXPECT_EQ("cookie-name=cookie-value; cookie-name-2=cookie-value-2",
             cookies_out);
@@ -722,11 +759,12 @@ TEST_P(RestrictedCookieManagerTest, GetCookieStringFromWrongOrigin) {
   ExpectBadMessage();
   std::string cookies_out;
   base::ReadOnlySharedMemoryRegion mapped_region;
+  uint64_t version;
 
   EXPECT_TRUE(backend()->GetCookiesString(
       kOtherUrlWithPath, kDefaultSiteForCookies, kDefaultOrigin,
       /*has_storage_access=*/false, /*get_version_shared_memory=*/false,
-      &mapped_region, &cookies_out));
+      &version, &mapped_region, &cookies_out));
   EXPECT_TRUE(received_bad_message());
   EXPECT_THAT(cookies_out, IsEmpty());
 
@@ -734,7 +772,7 @@ TEST_P(RestrictedCookieManagerTest, GetCookieStringFromWrongOrigin) {
   EXPECT_TRUE(backend()->GetCookiesString(
       kOtherUrlWithPath, kDefaultSiteForCookies, kDefaultOrigin,
       /*has_storage_access=*/false, /*get_version_shared_memory=*/true,
-      &mapped_region, &cookies_out));
+      &version, &mapped_region, &cookies_out));
   EXPECT_TRUE(received_bad_message());
   EXPECT_THAT(cookies_out, IsEmpty());
 }

third_party/blink/renderer/core/loader/cookie_jar.cc

@@ -14,10 +14,12 @@
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"
 #include "third_party/blink/renderer/core/frame/local_frame.h"
 #include "third_party/blink/renderer/core/frame/web_feature.h"
+#include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 #include "third_party/blink/renderer/platform/weborigin/kurl.h"
 #include "third_party/blink/renderer/platform/weborigin/kurl_hash.h"
 #include "third_party/blink/renderer/platform/wtf/hash_functions.h"
 #include "third_party/blink/renderer/platform/wtf/text/string_hash.h"
+#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
 
 namespace blink {
 namespace {
@@ -107,8 +109,7 @@ void CookieJar::SetCookie(const String& value) {
 
 void CookieJar::OnBackendDisconnect() {
   shared_memory_initialized_ = false;
-  last_cookies_hash_.reset();
-  last_version_ = network::mojom::blink::kInvalidCookieVersion;
+  InvalidateCache();
 }
 
 String CookieJar::Cookies() {
@@ -119,19 +120,32 @@ String CookieJar::Cookies() {
   base::ElapsedTimer timer;
   RequestRestrictedCookieManagerIfNeeded();
 
-  String value;
+  String value = g_empty_string;
   base::ReadOnlySharedMemoryRegion new_mapped_region;
   const bool get_version_shared_memory = !shared_memory_initialized_;
 
-  // Store the latest cookie version to update |last_version_| after getting the
-  // string.
-  const uint64_t new_version = GetSharedCookieVersion();
-
-  backend_->GetCookiesString(
-      cookie_url, document_->SiteForCookies(), document_->TopFrameOrigin(),
-      document_->GetExecutionContext()->HasStorageAccess(),
-      get_version_shared_memory, &new_mapped_region, &value);
+  // Store the latest cookie version to update |last_version_| after attempting
+  // to get the string. Will get updated once more by GetCookiesString() if an
+  // ipc is required.
+  uint64_t new_version = last_version_;
+  if (IPCNeeded()) {
+    if (!backend_->GetCookiesString(
+            cookie_url, document_->SiteForCookies(),
+            document_->TopFrameOrigin(),
+            document_->GetExecutionContext()->HasStorageAccess(),
+            get_version_shared_memory, &new_version, &new_mapped_region,
+            &value)) {
+      // On IPC failure invalidate cached values and return empty string since
+      // there is no guarantee the client can still validly access cookies in
+      // the current context. See crbug.com/1468909.
+      InvalidateCache();
+      return g_empty_string;
+    }
+    last_cookies_ = value;
+  }
 
+  // TODO(crbug.com/1465996): Once determined whether getting an invalid region
+  // is possible add a DCHECK or comment depending.
   if (!shared_memory_initialized_ && new_mapped_region.IsValid()) {
     mapped_region_ = std::move(new_mapped_region);
     mapping_ = mapped_region_.Map();
@@ -141,7 +155,7 @@ String CookieJar::Cookies() {
   UpdateCacheAfterGetRequest(cookie_url, value, new_version);
 
   last_operation_was_set_ = false;
-  return value;
+  return last_cookies_;
 }
 
 bool CookieJar::CookiesEnabled() {
@@ -169,6 +183,8 @@ void CookieJar::SetCookieManager(
 
 void CookieJar::InvalidateCache() {
   last_cookies_hash_.reset();
+  last_cookies_ = String();
+  last_version_ = network::mojom::blink::kInvalidCookieVersion;
 }
 
 uint64_t CookieJar::GetSharedCookieVersion() {
@@ -182,6 +198,29 @@ uint64_t CookieJar::GetSharedCookieVersion() {
   return network::mojom::blink::kInvalidCookieVersion;
 }
 
+bool CookieJar::IPCNeeded() {
+  // Not under the experiment, always use IPCs.
+  if (!RuntimeEnabledFeatures::ReduceCookieIPCsEnabled()) {
+    return true;
+  }
+
+  // An IPC is needed if there is no cached version.
+  if (last_version_ == network::mojom::blink::kInvalidCookieVersion) {
+    return true;
+  }
+
+  // If there is a cached version, there should also be a cached string.
+  CHECK(!last_cookies_.IsNull());
+
+  // Cookie string has changed.
+  if (last_version_ < GetSharedCookieVersion()) {
+    return true;
+  }
+
+  // No IPC needed!
+  return false;
+}
+
 void CookieJar::RequestRestrictedCookieManagerIfNeeded() {
   if (!backend_.is_bound() || !backend_.is_connected()) {
     backend_.reset();

third_party/blink/renderer/core/loader/cookie_jar.h

@@ -43,6 +43,10 @@ class CookieJar : public GarbageCollected<CookieJar> {
   void OnBackendDisconnect();
   uint64_t GetSharedCookieVersion();
 
+  // Returns true if last_cookies_ is not guaranteed to be up to date and an IPC
+  // is needed to get the current cookie string.
+  bool IPCNeeded();
+
   // Updates the fake cookie cache after a
   // RestrictedCookieManager::GetCookiesString request returns.
   //
@@ -78,6 +82,10 @@ class CookieJar : public GarbageCollected<CookieJar> {
   base::ReadOnlySharedMemoryMapping mapping_;
 
   uint64_t last_version_ = network::mojom::blink::kInvalidCookieVersion;
+
+  // Last received cookie string. Null if there is no last cached-version. Can
+  // be empty since that is a valid cookie string.
+  String last_cookies_;
 };
 
 }  // namespace blink

third_party/blink/renderer/platform/runtime_enabled_features.json5

@@ -2945,6 +2945,10 @@
       base_feature: "none",
       origin_trial_feature_name: "ReduceAcceptLanguage"
     },
+    {
+      name: "ReduceCookieIPCs",
+      status: "test",
+    },
     {
       // If enabled, the deviceModel will be reduced to "K" and the
       // androidVersion will be reduced to a static "10" string in android