Implement read-only mode for Optimization Guide when in incognito

Bug: 1151087
Change-Id: I4a6074a01793c5a97de7f07e14ccbd133ef6cb51
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2586246Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836442}

chrome/browser/optimization_guide/hints_fetcher_browsertest.cc

@@ -1277,6 +1277,27 @@ IN_PROC_BROWSER_TEST_F(HintsFetcherBrowserTest,
             kRaceNavigationFetchHost,
         1);
   }
+
+  // Incognito page loads should not initiate any fetches.
+  {
+    base::HistogramTester incognito_histogram_tester;
+    // Instantiate off the record Optimization Guide Service.
+    OptimizationGuideKeyedServiceFactory::GetForProfile(
+        browser()->profile()->GetPrimaryOTRProfile())
+        ->RegisterOptimizationTypes({optimization_guide::proto::NOSCRIPT});
+
+    Browser* otr_browser = CreateIncognitoBrowser(browser()->profile());
+    ui_test_utils::NavigateToURL(otr_browser, GURL(full_url));
+
+    // Make sure no additional hints requests were received.
+    RetryForHistogramUntilCountReached(
+        &incognito_histogram_tester,
+        optimization_guide::kLoadedHintLocalHistogramString, 1);
+    EXPECT_EQ(2u, count_hints_requests_received());
+
+    incognito_histogram_tester.ExpectTotalCount(
+        "OptimizationGuide.HintsManager.RaceNavigationFetchAttemptStatus", 0);
+  }
 }
 
 // Test that the hints are fetched at the time of the navigation.

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -291,8 +291,6 @@ OptimizationGuideHintsManager::OptimizationGuideHintsManager(
               ExternalAppPackageNamesApprovedForFetch()),
       top_host_provider_(top_host_provider),
       clock_(base::DefaultClock::GetInstance()) {
-  DCHECK(optimization_guide_service_);
-
   RegisterOptimizationTypes(optimization_types_at_initialization);
 
   g_browser_process->network_quality_tracker()
@@ -306,23 +304,27 @@ OptimizationGuideHintsManager::OptimizationGuideHintsManager(
 
   NavigationPredictorKeyedService* navigation_predictor_service =
       NavigationPredictorKeyedServiceFactory::GetForProfile(profile_);
-  navigation_predictor_service->AddObserver(this);
+  if (navigation_predictor_service)
+    navigation_predictor_service->AddObserver(this);
 }
 
 OptimizationGuideHintsManager::~OptimizationGuideHintsManager() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  optimization_guide_service_->RemoveObserver(this);
+  if (optimization_guide_service_)
+    optimization_guide_service_->RemoveObserver(this);
   g_browser_process->network_quality_tracker()
       ->RemoveEffectiveConnectionTypeObserver(this);
 
   NavigationPredictorKeyedService* navigation_predictor_service =
       NavigationPredictorKeyedServiceFactory::GetForProfile(profile_);
-  navigation_predictor_service->RemoveObserver(this);
+  if (navigation_predictor_service)
+    navigation_predictor_service->RemoveObserver(this);
 }
 
 void OptimizationGuideHintsManager::Shutdown() {
-  optimization_guide_service_->RemoveObserver(this);
+  if (optimization_guide_service_)
+    optimization_guide_service_->RemoveObserver(this);
   g_browser_process->network_quality_tracker()
       ->RemoveEffectiveConnectionTypeObserver(this);
 }
@@ -535,7 +537,8 @@ void OptimizationGuideHintsManager::OnHintCacheInitialized() {
 
   // Register as an observer regardless of hint proto override usage. This is
   // needed as a signal during testing.
-  optimization_guide_service_->AddObserver(this);
+  if (optimization_guide_service_)
+    optimization_guide_service_->AddObserver(this);
 }
 
 void OptimizationGuideHintsManager::UpdateComponentHints(
@@ -1231,6 +1234,7 @@ bool OptimizationGuideHintsManager::IsAllowedToFetchNavigationHints(
 
   if (!IsUserPermittedToFetchFromRemoteOptimizationGuide(profile_))
     return false;
+  DCHECK(!profile_->IsOffTheRecord());
 
   if (!url.is_valid() || !url.SchemeIsHTTPOrHTTPS())
     return false;

chrome/browser/optimization_guide/optimization_guide_keyed_service.cc

@@ -10,6 +10,7 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/optional.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/optimization_guide/optimization_guide_hints_manager.h"
 #include "chrome/browser/optimization_guide/optimization_guide_navigation_data.h"
@@ -96,37 +97,51 @@ OptimizationGuideKeyedService::OptimizationGuideKeyedService(
     content::BrowserContext* browser_context)
     : browser_context_(browser_context) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(!browser_context_->IsOffTheRecord());
+  Initialize();
 }
 
 OptimizationGuideKeyedService::~OptimizationGuideKeyedService() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 }
 
