Remove callback from Init parameters.

This CL makes the observation that the only thing that needs the
complete First-Party Sets is the network service, and the network
service already requests the list of sets when it starts up in
GetNetworkService. So there is no need to also queue up a callback when
the FirstPartySetsHandlerImpl is initialized; we can just wait until the
network service (or something else, in the future) issues its own query.

Change-Id: I897d9ecc523f5be037c8641af26f5eaa6085ee24
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3630911
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Shuran Huang <shuuran@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1001208}

content/browser/browser_main_loop.cc

@@ -980,11 +980,7 @@ int BrowserMainLoop::PreMainMessageLoopRun() {
   FirstPartySetsHandlerImpl::GetInstance()->Init(
       GetContentClient()->browser()->GetFirstPartySetsDirectory(),
       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-          network::switches::kUseFirstPartySet),
-      base::BindOnce([](const base::flat_map<net::SchemefulSite,
-                                             net::SchemefulSite>& sets) {
-        content::GetNetworkService()->SetFirstPartySets(sets);
-      }));
+          network::switches::kUseFirstPartySet));
 
   variations::MaybeScheduleFakeCrash();
 

content/browser/first_party_sets/first_party_sets_handler_impl.cc

@@ -108,25 +108,27 @@ FirstPartySetsHandlerImpl::FirstPartySetsHandlerImpl(
 FirstPartySetsHandlerImpl::~FirstPartySetsHandlerImpl() = default;
 
 absl::optional<FirstPartySetsHandlerImpl::FlattenedSets>
-FirstPartySetsHandlerImpl::GetSetsIfEnabledAndReady() const {
+FirstPartySetsHandlerImpl::GetSets(SetsReadyOnceCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return IsEnabledAndReady() ? sets_ : absl::nullopt;
+  DCHECK(IsEnabled());
+  DCHECK(!callback.is_null());
+  if (sets_.has_value())
+    return sets_;
+
+  on_sets_ready_callbacks_.push_back(std::move(callback));
+  return absl::nullopt;
 }
 
 void FirstPartySetsHandlerImpl::Init(const base::FilePath& user_data_dir,
-                                     const std::string& flag_value,
-                                     SetsReadyOnceCallback on_sets_ready) {
+                                     const std::string& flag_value) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(!initialized_);
   DCHECK(persisted_sets_path_.empty());
-  DCHECK(on_sets_ready_.is_null());
 
   initialized_ = true;
-  on_sets_ready_ = std::move(on_sets_ready);
   SetPersistedSets(user_data_dir);
 
   if (IsEnabled()) {
-    DCHECK(!on_sets_ready_.is_null());
     sets_loader_->SetManuallySpecifiedSet(flag_value);
     if (!embedder_will_provide_public_sets_) {
       sets_loader_->SetComponentSets(base::File());
@@ -164,7 +166,7 @@ void FirstPartySetsHandlerImpl::ResetForTesting() {
                      base::Unretained(this)),
       IsEnabled() ? GetContentClient()->browser()->GetFirstPartySetsOverrides()
                   : base::Value::Dict());
-  on_sets_ready_.Reset();
+  on_sets_ready_callbacks_.clear();
   persisted_sets_path_ = base::FilePath();
   sets_ = absl::nullopt;
   raw_persisted_sets_ = absl::nullopt;
@@ -208,8 +210,7 @@ void FirstPartySetsHandlerImpl::OnReadPersistedSetsFile(
     ClearSiteDataOnChangedSets();
 
     if (IsEnabled()) {
-      DCHECK(!on_sets_ready_.is_null());
-      std::move(on_sets_ready_).Run(sets_.value());
+      InvokePendingQueries();
     }
   }
 }
@@ -224,12 +225,22 @@ void FirstPartySetsHandlerImpl::SetCompleteSets(
     ClearSiteDataOnChangedSets();
 
     if (IsEnabled()) {
-      DCHECK(!on_sets_ready_.is_null());
-      std::move(on_sets_ready_).Run(sets_.value());
+      InvokePendingQueries();
     }
   }
 }
 
+void FirstPartySetsHandlerImpl::InvokePendingQueries() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  while (!on_sets_ready_callbacks_.empty()) {
+    SetsReadyOnceCallback callback =
+        std::move(on_sets_ready_callbacks_.front());
+    on_sets_ready_callbacks_.pop_front();
+    std::move(callback).Run(sets_.value());
+  }
+  on_sets_ready_callbacks_.shrink_to_fit();
+}
+
 // static
 base::flat_set<net::SchemefulSite> FirstPartySetsHandlerImpl::ComputeSetsDiff(
     const base::flat_map<net::SchemefulSite, net::SchemefulSite>& old_sets,
@@ -281,9 +292,4 @@ void FirstPartySetsHandlerImpl::ClearSiteDataOnChangedSets() const {
   }
 }
 
