Access Context Auditing: Stop use of HostContentSettingsMap on shutdown

To determine whether records should be cleared on shutdown a reference
to the HostContentsSettingMap is retained for use in a TaskRunner.
This causes a race between the service shutdown in the UI thread, and
the audit database clearing session only records.

This CL changes behavior such that the content settings at the time of
shutdown are copied from the HostContentSettingsMap for local use in
the TaskRunner.

Bug: 1113376
Change-Id: I4c259a69dbc1eff9f1e85b48f05da886e5f63957
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2339662
Commit-Queue: Theodore Olsauskas-Warren <sauski@google.com>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796389}

chrome/browser/browsing_data/access_context_audit_database.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/browsing_data/access_context_audit_database.h"
 
 #include "base/logging.h"
+#include "net/cookies/cookie_util.h"
 #include "sql/database.h"
 #include "sql/meta_table.h"
 #include "sql/recovery.h"
@@ -72,6 +73,25 @@ bool DeleteNonPersistentCookies(sql::Database* db) {
   return db->Execute(remove.c_str());
 }
 
+bool IsContentSettingSessionOnly(
+    const GURL& url,
+    const ContentSettingsForOneType& content_settings) {
+  // ContentSettingsForOneType are in order of decreasing specificity, such
+  // that the first matching entry defines the effective content setting.
+  for (const auto& setting : content_settings) {
+    // A match is performed against both primary and secondary patterns. This
+    // aligns with the behavior in CookieSettingsBase::ShouldDeleteCookieOnExit,
+    // which is used by the cookie store.
+    if (setting.primary_pattern.Matches(url) &&
+        setting.secondary_pattern.Matches(url)) {
+      return setting.GetContentSetting() ==
+             ContentSetting::CONTENT_SETTING_SESSION_ONLY;
+    }
+  }
+  NOTREACHED();
+  return false;
+}
+
 }  // namespace
 
 AccessContextAuditDatabase::AccessRecord::AccessRecord(
@@ -339,51 +359,70 @@ void AccessContextAuditDatabase::RemoveAllRecordsForTimeRange(base::Time begin,
 }
 
 void AccessContextAuditDatabase::RemoveSessionOnlyRecords(
-    scoped_refptr<content_settings::CookieSettings> cookie_settings,
     const ContentSettingsForOneType& content_settings) {
+  // ContentSettingsForOneType is a list of settings in decreasing specificity
+  // for origins, ending with a setting that matches all and is the default.
+  DCHECK(content_settings.size());
+  if (content_settings.size() == 1) {
+    DCHECK_EQ(content_settings[0].primary_pattern,
+              ContentSettingsPattern::Wildcard());
+    DCHECK_EQ(content_settings[0].secondary_pattern,
+              ContentSettingsPattern::Wildcard());
+    if (content_settings[0].GetContentSetting() ==
+        ContentSetting::CONTENT_SETTING_SESSION_ONLY) {
+      RemoveAllRecords();
+    }
+    // As only the default content setting is set, there is no need to inspect
+    // any individual records.
+    return;
+  }
+
   sql::Transaction transaction(&db_);
   if (!transaction.Begin())
     return;
 
-  // Extract the set of all domains from the cookies table.
+  // Extract the set of all domains from the cookies table, determine the
+  // effective content setting, and store for removal if appropriate.
   std::string select = "SELECT DISTINCT domain FROM ";
   select.append(kCookieTableName);
   sql::Statement select_cookie_domains(
       db_.GetCachedStatement(SQL_FROM_HERE, select.c_str()));
 
-  std::vector<std::string> cookie_domains;
+  std::vector<std::string> cookie_domains_for_removal;
   while (select_cookie_domains.Step()) {
-    cookie_domains.emplace_back(select_cookie_domains.ColumnString(0));
+    auto domain = select_cookie_domains.ColumnString(0);
+    GURL url = net::cookie_util::CookieOriginToURL(domain,
+                                                   /* is_https */ false);
+    GURL secure_url = net::cookie_util::CookieOriginToURL(domain,
+                                                          /* is_https */ true);
+    if (IsContentSettingSessionOnly(url, content_settings) ||
+        IsContentSettingSessionOnly(secure_url, content_settings)) {
+      cookie_domains_for_removal.emplace_back(std::move(domain));
+    }
   }
 
-  // Extract the set of all origins from the storage API table.
+  // Repeat the above, but for the origin keyed storage API table.
   select = "SELECT DISTINCT origin FROM ";
   select.append(kStorageAPITableName);
   sql::Statement select_storage_origins(
       db_.GetCachedStatement(SQL_FROM_HERE, select.c_str()));
 
-  std::vector<url::Origin> storage_origins;
+  std::vector<std::string> storage_origins_for_removal;
   while (select_storage_origins.Step()) {
-    storage_origins.emplace_back(
-        url::Origin::Create(GURL(select_storage_origins.ColumnString(0))));
+    auto origin = select_storage_origins.ColumnString(0);
+    if (IsContentSettingSessionOnly(GURL(origin), content_settings))
+      storage_origins_for_removal.emplace_back(origin);
   }
 
-  // Remove records for all cookie domains and storage origins for which the
-  // provided settings indicate should be cleared on exit.
+  // Remove entries belonging to cookie domains and origins identified as having
+  // a SESSION_ONLY content setting.
   std::string remove = "DELETE FROM ";
   remove.append(kCookieTableName);
   remove.append(" WHERE domain = ?");
   sql::Statement remove_cookies(
       db_.GetCachedStatement(SQL_FROM_HERE, remove.c_str()));
 
-  for (const auto& domain : cookie_domains) {
-    if (!cookie_settings->ShouldDeleteCookieOnExit(content_settings, domain,
-                                                   true) &&
-        !cookie_settings->ShouldDeleteCookieOnExit(content_settings, domain,
-                                                   false)) {
-      continue;
-    }
-
+  for (const auto& domain : cookie_domains_for_removal) {
     remove_cookies.BindString(0, domain);
     if (!remove_cookies.Run())
       return;
@@ -396,13 +435,8 @@ void AccessContextAuditDatabase::RemoveSessionOnlyRecords(
   sql::Statement remove_storage_apis(
       db_.GetCachedStatement(SQL_FROM_HERE, remove.c_str()));
 
-  for (const auto& origin : storage_origins) {
-    // TODO(crbug.com/1099164): Rename IsCookieSessionOnly to better convey
-    //                          its actual functionality.
-    if (!cookie_settings->IsCookieSessionOnly(origin.GetURL()))
-      continue;
-
-    remove_storage_apis.BindString(0, origin.Serialize());
+  for (const auto& origin : storage_origins_for_removal) {
+    remove_storage_apis.BindString(0, origin);
     if (!remove_storage_apis.Run())
       return;
     remove_storage_apis.Reset(true);

chrome/browser/browsing_data/access_context_audit_database.h

@@ -104,10 +104,10 @@ class AccessContextAuditDatabase
   void RemoveAllRecordsForTopFrameOrigins(
       const std::vector<url::Origin>& origins);
 
-  // Removes all records for cookie domains and API origins that match session
-  // only entries in |settings|
+  // Removes all records for which the result of inspecting |content_settings|
+  // for the storage origin or cookie domain is a content setting of
+  // CLEAR_ON_EXIT.
   void RemoveSessionOnlyRecords(
-      scoped_refptr<content_settings::CookieSettings> cookie_settings,
       const ContentSettingsForOneType& content_settings);
 
  protected:

chrome/browser/browsing_data/access_context_audit_database_unittest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/browsing_data/access_context_audit_database.h"
 
 #include "base/files/scoped_temp_dir.h"
+#include "components/content_settings/core/common/content_settings_utils.h"
 #include "sql/database.h"
 #include "sql/test/scoped_error_expecter.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -459,3 +460,63 @@ TEST_F(AccessContextAuditDatabaseTest, RepeatedAccesses) {
   EXPECT_EQ(num_test_cookie_entries, cookie_rows);
   EXPECT_EQ(num_test_storage_entries, storage_api_rows);
 }
+
+TEST_F(AccessContextAuditDatabaseTest, RemoveSessionOnlyRecords) {
+  // Check that records are cleared appropriately by RemoveSessionOnlyRecords
+  // when the provided content settings indicate they should.
+  auto test_records = GetTestRecords();
+  OpenDatabase();
+  database()->AddRecords(test_records);
+  ValidateDatabaseRecords(database(), test_records);
+
+  // Test that default content setting of SESSION_ONLY results in all records
+  // being removed.
+  ContentSettingsForOneType content_settings;
+  content_settings.emplace_back(
+      ContentSettingsPattern::Wildcard(), ContentSettingsPattern::Wildcard(),
+      base::Value::FromUniquePtrValue(content_settings::ContentSettingToValue(
+          CONTENT_SETTING_SESSION_ONLY)),
+      std::string(), /* incognito */ false);
+
+  database()->RemoveSessionOnlyRecords(content_settings);
+  ValidateDatabaseRecords(database(), {});
+
+  // Check that a more targeted content setting also removes the appropriate
+  // records.
+  content_settings.clear();
+  database()->AddRecords(test_records);
+  ValidateDatabaseRecords(database(), test_records);
+
+  content_settings.emplace_back(
+      ContentSettingsPattern::FromString(kManyContextsCookieDomain),
+      ContentSettingsPattern::Wildcard(),
+      base::Value::FromUniquePtrValue(content_settings::ContentSettingToValue(
+          CONTENT_SETTING_SESSION_ONLY)),
+      std::string(), /* incognito */ false);
+  content_settings.emplace_back(
+      ContentSettingsPattern::FromString(kManyContextsStorageAPIOrigin),
+      ContentSettingsPattern::Wildcard(),
+      base::Value::FromUniquePtrValue(content_settings::ContentSettingToValue(
+          CONTENT_SETTING_SESSION_ONLY)),
+      std::string(), /* incognito */ false);
+  content_settings.emplace_back(
+      ContentSettingsPattern::Wildcard(), ContentSettingsPattern::Wildcard(),
+      base::Value::FromUniquePtrValue(
+          content_settings::ContentSettingToValue(CONTENT_SETTING_ALLOW)),
+      std::string(), /* incognito */ false);
+  database()->RemoveSessionOnlyRecords(content_settings);
+
+  test_records.erase(
+      std::remove_if(
+          test_records.begin(), test_records.end(),
+          [=](const AccessContextAuditDatabase::AccessRecord& record) {
+            if (record.type ==
+                AccessContextAuditDatabase::StorageAPIType::kCookie) {
+              return record.domain == kManyContextsCookieDomain;
+            }
+            return record.origin ==
+                   url::Origin::Create(GURL(kManyContextsStorageAPIOrigin));
+          }),
+      test_records.end());
+  ValidateDatabaseRecords(database(), test_records);
+}

chrome/browser/browsing_data/access_context_audit_service.cc

@@ -192,6 +192,5 @@ void AccessContextAuditService::ClearSessionOnlyRecords() {
   database_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&AccessContextAuditDatabase::RemoveSessionOnlyRecords,
-                     database_, CookieSettingsFactory::GetForProfile(profile_),
-                     std::move(settings)));
+                     database_, std::move(settings)));
 }

chrome/browser/browsing_data/access_context_audit_service_unittest.cc

@@ -448,12 +448,7 @@ TEST_F(AccessContextAuditServiceTest, TimeRangeHistoryDeletion) {
   CheckContainsStorageAPIRecord(kOrigin1, kTestStorageType2, kOrigin1, records);
 }
 
-#if defined(THREAD_SANITIZER)
-#define MAYBE_SessionOnlyRecords DISABLED_SessionOnlyRecords
-#else
-#define MAYBE_SessionOnlyRecords SessionOnlyRecords
-#endif
-TEST_F(AccessContextAuditServiceTest, MAYBE_SessionOnlyRecords) {
+TEST_F(AccessContextAuditServiceTest, SessionOnlyRecords) {
   // Check that data for cookie domains and storage origins are cleared on
   // service shutdown when the associated content settings indicate they should.
   const GURL kTestPersistentURL("https://persistent.com");
@@ -516,4 +511,13 @@ TEST_F(AccessContextAuditServiceTest, MAYBE_SessionOnlyRecords) {
                             kTopFrameOrigin, records);
   CheckContainsStorageAPIRecord(url::Origin::Create(GURL(kTestPersistentURL)),
                                 kTestStorageType, kTopFrameOrigin, records);
+
+  // Update the default content setting to SESSION_ONLY and ensure that all
+  // records are cleared.
+  HostContentSettingsMapFactory::GetForProfile(profile())
+      ->SetDefaultContentSetting(ContentSettingsType::COOKIES,
+                                 ContentSetting::CONTENT_SETTING_SESSION_ONLY);
+  service()->ClearSessionOnlyRecords();
+  records = GetAllAccessRecords();
+  ASSERT_EQ(0u, records.size());
 }