-void OptimizationGuideKeyedService::Initialize(
-    optimization_guide::OptimizationGuideService* optimization_guide_service,
-    leveldb_proto::ProtoDatabaseProvider* database_provider,
-    const base::FilePath& profile_path) {
+void OptimizationGuideKeyedService::Initialize() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(optimization_guide_service);
 
   Profile* profile = Profile::FromBrowserContext(browser_context_);
-  top_host_provider_ = GetTopHostProviderIfUserPermitted(browser_context_);
-  bool optimization_guide_fetching_enabled = top_host_provider_ != nullptr;
-  UMA_HISTOGRAM_BOOLEAN("OptimizationGuide.RemoteFetchingEnabled",
-                        optimization_guide_fetching_enabled);
-  ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(
-      "SyntheticOptimizationGuideRemoteFetching",
-      optimization_guide_fetching_enabled ? "Enabled" : "Disabled");
-  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory =
-      content::BrowserContext::GetDefaultStoragePartition(profile)
-          ->GetURLLoaderFactoryForBrowserProcess();
+
+  // Regardless of whether the profile is off the record or not, we initialize
+  // the Optimization Guide with the database associated with the original
+  // profile.
+  auto* proto_db_provider = content::BrowserContext::GetDefaultStoragePartition(
+                                profile->GetOriginalProfile())
+                                ->GetProtoDatabaseProvider();
+  base::FilePath profile_path = profile->GetOriginalProfile()->GetPath();
+
+  // We should not be fetching anything from the remote Optimization Guide
+  // Service, only instantiate a URLLoaderFactory and TopHostProvider if the
+  // profile is a regular profile.
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory;
+  if (!profile->IsOffTheRecord()) {
+    url_loader_factory =
+        content::BrowserContext::GetDefaultStoragePartition(profile)
+            ->GetURLLoaderFactoryForBrowserProcess();
+
+    top_host_provider_ = GetTopHostProviderIfUserPermitted(browser_context_);
+    bool optimization_guide_fetching_enabled = top_host_provider_ != nullptr;
+    UMA_HISTOGRAM_BOOLEAN("OptimizationGuide.RemoteFetchingEnabled",
+                          optimization_guide_fetching_enabled);
+    ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(
+        "SyntheticOptimizationGuideRemoteFetching",
+        optimization_guide_fetching_enabled ? "Enabled" : "Disabled");
+  }
+
   hints_manager_ = std::make_unique<OptimizationGuideHintsManager>(
-      pre_initialized_optimization_types_, optimization_guide_service, profile,
-      profile_path, profile->GetPrefs(), database_provider,
-      top_host_provider_.get(), url_loader_factory);
+      pre_initialized_optimization_types_,
+      g_browser_process->optimization_guide_service(), profile, profile_path,
+      profile->GetPrefs(), proto_db_provider, top_host_provider_.get(),
+      url_loader_factory);
   prediction_manager_ = std::make_unique<optimization_guide::PredictionManager>(
-      pre_initialized_optimization_targets_, profile_path, database_provider,
+      pre_initialized_optimization_targets_, profile_path, proto_db_provider,
       top_host_provider_.get(), url_loader_factory, profile->GetPrefs(),
       profile);
 }

chrome/browser/optimization_guide/optimization_guide_keyed_service.h

@@ -25,24 +25,26 @@ class BrowserContext;
 class NavigationHandle;
 }  // namespace content
 
-namespace leveldb_proto {
-class ProtoDatabaseProvider;
-}  // namespace leveldb_proto
-
 namespace optimization_guide {
 namespace android {
 class OptimizationGuideBridge;
 }  // namespace android
-class OptimizationGuideService;
-class TopHostProvider;
 class PredictionManager;
 class PredictionManagerBrowserTestBase;
 class PredictionModelDownloadClient;
+class TopHostProvider;
 }  // namespace optimization_guide
 
 class GURL;
 class OptimizationGuideHintsManager;
 
