[floc] Skip setting floc_allowed in incognito / for null HistoryService

Incognito doesn't save history at all, so avoid calling
HistoryService::SetFlocAllowed in incognito. Also don't set it when the
history service is null. This aligns with the
pattern elsewhere (e.g. calling hs->UpdateWithPageEndTime inside
HistoryTabHelper::WebContentsDestroyed).

Besides, this is a speculative fix for the crash crbug.com/1166194, as
dereferencing a null history_service can trigger undefined behavior.

Bug: 1162874, 1166194
Change-Id: Icb3adbd4ce311d4153bfd2b08b2bd43c4b4aa726
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2628927
Commit-Queue: Yao Xiao <yaoxia@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843654}

chrome/browser/federated_learning/floc_eligibility_observer.cc

@@ -14,6 +14,22 @@
 
 namespace federated_learning {
 
+namespace {
+
+history::HistoryService* GetHistoryService(content::WebContents* web_contents) {
+  DCHECK(web_contents);
+
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  if (profile->IsOffTheRecord())
+    return nullptr;
+
+  return HistoryServiceFactory::GetForProfile(
+      profile, ServiceAccessType::IMPLICIT_ACCESS);
+}
+
+}  // namespace
+
 FlocEligibilityObserver::~FlocEligibilityObserver() = default;
 
 page_load_metrics::PageLoadMetricsObserver::ObservePolicy
@@ -48,30 +64,28 @@ FlocEligibilityObserver::OnCommit(
 }
 
 void FlocEligibilityObserver::OnAdResource() {
-  MaybeSetFlocAllowedInHistory();
+  OnOptInSignalObserved();
 }
 
 void FlocEligibilityObserver::OnInterestCohortApiUsed() {
-  MaybeSetFlocAllowedInHistory();
+  OnOptInSignalObserved();
 }
 
 FlocEligibilityObserver::FlocEligibilityObserver(content::RenderFrameHost* rfh)
     : web_contents_(content::WebContents::FromRenderFrameHost(rfh)) {}
 
-void FlocEligibilityObserver::MaybeSetFlocAllowedInHistory() {
-  if (!eligible_commit_ || did_set_floc_allowed_)
+void FlocEligibilityObserver::OnOptInSignalObserved() {
+  if (!eligible_commit_ || observed_opt_in_signal_)
     return;
 
-  history::HistoryService* hs = HistoryServiceFactory::GetForProfile(
-      Profile::FromBrowserContext(web_contents_->GetBrowserContext()),
-      ServiceAccessType::IMPLICIT_ACCESS);
-
+  if (history::HistoryService* hs = GetHistoryService(web_contents_)) {
     hs->SetFlocAllowed(
         history::ContextIDForWebContents(web_contents_),
         web_contents_->GetController().GetLastCommittedEntry()->GetUniqueID(),
         web_contents_->GetLastCommittedURL());
+  }
 
-  did_set_floc_allowed_ = true;
+  observed_opt_in_signal_ = true;
 }
 
 RENDER_DOCUMENT_HOST_USER_DATA_KEY_IMPL(FlocEligibilityObserver)

chrome/browser/federated_learning/floc_eligibility_observer.h

@@ -49,18 +49,18 @@ class FlocEligibilityObserver
 
   friend class content::RenderDocumentHostUserData<FlocEligibilityObserver>;
 
-  void MaybeSetFlocAllowedInHistory();
+  void OnOptInSignalObserved();
 
   content::WebContents* web_contents_;
 
   // |eligible_commit_| means all the commit time prerequisites are met
-  // (i.e. IP was publicly routable). It can only be set to true at commit time.
-  // When it's set, it also implies that the add-page-to-history decision has
-  // been made, i.e. either the page has been added to history, or has been
-  // skipped.
+  // (i.e. IP was publicly routable AND permissions policy is "allow"). It can
+  // only be set to true at commit time. When it's set, it also implies that the
+  // add-page-to-history decision has been made, i.e. either the page has been
+  // added to history, or has been skipped.
   bool eligible_commit_ = false;
 
-  bool did_set_floc_allowed_ = false;
+  bool observed_opt_in_signal_ = false;
 
   RENDER_DOCUMENT_HOST_USER_DATA_KEY_DECL();
 };

chrome/browser/federated_learning/floc_eligibility_unittest.cc

@@ -14,6 +14,7 @@
 #include "components/page_load_metrics/common/page_load_metrics.mojom.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/test/navigation_simulator.h"
+#include "content/public/test/web_contents_tester.h"
 
 namespace federated_learning {
 
@@ -26,13 +27,20 @@ class FlocEligibilityUnitTest : public ChromeRenderViewHostTestHarness {
     ChromeRenderViewHostTestHarness::SetUp();
     ASSERT_TRUE(profile()->CreateHistoryService());
 
+    InitWebContents();
+
     tester_ =
         std::make_unique<page_load_metrics::PageLoadMetricsObserverTester>(
-            web_contents(), this,
+            GetWebContents(), this,
             base::BindRepeating(&FlocEligibilityUnitTest::RegisterObservers,
                                 base::Unretained(this)));
   }
 
+  // Can be overridden in child class to initialize an incognito web_contents.
+  virtual void InitWebContents() {}
+
+  virtual content::WebContents* GetWebContents() { return web_contents(); }
+
   history::HistoryService* history_service() {
     return HistoryServiceFactory::GetForProfile(
         profile(), ServiceAccessType::EXPLICIT_ACCESS);
@@ -71,14 +79,14 @@ class FlocEligibilityUnitTest : public ChromeRenderViewHostTestHarness {
     resources.push_back(std::move(resource));
 
     tester_->SimulateResourceDataUseUpdate(resources,
-                                           web_contents()->GetMainFrame());
+                                           GetWebContents()->GetMainFrame());
   }
 
   void NavigateToPage(const GURL& url,
                       bool publicly_routable,
                       bool floc_feature_policy_enabled) {
     auto simulator = content::NavigationSimulator::CreateBrowserInitiated(
-        url, web_contents());
+        url, GetWebContents());
     simulator->SetTransition(ui::PageTransition::PAGE_TRANSITION_TYPED);
 
     if (!publicly_routable) {
@@ -98,8 +106,11 @@ class FlocEligibilityUnitTest : public ChromeRenderViewHostTestHarness {
 
     history_service()->AddPage(
         url, base::Time::Now(),
-        history::ContextIDForWebContents(web_contents()),
-        web_contents()->GetController().GetLastCommittedEntry()->GetUniqueID(),
+        history::ContextIDForWebContents(GetWebContents()),
+        GetWebContents()
+            ->GetController()
+            .GetLastCommittedEntry()
+            ->GetUniqueID(),
         /*referrer=*/GURL(),
         /*redirects=*/{}, ui::PageTransition::PAGE_TRANSITION_TYPED,
         history::VisitSource::SOURCE_BROWSED,
@@ -109,7 +120,7 @@ class FlocEligibilityUnitTest : public ChromeRenderViewHostTestHarness {
 
   FlocEligibilityObserver* GetFlocEligibilityObserver() {
     return FlocEligibilityObserver::GetOrCreateForCurrentDocument(
-        web_contents()->GetMainFrame());
+        GetWebContents()->GetMainFrame());
   }
 
  private:
@@ -184,4 +195,39 @@ TEST_F(FlocEligibilityUnitTest, StopObservingFlocFeaturePolicyDisabled) {
   EXPECT_FALSE(IsUrlVisitEligibleToComputeFloc(url));
 }
 
+class FlocEligibilityIncognitoUnitTest : public FlocEligibilityUnitTest {
+ public:
+  FlocEligibilityIncognitoUnitTest() = default;
+  ~FlocEligibilityIncognitoUnitTest() override = default;
+
+  void InitWebContents() override {
+    TestingProfile::Builder().BuildIncognito(profile());
+    incognito_web_contents_ = content::WebContentsTester::CreateTestWebContents(
+        profile()->GetPrimaryOTRProfile(),
+        content::SiteInstance::Create(profile()->GetPrimaryOTRProfile()));
+  }
+
+  content::WebContents* GetWebContents() override {
+    return incognito_web_contents_.get();
+  }
+
+  void TearDown() override {
+    incognito_web_contents_.reset();
+    FlocEligibilityUnitTest::TearDown();
+  }
+
+ private:
+  std::unique_ptr<content::WebContents> incognito_web_contents_;
+};
+
+TEST_F(FlocEligibilityIncognitoUnitTest, SkipSettingFlocAllowedInIncognito) {
+  GURL url("https://foo.com");
+  NavigateToPage(url, /*publicly_routable=*/true,
+                 /*floc_feature_policy_enabled=*/true);
+
+  SimulateResourceDataUseUpdate(/*is_ad_resource=*/true);
+
+  EXPECT_FALSE(IsUrlVisitEligibleToComputeFloc(url));
+}
+
 }  // namespace federated_learning