Always log the sim-hash floc (pre- blocklist/sorting-lsh)

The raw sim-hash values are needed by the server to calculate the
sorting-lsh cutting points and/or blocklist. We should always log it
regardless of the final result after applying the blocklist or (the
upcoming) sorting-lsh.

Removed the NotifyFlocUpdated method, as we no longer need to notify
floc update event (which could be useful for the header. we can add it
back later if we want the header).

Bug: 1062736
Change-Id: I50bba6a1f1945c7b1c57dfc01724c8375da89d2b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2438483
Commit-Queue: Yao Xiao <yaoxia@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812683}

chrome/browser/federated_learning/floc_id_provider_browsertest.cc

@@ -337,7 +337,7 @@ IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,
 
   EXPECT_EQ(1u, GetHistoryUrls().size());
 
-  EXPECT_EQ(GetFlocId(), FlocId());
+  EXPECT_FALSE(GetFlocId().IsValid());
 
   InitializeBlocklist({});
 
@@ -367,12 +367,15 @@ IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,
 
   EXPECT_EQ(1u, GetHistoryUrls().size());
 
-  EXPECT_EQ(GetFlocId(), FlocId());
+  EXPECT_FALSE(GetFlocId().IsValid());
 
   InitializeBlocklist({});
 
-  // Expect that the FlocIdComputed user event is not recorded.
+  // Expect that the FlocIdComputed user event is not recorded, as we won't
+  // record the 1st event after browser/sync startup if there are permission
+  // errors. The floc is should also be invalid.
   ASSERT_EQ(0u, user_event_service()->GetRecordedUserEvents().size());
+  EXPECT_FALSE(GetFlocId().IsValid());
 }
 
 IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,
@@ -387,7 +390,7 @@ IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,
 
   EXPECT_EQ(1u, GetHistoryUrls().size());
 
-  EXPECT_EQ(GetFlocId(), FlocId());
+  EXPECT_FALSE(GetFlocId().IsValid());
 
   InitializeBlocklist({});
 
@@ -424,13 +427,16 @@ IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,
 
   EXPECT_EQ(1u, GetHistoryUrls().size());
 
-  EXPECT_EQ(GetFlocId(), FlocId());
+  EXPECT_FALSE(GetFlocId().IsValid());
 
   // Load a blocklist that would block the upcoming floc.
   InitializeBlocklist({FlocId::CreateFromHistory({test_host()}).ToUint64()});
 
-  // Expect that the FlocIdComputed user event is not recorded.
-  ASSERT_EQ(0u, user_event_service()->GetRecordedUserEvents().size());
+  // Expect that the FlocIdComputed user event is recorded.
+  ASSERT_EQ(1u, user_event_service()->GetRecordedUserEvents().size());
+
+  // Expect that the API call would reject.
+  EXPECT_EQ("rejected", InvokeInterestCohortJsApi(web_contents()));
 }
 
 IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,
@@ -445,13 +451,20 @@ IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,
 
   EXPECT_EQ(1u, GetHistoryUrls().size());
 
-  EXPECT_EQ(GetFlocId(), FlocId());
+  EXPECT_FALSE(GetFlocId().IsValid());
 
   // Load a blocklist that would block a floc different from the upcoming floc.
   InitializeBlocklist({FlocId::CreateFromHistory({"b.test"}).ToUint64()});
 
+  // Expect the current floc to have the expected value.
+  EXPECT_EQ(GetFlocId(), FlocId::CreateFromHistory({test_host()}));
+
   // Expect that the FlocIdComputed user event is recorded.
   ASSERT_EQ(1u, user_event_service()->GetRecordedUserEvents().size());
