HostContentSettingsMap::GetSettingsForOneType: factor in inheritance behavior

This previously inherited settings from the base profile even if they
were configured not to, which e.g. made client hints in incognito be
improperly influenced by the normal profile state, as noticed
by Aaron Tagliaboschi (and originally started to fix in a different
way in https://chromium-review.googlesource.com/c/chromium/src/+/2034051)

This also appeared to cause some weirdness with important site computation (bug 993021) and with some device permissions.

Bug: 1052406, 993021
Change-Id: Ieeaaf0d508161568455ed077be820fd03831afdc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2047446
Commit-Queue: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Ovidio de Jesús Ruiz-Henríquez <odejesush@chromium.org>
Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Carlos IL <carlosil@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745488}

chrome/browser/bluetooth/bluetooth_chooser_context_unittest.cc

@@ -326,13 +326,12 @@ TEST_F(BluetoothChooserContextTest, GrantPermissionInIncognito) {
         incognito_context->GetGrantedObjects(foo_origin_, foo_origin_);
     EXPECT_EQ(1u, origin_objects.size());
 
-    // GetAllGrantedObjects() on an incognito session also returns the
-    // permission objects granted in the non-incognito session.
+    // GetAllGrantedObjects() on an incognito session only returns objects
+    // relevant to it.
     std::vector<std::unique_ptr<ChooserContextBase::Object>>
         all_origin_objects = incognito_context->GetAllGrantedObjects();
-    ASSERT_EQ(2u, all_origin_objects.size());
-    EXPECT_TRUE(all_origin_objects[0]->incognito ^
-                all_origin_objects[1]->incognito);
+    ASSERT_EQ(1u, all_origin_objects.size());
+    EXPECT_TRUE(all_origin_objects[0]->incognito);
   }
 }
 

chrome/browser/client_hints/client_hints_browsertest.cc

@@ -305,6 +305,8 @@ class ClientHintsBrowserTest : public InProcessBrowserTest,
     view->UpdateWebkitPreferences(prefs);
   }
 
+  void TestProfilesIndependent(Browser* browser_a, Browser* browser_b);
+
   const GURL& accept_ch_with_lifetime_http_local_url() const {
     return accept_ch_with_lifetime_http_local_url_;
   }
@@ -873,6 +875,44 @@ IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest,
   EXPECT_EQ(2u, third_party_client_hints_count_seen());
 }
 
+// Test that client hints are attached to subresources checks the right setting
+// for OTR profile.
+IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest,
+                       ClientHintsHttpsSubresourceOffTheRecord) {
+  const GURL gurl = GetParam() ? http_equiv_accept_ch_with_lifetime()
+                               : accept_ch_with_lifetime_url();
+
+  base::HistogramTester histogram_tester;
+
+  // Add client hints for the embedded test server.
+  ui_test_utils::NavigateToURL(browser(), gurl);
+  histogram_tester.ExpectUniqueSample("ClientHints.UpdateEventCount", 1, 1);
+
+  // Main profile should get hints for both page and subresources.
+  SetClientHintExpectationsOnMainFrame(true);
+  SetClientHintExpectationsOnSubresources(true);
+  ui_test_utils::NavigateToURL(
+      browser(), without_accept_ch_without_lifetime_img_localhost());
+  base::RunLoop().RunUntilIdle();
+  content::FetchHistogramsFromChildProcesses();
+  SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
+
+  // The user agent hint is attached to all three requests:
+  EXPECT_EQ(3u, count_user_agent_hint_headers_seen());
+  EXPECT_EQ(3u, count_ua_mobile_client_hints_headers_seen());
+
+  // Ten other client hints are attached to the image request, and ten to the
+  // main frame request.
+  EXPECT_EQ(20u, count_client_hints_headers_seen());
+
+  // OTR profile should get neither.
+  Browser* otr_browser = CreateIncognitoBrowser(browser()->profile());
+  SetClientHintExpectationsOnMainFrame(false);
+  SetClientHintExpectationsOnSubresources(false);
+  ui_test_utils::NavigateToURL(
+      otr_browser, without_accept_ch_without_lifetime_img_localhost());
+}
+
 // Verify that we send only major version information in the `Sec-CH-UA` header
 // by default, and full version information after an opt-in.
 IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest, UserAgentVersion) {
@@ -897,6 +937,50 @@ IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest, UserAgentVersion) {
                              base::CompareCase::SENSITIVE));
 }
 