+// Keyed service that can be used to get information received from the remote
+// Optimization Guide Service. For regular profiles, this will do the work to
+// fetch the necessary information from the remote Optimization Guide Service
+// in anticipation for when it is needed. For off the record profiles, this will
+// act in a "read-only" mode where it will only serve information that was
+// received from the remote Optimization Guide Service when not off the record
+// and no information will be retrieved.
 class OptimizationGuideKeyedService
     : public KeyedService,
       public optimization_guide::OptimizationGuideDecider {
@@ -107,18 +109,12 @@ class OptimizationGuideKeyedService
   friend class HintsFetcherBrowserTest;
   friend class OptimizationGuideKeyedServiceBrowserTest;
   friend class OptimizationGuideWebContentsObserver;
-  friend class ProfileManager;
   friend class optimization_guide::PredictionModelDownloadClient;
   friend class optimization_guide::PredictionManagerBrowserTestBase;
   friend class optimization_guide::android::OptimizationGuideBridge;
 
-  // Initializes the service. |optimization_guide_service| is the
-  // Optimization Guide Service that is being listened to and is guaranteed to
-  // outlive |this|. |profile_path| is the path to user data on disk.
-  void Initialize(
-      optimization_guide::OptimizationGuideService* optimization_guide_service,
-      leveldb_proto::ProtoDatabaseProvider* database_provider,
-      const base::FilePath& profile_path);
+  // Initializes |this|.
+  void Initialize();
 
   // Virtualized for testing.
   virtual OptimizationGuideHintsManager* GetHintsManager();

chrome/browser/optimization_guide/optimization_guide_keyed_service_browsertest.cc

@@ -618,6 +618,42 @@ IN_PROC_BROWSER_TEST_F(
 #endif
 }
 
+IN_PROC_BROWSER_TEST_F(
+    OptimizationGuideKeyedServiceDataSaverUserWithInfobarShownTest,
+    IncognitoCanStillReadFromComponentHints) {
+  // Instantiate off the record Optimization Guide Service.
+  OptimizationGuideKeyedServiceFactory::GetForProfile(
+      browser()->profile()->GetPrimaryOTRProfile())
+      ->RegisterOptimizationTypes({optimization_guide::proto::NOSCRIPT});
+
+  // Wait until initialization logic finishes running and component pushed to
+  // both incognito and regular browsers.
+  PushHintsComponentAndWaitForCompletion();
+  base::RunLoop().RunUntilIdle();
+
+  // Set up incognito browser and incognito OptimizationGuideKeyedService
+  // consumer.
+  Browser* otr_browser = CreateIncognitoBrowser(browser()->profile());
+  auto otr_consumer =
+      std::make_unique<OptimizationGuideConsumerWebContentsObserver>(
+          otr_browser->tab_strip_model()->GetActiveWebContents());
+  std::unique_ptr<base::RunLoop> run_loop = std::make_unique<base::RunLoop>();
+  otr_consumer->set_callback(base::BindOnce(
+      [](base::RunLoop* run_loop,
+         optimization_guide::OptimizationGuideDecision decision,
+         const optimization_guide::OptimizationMetadata& metadata) {
+        // Should still get decision in incognito.
+        EXPECT_EQ(decision,
+                  optimization_guide::OptimizationGuideDecision::kTrue);
+        run_loop->Quit();
+      },
+      run_loop.get()));
+
+  // Navigate to a URL that has a hint from a component.
+  ui_test_utils::NavigateToURL(otr_browser, url_with_hints());
+  run_loop->Run();
+}
+
 class OptimizationGuideKeyedServiceCommandLineOverridesTest
     : public OptimizationGuideKeyedServiceDataSaverUserWithInfobarShownTest {
  public:

chrome/browser/optimization_guide/optimization_guide_keyed_service_factory.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/optimization_guide/optimization_guide_keyed_service_factory.h"
 
 #include "chrome/browser/optimization_guide/optimization_guide_keyed_service.h"
+#include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/optimization_guide/optimization_guide_features.h"
@@ -39,3 +40,18 @@ KeyedService* OptimizationGuideKeyedServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   return new OptimizationGuideKeyedService(context);
 }
+
+content::BrowserContext*
+OptimizationGuideKeyedServiceFactory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  return chrome::GetBrowserContextOwnInstanceInIncognito(context);
+}
+
+bool OptimizationGuideKeyedServiceFactory::ServiceIsCreatedWithBrowserContext()
+    const {
+  return optimization_guide::features::IsOptimizationHintsEnabled();
+}
+
+bool OptimizationGuideKeyedServiceFactory::ServiceIsNULLWhileTesting() const {
+  return true;
+}

chrome/browser/optimization_guide/optimization_guide_keyed_service_factory.h

@@ -39,6 +39,10 @@ class OptimizationGuideKeyedServiceFactory
   // BrowserContextKeyedServiceFactory:
   KeyedService* BuildServiceInstanceFor(
       content::BrowserContext* context) const override;