+
+  // Expect that the API call would return the expected floc.
+  EXPECT_EQ(FlocId::CreateFromHistory({test_host()}).ToString(),
+            InvokeInterestCohortJsApi(web_contents()));
 }
 
 IN_PROC_BROWSER_TEST_F(FlocIdProviderWithCustomizedServicesBrowserTest,

chrome/browser/federated_learning/floc_id_provider_impl.cc

@@ -81,7 +81,7 @@ std::string FlocIdProviderImpl::GetInterestCohortForJsApi(
 }
 
 void FlocIdProviderImpl::OnComputeFlocCompleted(ComputeFlocTrigger trigger,
-                                                FlocId floc_id) {
+                                                ComputeFlocResult result) {
   DCHECK(floc_computation_in_progress_);
   floc_computation_in_progress_ = false;
 
@@ -94,10 +94,8 @@ void FlocIdProviderImpl::OnComputeFlocCompleted(ComputeFlocTrigger trigger,
     return;
   }
 
-  if (floc_id_ != floc_id) {
-    floc_id_ = floc_id;
-    NotifyFlocUpdated(trigger);
-  }
+  LogFlocComputedEvent(trigger, result);
+  floc_id_ = result.final_hash;
 
   // Abandon the scheduled task if any, and schedule a new compute-floc task
   // that is |kFlocScheduledUpdateInterval| from now.
@@ -107,10 +105,20 @@ void FlocIdProviderImpl::OnComputeFlocCompleted(ComputeFlocTrigger trigger,
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
-void FlocIdProviderImpl::NotifyFlocUpdated(ComputeFlocTrigger trigger) {
+void FlocIdProviderImpl::LogFlocComputedEvent(ComputeFlocTrigger trigger,
+                                              const ComputeFlocResult& result) {
   if (!base::FeatureList::IsEnabled(features::kFlocIdComputedEventLogging))
     return;
 
+  // Don't log if it's the 1st computation and sim_hash is not computed. This
+  // is likely due to sync just gets enabled but some floc permission settings
+  // are disabled. We don't want to mess up with the initial user event
+  // messagings (and some sync integration tests would fail otherwise).
+  if (trigger == ComputeFlocTrigger::kBrowserStart &&
+      !result.sim_hash.IsValid()) {
+    return;
+  }
+
   auto specifics = std::make_unique<sync_pb::UserEventSpecifics>();
   specifics->set_event_time_usec(
       base::Time::Now().ToDeltaSinceWindowsEpoch().InMicroseconds());
@@ -136,8 +144,8 @@ void FlocIdProviderImpl::NotifyFlocUpdated(ComputeFlocTrigger trigger) {
 
   floc_id_computed_event->set_event_trigger(event_trigger);
 
-  if (floc_id_.IsValid())
-    floc_id_computed_event->set_floc_id(floc_id_.ToUint64());
+  if (result.sim_hash.IsValid())
+    floc_id_computed_event->set_floc_id(result.sim_hash.ToUint64());
 
   user_event_service_->RecordUserEvent(std::move(specifics));
 }
@@ -249,7 +257,7 @@ void FlocIdProviderImpl::OnCheckCanComputeFlocCompleted(
     ComputeFlocCompletedCallback callback,
     bool can_compute_floc) {
   if (!can_compute_floc) {
-    std::move(callback).Run(FlocId());
+    std::move(callback).Run(ComputeFlocResult());
     return;
   }
 
@@ -347,29 +355,28 @@ void FlocIdProviderImpl::OnGetRecentlyVisitedURLsCompleted(
         net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES));
   }
 
-  FlocId floc_id = domains.size() >= kMinHistoryDomainSizeToReportFlocId
-                       ? FlocId::CreateFromHistory(domains)
-                       : FlocId();
+  if (domains.size() < kMinHistoryDomainSizeToReportFlocId) {
+    std::move(callback).Run(ComputeFlocResult());
+    return;
+  }
 
-  ApplyAdditionalFiltering(std::move(callback), floc_id);
+  ApplyAdditionalFiltering(std::move(callback),
+                           FlocId::CreateFromHistory(domains));
 }
 
 void FlocIdProviderImpl::ApplyAdditionalFiltering(
     ComputeFlocCompletedCallback callback,
-    const FlocId& floc_id) {
-  if (!floc_id.IsValid()) {
-    std::move(callback).Run(floc_id);
-    return;
-  }
+    const FlocId& sim_hash) {
+  DCHECK(sim_hash.IsValid());
 
   if (base::FeatureList::IsEnabled(features::kFlocIdBlocklistFiltering) &&
       g_browser_process->floc_blocklist_service()->ShouldBlockFloc(
-          floc_id.ToUint64())) {
-    std::move(callback).Run(FlocId());
+          sim_hash.ToUint64())) {
+    std::move(callback).Run(ComputeFlocResult(sim_hash, FlocId()));
     return;
   }
 
-  std::move(callback).Run(floc_id);
+  std::move(callback).Run(ComputeFlocResult(sim_hash, sim_hash));
 }
 
 }  // namespace federated_learning

chrome/browser/federated_learning/floc_id_provider_impl.h

@@ -54,8 +54,26 @@ class FlocIdProviderImpl : public FlocIdProvider,
     kHistoryDelete,
   };
 
+  struct ComputeFlocResult {
+    ComputeFlocResult() = default;
+
+    ComputeFlocResult(const FlocId& sim_hash, const FlocId& final_hash)
+        : sim_hash(sim_hash), final_hash(final_hash) {}
+
+    // Sim-hash of the browsing history. This is the baseline value where the
+    // |final_hash| field should be derived from. We'll log this field for the
+    // server to calculate the sorting-lsh cutting points and/or the blocklist.
+    FlocId sim_hash;
+
+    // The floc to be exposed to JS API. It can be set to a value different from
+    // |sim_hash| if we use sorting-lsh based encoding, or can be invalid if the
+    // final value is blocked.
+    FlocId final_hash;
+  };
+
   using CanComputeFlocCallback = base::OnceCallback<void(bool)>;
-  using ComputeFlocCompletedCallback = base::OnceCallback<void(FlocId)>;
+  using ComputeFlocCompletedCallback =
+      base::OnceCallback<void(ComputeFlocResult)>;
   using GetRecentlyVisitedURLsCallback =
       history::HistoryService::QueryHistoryCallback;
 
@@ -76,8 +94,9 @@ class FlocIdProviderImpl : public FlocIdProvider,
  protected:
   // protected virtual for testing.
   virtual void OnComputeFlocCompleted(ComputeFlocTrigger trigger,
-                                      FlocId floc_id);
-  virtual void NotifyFlocUpdated(ComputeFlocTrigger trigger);
+                                      ComputeFlocResult result);
+  virtual void LogFlocComputedEvent(ComputeFlocTrigger trigger,
+                                    const ComputeFlocResult& result);
 
  private:
   friend class FlocIdProviderUnitTest;
@@ -123,9 +142,11 @@ class FlocIdProviderImpl : public FlocIdProvider,
   // Apply any additional filtering or transformation on a floc computed from
   // history. For example, invalidate it if it's in the blocklist.
   void ApplyAdditionalFiltering(ComputeFlocCompletedCallback callback,
-                                const FlocId& floc_id);
+                                const FlocId& sim_hash);
 
+  // The id to be exposed to the JS API.
   FlocId floc_id_;
+
   bool floc_computation_in_progress_ = false;
   bool first_floc_computation_triggered_ = false;