+void ClientHintsBrowserTest::TestProfilesIndependent(Browser* browser_a,
+                                                     Browser* browser_b) {
+  const GURL gurl = GetParam() ? http_equiv_accept_ch_with_lifetime()
+                               : accept_ch_with_lifetime_url();
+
+  blink::UserAgentMetadata ua = ::GetUserAgentMetadata();
+
+  // Navigate |browser_a| to a page that opts-into the header: the value should
+  // end with the major version, and not contain the full version.
+  SetClientHintExpectationsOnMainFrame(false);
+  ui_test_utils::NavigateToURL(browser_a, gurl);
+  EXPECT_TRUE(base::EndsWith(main_frame_ua_observed(), ua.major_version,
+                             base::CompareCase::SENSITIVE));
+  EXPECT_EQ(std::string::npos, main_frame_ua_observed().find(ua.full_version));
+
+  // Try again on |browser_a|, the header should have an effect there.
+  SetClientHintExpectationsOnMainFrame(true);
+  ui_test_utils::NavigateToURL(browser_a, gurl);
+  EXPECT_TRUE(base::EndsWith(main_frame_ua_observed(), ua.full_version,
+                             base::CompareCase::SENSITIVE));
+
+  // Navigate on |browser_b|. That should still only have the major
+  // version.
+  SetClientHintExpectationsOnMainFrame(false);
+  ui_test_utils::NavigateToURL(browser_b, gurl);
+  EXPECT_TRUE(base::EndsWith(main_frame_ua_observed(), ua.major_version,
+                             base::CompareCase::SENSITIVE));
+  EXPECT_EQ(std::string::npos, main_frame_ua_observed().find(ua.full_version));
+}
+
+// Check that client hints attached to navigation inside OTR profiles
+// use the right settings, regular -> OTR direction.
+IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest, OffTheRecordIndependent) {
+  TestProfilesIndependent(browser(),
+                          CreateIncognitoBrowser(browser()->profile()));
+}
+
+// Check that client hints attached to navigation inside OTR profiles
+// use the right settings, OTR -> regular direction.
+IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest, OffTheRecordIndependent2) {
+  TestProfilesIndependent(CreateIncognitoBrowser(browser()->profile()),
+                          browser());
+}
+
 // Test that client hints are attached to third party subresources if
 // AllowClientHintsToThirdParty feature is enabled.
 IN_PROC_BROWSER_TEST_P(ClientHintsAllowThirdPartyBrowserTest,

chrome/browser/content_settings/host_content_settings_map_unittest.cc

@@ -32,6 +32,7 @@
 #include "components/content_settings/core/browser/user_modifiable_provider.h"
 #include "components/content_settings/core/browser/website_settings_info.h"
 #include "components/content_settings/core/browser/website_settings_registry.h"
+#include "components/content_settings/core/common/content_settings_utils.h"
 #include "components/content_settings/core/common/pref_names.h"
 #include "components/content_settings/core/test/content_settings_test_utils.h"
 #include "components/prefs/pref_service.h"
@@ -792,6 +793,18 @@ TEST_F(HostContentSettingsMapTest, IncognitoInheritInitialAllow) {
             otr_map->GetContentSetting(host, host, ContentSettingsType::COOKIES,
                                        std::string()));
 
+  // The inherited setting should be included in ContentSettingsForOneType
+  // table as well.
+  {
+    ContentSettingsForOneType otr_settings;
+    otr_map->GetSettingsForOneType(ContentSettingsType::COOKIES, std::string(),
+                                   &otr_settings);
+    ASSERT_EQ(1u, otr_settings.size());
+    EXPECT_FALSE(otr_settings[0].incognito);
+    EXPECT_EQ(CONTENT_SETTING_ALLOW, content_settings::ValueToContentSetting(
+                                         &otr_settings[0].setting_value));
+  }
+
   // Changing content settings on the main map should also affect the
   // incognito map.
   host_content_settings_map->SetContentSettingDefaultScope(
@@ -804,6 +817,21 @@ TEST_F(HostContentSettingsMapTest, IncognitoInheritInitialAllow) {
             otr_map->GetContentSetting(host, host, ContentSettingsType::COOKIES,
                                        std::string()));
 
+  // Inherited setting + default in ContentSettingsForOneType
+  {
+    ContentSettingsForOneType otr_settings;
+    otr_map->GetSettingsForOneType(ContentSettingsType::COOKIES, std::string(),
+                                   &otr_settings);
+    ASSERT_EQ(2u, otr_settings.size());
+    EXPECT_FALSE(otr_settings[0].incognito);
+    EXPECT_EQ(CONTENT_SETTING_SESSION_ONLY,
+              content_settings::ValueToContentSetting(
+                  &otr_settings[0].setting_value));
+    EXPECT_FALSE(otr_settings[1].incognito);
+    EXPECT_EQ(CONTENT_SETTING_ALLOW, content_settings::ValueToContentSetting(
+                                         &otr_settings[1].setting_value));
+  }
+
   host_content_settings_map->SetContentSettingDefaultScope(
       host, GURL(), ContentSettingsType::COOKIES, std::string(),
       CONTENT_SETTING_BLOCK);
@@ -825,6 +853,24 @@ TEST_F(HostContentSettingsMapTest, IncognitoInheritInitialAllow) {
   EXPECT_EQ(CONTENT_SETTING_ALLOW,
             otr_map->GetContentSetting(host, host, ContentSettingsType::COOKIES,
                                        std::string()));
+
+  // The GetSettingsForOneType includes incognito setting first, but also
+  // inherited ones.
+  {
+    ContentSettingsForOneType otr_settings;
+    otr_map->GetSettingsForOneType(ContentSettingsType::COOKIES, std::string(),
+                                   &otr_settings);
+    ASSERT_EQ(3u, otr_settings.size());
+    EXPECT_TRUE(otr_settings[0].incognito);
+    EXPECT_EQ(CONTENT_SETTING_ALLOW, content_settings::ValueToContentSetting(
+                                         &otr_settings[0].setting_value));
+    EXPECT_FALSE(otr_settings[1].incognito);
+    EXPECT_EQ(CONTENT_SETTING_BLOCK, content_settings::ValueToContentSetting(
+                                         &otr_settings[1].setting_value));
+    EXPECT_FALSE(otr_settings[2].incognito);
+    EXPECT_EQ(CONTENT_SETTING_ALLOW, content_settings::ValueToContentSetting(
+                                         &otr_settings[2].setting_value));
+  }
 }
 
 TEST_F(HostContentSettingsMapTest, IncognitoInheritPopups) {
@@ -896,6 +942,16 @@ TEST_F(HostContentSettingsMapTest, IncognitoPartialInheritPref) {
       otr_map->GetContentSetting(
           host, host, ContentSettingsType::MEDIASTREAM_MIC, std::string()));
 
+  {
+    ContentSettingsForOneType otr_settings;
+    otr_map->GetSettingsForOneType(ContentSettingsType::MEDIASTREAM_MIC,
+                                   std::string(), &otr_settings);
+    ASSERT_EQ(1u, otr_settings.size());
+    EXPECT_FALSE(otr_settings[0].incognito);
+    EXPECT_EQ(CONTENT_SETTING_ASK, content_settings::ValueToContentSetting(
+                                       &otr_settings[0].setting_value));
+  }
+
   // BLOCK should be inherited from the main map to the incognito map.
   host_content_settings_map->SetContentSettingDefaultScope(
       host, GURL(), ContentSettingsType::MEDIASTREAM_MIC, std::string(),
@@ -909,6 +965,22 @@ TEST_F(HostContentSettingsMapTest, IncognitoPartialInheritPref) {
       otr_map->GetContentSetting(
           host, host, ContentSettingsType::MEDIASTREAM_MIC, std::string()));
 
+  // GetSettingsForOneType should return preference followed by default, both inherited.
+  {
+    
+    ContentSettingsForOneType otr_settings;
+    otr_map->GetSettingsForOneType(ContentSettingsType::MEDIASTREAM_MIC,
+                                   std::string(), &otr_settings);
+    ASSERT_EQ(2u, otr_settings.size());
+    EXPECT_FALSE(otr_settings[0].incognito);
+    EXPECT_EQ(CONTENT_SETTING_BLOCK, content_settings::ValueToContentSetting(
+                                         &otr_settings[0].setting_value));
+
+    EXPECT_FALSE(otr_settings[1].incognito);
+    EXPECT_EQ(CONTENT_SETTING_ASK, content_settings::ValueToContentSetting(
+                                       &otr_settings[1].setting_value));
+  }
+
   // ALLOW should not be inherited from the main map to the incognito map (but
   // it still overwrites the BLOCK, hence incognito reverts to ASK).
   host_content_settings_map->SetContentSettingDefaultScope(
@@ -922,6 +994,22 @@ TEST_F(HostContentSettingsMapTest, IncognitoPartialInheritPref) {
       CONTENT_SETTING_ASK,
       otr_map->GetContentSetting(
           host, host, ContentSettingsType::MEDIASTREAM_MIC, std::string()));
+
+  // The inherited ALLOW gets turned back into ASK in GetSettingsForOneType, mirroring the 
+  // reverting to ASK behavior above.
+  {
+    ContentSettingsForOneType otr_settings;
+    otr_map->GetSettingsForOneType(ContentSettingsType::MEDIASTREAM_MIC,
+                                   std::string(), &otr_settings);
+    ASSERT_EQ(2u, otr_settings.size());
+    EXPECT_FALSE(otr_settings[0].incognito);
+    EXPECT_EQ(CONTENT_SETTING_ASK, content_settings::ValueToContentSetting(
+                                       &otr_settings[0].setting_value));
+
+    EXPECT_FALSE(otr_settings[1].incognito);
+    EXPECT_EQ(CONTENT_SETTING_ASK, content_settings::ValueToContentSetting(
+                                       &otr_settings[1].setting_value));
+  }
 }
 
 TEST_F(HostContentSettingsMapTest, IncognitoPartialInheritDefault) {
@@ -1017,6 +1105,13 @@ TEST_F(HostContentSettingsMapTest, IncognitoDontInheritSetting) {
   EXPECT_EQ(nullptr, otr_map->GetWebsiteSetting(
                          host, host, ContentSettingsType::USB_CHOOSER_DATA,
                          std::string(), nullptr));
+  {
+    ContentSettingsForOneType otr_settings;
+    otr_map->GetSettingsForOneType(ContentSettingsType::USB_CHOOSER_DATA,
+                                   std::string(), &otr_settings);
+    // Nothing gets inherited here, and there is no default.
+    ASSERT_EQ(0u, otr_settings.size());
+  }
 }
 
 TEST_F(HostContentSettingsMapTest, PrefExceptionsOperation) {

chrome/browser/engagement/important_sites_util_browsertest.cc

@@ -83,31 +83,15 @@ IN_PROC_BROWSER_TEST_F(ImportantSitesUtilBrowserTest,
               ::testing::ElementsAre(kNonDSEOrigin.host()));
 
   // Important site calculation in incognito mode used to crash in Android
-  // pre-O where notification channels are not yet used, see crbug.com/989890,
-  // and with that bug fixed, it is now just broken, see crbug.com/993021.
+  // pre-O where notification channels are not yet used, see crbug.com/989890.
   //
-  // In more detail, while the notification permission is not inherited when
-  // determining if the notification capability should be available to a
-  // website, it *is* inherited for the purposes of calculating whether the site
-  // is important.
-  //
-  // The unexpected consequence is, however, that because in incognito mode the
-  // notification permission is no longer technically auto-granted to the DSE
-  // origin, the importance calculating logic no longer blocklists it, which
-  // means that in incognito mode the DSE ends up being considered important.
-  // Because nothing useful in incognito mode is gated on the site being
-  // important, this really is just a nuisance that, however, led to a
-  // significant number of crashes.
-  //
-  // For now this test codifies the incorrect behavior just for the purposes of
-  // making sure we no longer crash.
+  // It also used to produce wrong results, since notification permission
+  // information got inherited incorrectly.
+  // See crbug.com/993021, crbug.com/1052406
   auto* incognito_profile = profile()->GetOffTheRecordProfile();
   ASSERT_TRUE(incognito_profile);
   ASSERT_TRUE(incognito_profile->IsOffTheRecord());
 
-  EXPECT_THAT(
-      GetImportantDomains(incognito_profile),
-      ::testing::UnorderedElementsAre(
-          ImportantSitesUtil::GetRegisterableDomainOrIP(kDSEOrigin.GetURL()),
-          kNonDSEOrigin.host()));
+  EXPECT_THAT(GetImportantDomains(profile()),
+              ::testing::ElementsAre(kNonDSEOrigin.host()));
 }

components/content_settings/core/browser/host_content_settings_map.cc

@@ -819,8 +819,21 @@ void HostContentSettingsMap::AddSettingsForOneType(
 
   while (rule_iterator->HasNext()) {
     content_settings::Rule rule = rule_iterator->Next();
+    base::Value value = std::move(rule.value);
+
+    // Normal rules applied to incognito profiles are subject to inheritance
+    // settings.
+    if (!incognito && is_off_the_record_) {
+      std::unique_ptr<base::Value> inherit_value =
+          ProcessIncognitoInheritanceBehavior(
+              content_type, base::Value::ToUniquePtrValue(std::move(value)));
+      if (inherit_value)
+        value = std::move(*inherit_value);
+      else
+        continue;
+    }
     settings->emplace_back(
-        rule.primary_pattern, rule.secondary_pattern, std::move(rule.value),
+        rule.primary_pattern, rule.secondary_pattern, std::move(value),
         kProviderNamesSourceMap[provider_type].provider_name, incognito);
   }
 }

components/content_settings/core/browser/host_content_settings_map.h

@@ -337,7 +337,7 @@ class HostContentSettingsMap : public content_settings::Observer,
   // provided by |provider|, into |settings|. If |incognito| is true, adds only
   // the content settings which are applicable to the incognito mode and differ
   // from the normal mode. Otherwise, adds the content settings for the normal
-  // mode.
+  // mode (applying inheritance rules if |is_off_the_record_|).
   void AddSettingsForOneType(
       const content_settings::ProviderInterface* provider,
       ProviderType provider_type,