+  content::BrowserContext* GetBrowserContextToUse(
+      content::BrowserContext* context) const override;
+  bool ServiceIsCreatedWithBrowserContext() const override;
+  bool ServiceIsNULLWhileTesting() const override;
 
   DISALLOW_COPY_AND_ASSIGN(OptimizationGuideKeyedServiceFactory);
 };

chrome/browser/optimization_guide/optimization_guide_permissions_util.cc

@@ -54,14 +54,14 @@ bool IsUserConsentedToAnonymousDataCollectionAndAllowedToFetchFromRemoteService(
 }  // namespace
 
 bool IsUserPermittedToFetchFromRemoteOptimizationGuide(Profile* profile) {
+  if (profile->IsOffTheRecord())
+    return false;
+
   if (optimization_guide::switches::
           ShouldOverrideCheckingUserPermissionsToFetchHintsForTesting()) {
     return true;
   }
 
-  if (profile->IsOffTheRecord())
-    return false;
-
   if (!optimization_guide::features::IsRemoteFetchingEnabled())
     return false;
 

chrome/browser/optimization_guide/optimization_guide_top_host_provider.cc

@@ -203,6 +203,11 @@ void OptimizationGuideTopHostProvider::MaybeUpdateTopHostBlacklist(
 
   Profile* profile = Profile::FromBrowserContext(
       navigation_handle->GetWebContents()->GetBrowserContext());
+
+  // Do not update the top host list if the profile is off the record.
+  if (profile->IsOffTheRecord())
+    return;
+
   PrefService* pref_service = profile->GetPrefs();
 
   bool is_user_permitted_to_fetch_hints =

chrome/browser/optimization_guide/prediction/prediction_manager.cc

@@ -180,6 +180,13 @@ void RecordModelTypeChanged(
       changed);
 }
 
+// Returns whether models and host model features should be fetched from the
+// remote Optimization Guide Service.
+bool ShouldFetchModelsAndHostModelFeatures(Profile* profile) {
+  return optimization_guide::features::IsRemoteFetchingEnabled() &&
+         !profile->IsOffTheRecord();
+}
+
 }  // namespace
 
 namespace optimization_guide {
@@ -610,7 +617,7 @@ void PredictionManager::SetPredictionModelDownloadManagerForTesting(
 void PredictionManager::FetchModelsAndHostModelFeatures() {
   SEQUENCE_CHECKER(sequence_checker_);
 
-  if (!features::IsRemoteFetchingEnabled())
+  if (!ShouldFetchModelsAndHostModelFeatures(profile_))
     return;
 
   ScheduleModelsAndHostModelFeaturesFetch();
@@ -1110,7 +1117,7 @@ bool PredictionManager::ProcessAndStoreHostModelFeatures(
 }
 
 void PredictionManager::MaybeScheduleModelAndHostModelFeaturesFetch() {
-  if (!features::IsRemoteFetchingEnabled())
+  if (!ShouldFetchModelsAndHostModelFeatures(profile_))
     return;
 
   if (optimization_guide::switches::

chrome/browser/optimization_guide/prediction/prediction_manager_browsertest.cc

@@ -645,6 +645,107 @@ IN_PROC_BROWSER_TEST_F(
       1);
 }
 
+IN_PROC_BROWSER_TEST_F(PredictionManagerBrowserTest,
+                       DISABLE_ON_WIN_MAC_CHROMEOS(IncognitoCanStillRead)) {
+  SetResponseType(PredictionModelsFetcherRemoteResponseType::
+                      kSuccessfulWithModelsAndFeatures);
+  base::HistogramTester histogram_tester;
+
+  // Register with regular profile.
+  RegisterWithKeyedService();
+
+  // Wait until model has been fetched via regular profile.
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionManager.HostModelFeaturesStored", 1);
+
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionManager.PredictionModelsStored", 1);
+
+  RetryForHistogramUntilCountReached(
+      &histogram_tester,
+      "OptimizationGuide.PredictionModelLoadedVersion.PainfulPageLoad", 1);
+
+  // Set up incognito browser.
+  Browser* otr_browser = CreateIncognitoBrowser(browser()->profile());
+
+  // Register with off the record profile.
+  OptimizationGuideKeyedServiceFactory::GetForProfile(
+      browser()->profile()->GetPrimaryOTRProfile())
+      ->RegisterOptimizationTargets(
+          {proto::OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD});
+  // Set up an OptimizationGuideKeyedService consumer.
+  auto otr_consumer =
+      std::make_unique<OptimizationGuideConsumerWebContentsObserver>(
+          otr_browser->tab_strip_model()->GetActiveWebContents());
+  std::unique_ptr<base::RunLoop> run_loop = std::make_unique<base::RunLoop>();
+  otr_consumer->set_callback(base::BindOnce(
+      [](base::RunLoop* run_loop,
+         optimization_guide::OptimizationGuideDecision decision) {
+        // We should have the model on the client so we have everything to make
+        // a decision.
+        EXPECT_NE(decision,
+                  optimization_guide::OptimizationGuideDecision::kUnknown);
+        run_loop->Quit();
+      },
+      run_loop.get()));
+
+  // Navigate to a URL with a host model feature in incognito.
+  ui_test_utils::NavigateToURL(otr_browser, https_url_with_content());
+  run_loop->Run();
+
+  // The store should still be able to be read.
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.PredictionManager.HasHostModelFeaturesForHost", true,
+      1);
+}
+
+IN_PROC_BROWSER_TEST_F(
+    PredictionManagerBrowserTest,
+    DISABLE_ON_WIN_MAC_CHROMEOS(IncognitoDoesntFetchModels)) {
+  SetResponseType(PredictionModelsFetcherRemoteResponseType::
+                      kSuccessfulWithModelsAndFeatures);
+  base::HistogramTester histogram_tester;
+
+  // Set up incognito browser.
+  Browser* otr_browser = CreateIncognitoBrowser(browser()->profile());
+
+  // Register with off the record profile.
+  OptimizationGuideKeyedServiceFactory::GetForProfile(
+      browser()->profile()->GetPrimaryOTRProfile())
+      ->RegisterOptimizationTargets(
+          {proto::OPTIMIZATION_TARGET_PAINFUL_PAGE_LOAD});
+  // Wait until logic finishes running.
+  base::RunLoop().RunUntilIdle();
+
+  // Ensure that GetModelsRequest did not go out.
+  histogram_tester.ExpectTotalCount(
+      "OptimizationGuide.PredictionModelFetcher.GetModelsRequest.HostCount", 0);
+
+  // Set up an OptimizationGuideKeyedService consumer.
+  auto otr_consumer =
+      std::make_unique<OptimizationGuideConsumerWebContentsObserver>(
+          otr_browser->tab_strip_model()->GetActiveWebContents());
+  std::unique_ptr<base::RunLoop> run_loop = std::make_unique<base::RunLoop>();
+  otr_consumer->set_callback(base::BindOnce(
+      [](base::RunLoop* run_loop,
+         optimization_guide::OptimizationGuideDecision decision) {
+        run_loop->Quit();
+      },
+      run_loop.get()));
+
+  // Navigate to a URL that would normally have a model had we not been in
+  // incognito.
+  ui_test_utils::NavigateToURL(otr_browser, https_url_with_content());
+  run_loop->Run();
+
+  // The model should not be available on the client.
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.TargetDecision.PainfulPageLoad",
+      OptimizationTargetDecision::kModelNotAvailableOnClient, 1);
+}
+
 class PredictionManagerBrowserSameOriginTest
     : public PredictionManagerBrowserTest {
  public:

chrome/browser/profiles/profile_manager.cc

@@ -48,8 +48,6 @@
 #include "chrome/browser/lite_video/lite_video_keyed_service.h"
 #include "chrome/browser/lite_video/lite_video_keyed_service_factory.h"
 #include "chrome/browser/navigation_predictor/navigation_predictor_keyed_service_factory.h"
-#include "chrome/browser/optimization_guide/optimization_guide_keyed_service.h"
-#include "chrome/browser/optimization_guide/optimization_guide_keyed_service_factory.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/permissions/adaptive_quiet_notification_permission_ui_enabler.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
@@ -1367,20 +1365,6 @@ void ProfileManager::DoFinalInitForServices(Profile* profile,
   DataReductionProxyChromeSettingsFactory::GetForBrowserContext(profile)->
       MaybeActivateDataReductionProxy(true);
 
-  auto* proto_db_provider =
-      content::BrowserContext::GetDefaultStoragePartition(profile)
-          ->GetProtoDatabaseProvider();
-
-  // Creates the Optimization Guide Keyed Service and begins loading the
-  // hint cache from persistent memory.
-  auto* optimization_guide_keyed_service =
-      OptimizationGuideKeyedServiceFactory::GetForProfile(profile);
-  if (optimization_guide_keyed_service) {
-    optimization_guide_keyed_service->Initialize(
-        g_browser_process->optimization_guide_service(), proto_db_provider,
-        profile->GetPath());
-  }
-
   // Create the Previews Service and begin loading opt out history from
   // persistent memory.
   PreviewsServiceFactory::GetForProfile(profile)->Initialize(