-bool FirstPartySetsHandlerImpl::IsEnabledAndReady() const {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return IsEnabled() && sets_.has_value();
-}
-
 }  // namespace content

content/browser/first_party_sets/first_party_sets_handler_impl.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/callback.h"
+#include "base/containers/circular_deque.h"
 #include "base/containers/flat_map.h"
 #include "base/containers/flat_set.h"
 #include "base/files/file.h"
@@ -36,7 +37,7 @@ namespace content {
 class CONTENT_EXPORT FirstPartySetsHandlerImpl : public FirstPartySetsHandler {
  public:
   using FlattenedSets = base::flat_map<net::SchemefulSite, net::SchemefulSite>;
-  using SetsReadyOnceCallback = base::OnceCallback<void(const FlattenedSets&)>;
+  using SetsReadyOnceCallback = base::OnceCallback<void(FlattenedSets)>;
 
   static FirstPartySetsHandlerImpl* GetInstance();
 
@@ -47,24 +48,25 @@ class CONTENT_EXPORT FirstPartySetsHandlerImpl : public FirstPartySetsHandler {
       delete;
 
   // This method reads the persisted First-Party Sets from the file under
-  // `user_data_dir`, sets the First-Party Set that was provided via the
-  // flag/switch, and sets a callback that should eventually be invoked with the
-  // current First-Party Sets data.
+  // `user_data_dir` and sets the First-Party Set that was provided via the
+  // flag/switch.
   //
   // If First-Party Sets is disabled, then this method still needs to read the
   // persisted sets, since we may still need to clear data from a previous
   // invocation of Chromium which had First-Party Sets enabled.
   //
-  // If First-Party Sets is enabled, `on_sets_ready` must not be null.
-  //
   // Must be called exactly once.
-  void Init(const base::FilePath& user_data_dir,
-            const std::string& flag_value,
-            SetsReadyOnceCallback on_sets_ready);
+  void Init(const base::FilePath& user_data_dir, const std::string& flag_value);
 
-  // Returns the current First-Party Sets data, if the data is ready and the
-  // feature is enabled.
-  absl::optional<FlattenedSets> GetSetsIfEnabledAndReady() const;
+  // Returns the current First-Party Sets data. Returns the data synchronously
+  // via an optional if it's available, or via an asynchronously-invoked
+  // callback if the data is not ready yet.
+  //
+  // `callback` must not be null.
+  //
+  // Must not be called if First-Party Sets is disabled.
+  [[nodiscard]] absl::optional<FlattenedSets> GetSets(
+      SetsReadyOnceCallback callback);
 
   // FirstPartySetsHandler
   bool IsEnabled() const override;
@@ -110,6 +112,9 @@ class CONTENT_EXPORT FirstPartySetsHandlerImpl : public FirstPartySetsHandler {
   // Sets the current First-Party Sets data. Must be called exactly once.
   void SetCompleteSets(FlattenedSets sets);
 
+  // Invokes any pending queries.
+  void InvokePendingQueries();
+
   // Does the following:
   // 1) computes the diff between the `sets_` and the parsed
   // `raw_persisted_sets_`;
@@ -120,11 +125,6 @@ class CONTENT_EXPORT FirstPartySetsHandlerImpl : public FirstPartySetsHandler {
   // TODO(shuuran@chromium.org): Implement the code to clear site state.
   void ClearSiteDataOnChangedSets() const;
 
-  // Returns true if:
-  // * First-Party Sets are enabled;
-  // * `sets_` is ready to be used.
-  bool IsEnabledAndReady() const;
-
   // Whether Init has been called already or not.
   bool initialized_ = false;
 
@@ -149,7 +149,8 @@ class CONTENT_EXPORT FirstPartySetsHandlerImpl : public FirstPartySetsHandler {
 
   // We use a OnceCallback to ensure we only pass along the sets once
   // during Chrome's lifetime (modulo reconfiguring the network service).
-  SetsReadyOnceCallback on_sets_ready_ GUARDED_BY_CONTEXT(sequence_checker_);
+  base::circular_deque<SetsReadyOnceCallback> on_sets_ready_callbacks_
+      GUARDED_BY_CONTEXT(sequence_checker_);
 
   std::unique_ptr<FirstPartySetsLoader> sets_loader_
       GUARDED_BY_CONTEXT(sequence_checker_);

content/browser/first_party_sets/first_party_sets_handler_impl_unittest.cc

@@ -41,6 +41,13 @@ FirstPartySetsHandlerImpl::FlattenedSets ParseSetsFromStream(
   return FirstPartySetParser::ParseSetsFromStream(stream);
 }
 
+FirstPartySetsHandlerImpl::FlattenedSets GetSetsAndWait() {
+  base::test::TestFuture<FirstPartySetsHandlerImpl::FlattenedSets> future;
+  absl::optional<FirstPartySetsHandlerImpl::FlattenedSets> result =
+      FirstPartySetsHandlerImpl::GetInstance()->GetSets(future.GetCallback());
+  return result.has_value() ? result.value() : future.Get();
+}
+
 TEST(FirstPartySetsHandlerImpl, ComputeSetsDiff_SitesJoined) {
   FirstPartySetsHandlerImpl::FlattenedSets old_sets = {
       {net::SchemefulSite(GURL("https://example.test")),
@@ -359,13 +366,8 @@ class FirstPartySetsHandlerImplDisabledTest
 
 TEST_F(FirstPartySetsHandlerImplDisabledTest, IgnoresValid) {
   // Persisted sets are expected to be loaded with the provided path.
-  FirstPartySetsHandlerImpl::GetInstance()->Init(
-      scoped_dir_.GetPath(),
-      /*flag_value=*/"",
-      base::BindLambdaForTesting(
-          [](const FirstPartySetsHandlerImpl::FlattenedSets& got) {
-            FAIL();  // Should not be called.
-          }));
+  FirstPartySetsHandlerImpl::GetInstance()->Init(scoped_dir_.GetPath(),
+                                                 /*flag_value=*/"");
 
   env().RunUntilIdle();
 
@@ -377,23 +379,6 @@ TEST_F(FirstPartySetsHandlerImplDisabledTest, IgnoresValid) {
   EXPECT_EQ(got, "{}");
 }
 
-TEST_F(FirstPartySetsHandlerImplDisabledTest,
-       GetSetsIfEnabledAndReady_AfterSetsReady) {
-  ASSERT_TRUE(base::WriteFile(persisted_sets_path_, "{}"));
-
-  FirstPartySetsHandlerImpl::GetInstance()->Init(
-      scoped_dir_.GetPath(),
-      /*flag_value=*/"",
-      base::BindLambdaForTesting(
-          [](const FirstPartySetsHandlerImpl::FlattenedSets& got) {
-            FAIL();  // Should not be called.
-          }));
-
-  EXPECT_EQ(
-      FirstPartySetsHandlerImpl::GetInstance()->GetSetsIfEnabledAndReady(),
-      absl::nullopt);
-}
-
 class FirstPartySetsHandlerImplEnabledTest
     : public FirstPartySetsHandlerImplTest {
  public:
@@ -404,37 +389,17 @@ class FirstPartySetsHandlerImplEnabledTest
 TEST_F(FirstPartySetsHandlerImplEnabledTest, EmptyPersistedSetsDir) {
   // Empty `user_data_dir` will fail to load persisted sets, but that will not
   // prevent `on_sets_ready` from being invoked.
-  base::test::TestFuture<const FirstPartySetsHandlerImpl::FlattenedSets&>
-      future;
   FirstPartySetsHandlerImpl::GetInstance()->Init(
       /*user_data_dir=*/{},
-      /*flag_value=*/"https://example.test,https://member1.test",
-      future.GetCallback());
+      /*flag_value=*/"https://example.test,https://member1.test");
 
-  EXPECT_THAT(future.Get(),
+  EXPECT_THAT(GetSetsAndWait(),
               UnorderedElementsAre(Pair(SerializesTo("https://example.test"),
                                         SerializesTo("https://example.test")),
                                    Pair(SerializesTo("https://member1.test"),
                                         SerializesTo("https://example.test"))));
 }
 
-TEST_F(FirstPartySetsHandlerImplEnabledTest, PublicFirstPartySetsNotReady) {
-  FirstPartySetsHandlerImpl::GetInstance()
-      ->SetEmbedderWillProvidePublicSetsForTesting(true);
-  ASSERT_TRUE(base::WriteFile(persisted_sets_path_, "{}"));
-
-  // Persisted sets are expected to be loaded with the provided path.
-  FirstPartySetsHandlerImpl::GetInstance()->Init(
-      scoped_dir_.GetPath(),
-      /*flag_value=*/"https://example.test,https://member1.test",
-      base::BindLambdaForTesting(
-          [](const FirstPartySetsHandlerImpl::FlattenedSets& got) {
-            FAIL();  // Should not be called.
-          }));
-
-  env().RunUntilIdle();
-}
-
 TEST_F(FirstPartySetsHandlerImplEnabledTest,
        Successful_PersistedSetsFileNotExist) {
   FirstPartySetsHandlerImpl::GetInstance()
@@ -454,13 +419,10 @@ TEST_F(FirstPartySetsHandlerImplEnabledTest,
            SerializesTo("https://foo.test")));
 
   // Persisted sets are expected to be loaded with the provided path.
-  base::test::TestFuture<const FirstPartySetsHandlerImpl::FlattenedSets&>
-      future;
   FirstPartySetsHandlerImpl::GetInstance()->Init(
       scoped_dir_.GetPath(),
-      /*flag_value=*/"https://example.test,https://member1.test",
-      future.GetCallback());
-  EXPECT_THAT(future.Get(), expected_sets);
+      /*flag_value=*/"https://example.test,https://member1.test");
+  EXPECT_THAT(GetSetsAndWait(), expected_sets);
 
   env().RunUntilIdle();
 
@@ -490,13 +452,10 @@ TEST_F(FirstPartySetsHandlerImplEnabledTest, Successful_PersistedSetsEmpty) {
            SerializesTo("https://foo.test")));
 
   // Persisted sets are expected to be loaded with the provided path.
-  base::test::TestFuture<const FirstPartySetsHandlerImpl::FlattenedSets&>
-      future;
   FirstPartySetsHandlerImpl::GetInstance()->Init(
       scoped_dir_.GetPath(),
-      /*flag_value=*/"https://example.test,https://member1.test",
-      future.GetCallback());
-  EXPECT_THAT(future.Get(), expected_sets);
+      /*flag_value=*/"https://example.test,https://member1.test");
+  EXPECT_THAT(GetSetsAndWait(), expected_sets);
 
   env().RunUntilIdle();
 
@@ -524,12 +483,9 @@ TEST_F(FirstPartySetsHandlerImplEnabledTest,
                                 SerializesTo("https://example.test")));
 
   // Persisted sets are expected to be loaded with the provided path.
-  base::test::TestFuture<const FirstPartySetsHandlerImpl::FlattenedSets&>
-      future;
   FirstPartySetsHandlerImpl::GetInstance()->Init(scoped_dir_.GetPath(),
-                                                 /*flag_value=*/"",
-                                                 future.GetCallback());
-  EXPECT_THAT(future.Get(), expected_sets);
+                                                 /*flag_value=*/"");
+  EXPECT_THAT(GetSetsAndWait(), expected_sets);
 
   env().RunUntilIdle();
 
@@ -539,7 +495,9 @@ TEST_F(FirstPartySetsHandlerImplEnabledTest,
               expected_sets);
 
   EXPECT_THAT(
-      FirstPartySetsHandlerImpl::GetInstance()->GetSetsIfEnabledAndReady(),
+      FirstPartySetsHandlerImpl::GetInstance()->GetSets(
+          base::BindLambdaForTesting(
+              [](FirstPartySetsHandlerImpl::FlattenedSets) { FAIL(); })),
       testing::Optional(expected_sets));
 }
 
@@ -549,17 +507,15 @@ TEST_F(FirstPartySetsHandlerImplEnabledTest,
       ->SetEmbedderWillProvidePublicSetsForTesting(true);
   ASSERT_TRUE(base::WriteFile(persisted_sets_path_, "{}"));
 
-  // Call GetSetsIfEnabledAndReady before the sets are ready.
+  // Call GetSets before the sets are ready, and before Init has been called.
+  base::test::TestFuture<FirstPartySetsHandlerImpl::FlattenedSets> future;
   EXPECT_EQ(
-      FirstPartySetsHandlerImpl::GetInstance()->GetSetsIfEnabledAndReady(),
+      FirstPartySetsHandlerImpl::GetInstance()->GetSets(future.GetCallback()),
       absl::nullopt);
 
   // Persisted sets are expected to be loaded with the provided path.
-  base::test::TestFuture<const FirstPartySetsHandlerImpl::FlattenedSets&>
-      future;
   FirstPartySetsHandlerImpl::GetInstance()->Init(scoped_dir_.GetPath(),
-                                                 /*flag_value=*/"",
-                                                 future.GetCallback());
+                                                 /*flag_value=*/"");
 
   const std::string input = R"({"owner": "https://example.test", )"
                             R"("members": ["https://member.test"]})";
@@ -573,7 +529,9 @@ TEST_F(FirstPartySetsHandlerImplEnabledTest,
                                         SerializesTo("https://example.test"))));
 
   EXPECT_THAT(
-      FirstPartySetsHandlerImpl::GetInstance()->GetSetsIfEnabledAndReady(),
+      FirstPartySetsHandlerImpl::GetInstance()->GetSets(
+          base::BindLambdaForTesting(
+              [](FirstPartySetsHandlerImpl::FlattenedSets) { FAIL(); })),
       testing::Optional(
           UnorderedElementsAre(Pair(SerializesTo("https://example.test"),
                                     SerializesTo("https://example.test")),

content/browser/network_service_instance_impl.cc

@@ -582,11 +582,18 @@ network::mojom::NetworkService* GetNetworkService() {
         }
       }
 
-      if (absl::optional<FirstPartySetsHandlerImpl::FlattenedSets> sets =
-              FirstPartySetsHandlerImpl::GetInstance()
-                  ->GetSetsIfEnabledAndReady();
-          sets.has_value()) {
-        g_network_service_remote->get()->SetFirstPartySets(*sets);
+      if (FirstPartySetsHandlerImpl::GetInstance()->IsEnabled()) {
+        if (absl::optional<FirstPartySetsHandlerImpl::FlattenedSets> sets =
+                FirstPartySetsHandlerImpl::GetInstance()->GetSets(
+                    base::BindOnce(
+                        [](FirstPartySetsHandlerImpl::FlattenedSets sets) {
+                          GetNetworkService()->SetFirstPartySets(
+                              std::move(sets));
+                        }));
+            sets.has_value()) {
+          g_network_service_remote->get()->SetFirstPartySets(
+              std::move(sets.value()));
+        }
       }
 
       GetContentClient()->browser()->OnNetworkServiceCreated(