Access Context Auditing: Only write cookie accesses in bulk

Improve performance of recording cookie accesses by only writing the
existing in memory cookie store to the database on top level frame
origin change, or on record access.

Bug: 1136016
Change-Id: I97159f75d4afaea004da00b3a37988816a972f88
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2450281
Commit-Queue: Theodore Olsauskas-Warren <sauski@google.com>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814641}

chrome/browser/browsing_data/access_context_audit_browsertest.cc

@@ -498,6 +498,51 @@ IN_PROC_BROWSER_TEST_F(AccessContextAuditBrowserTest, MultipleAccesses) {
     EXPECT_EQ(record.last_access_time, clock.Now());
 }
 
+IN_PROC_BROWSER_TEST_F(AccessContextAuditBrowserTest, TabClosed) {
+  // Ensure closing a tab correctly flushes access records.
+  NavigateToTopLevelPage();
+  NavigateToEmbeddedPage();
+
+  // Close the previous tab, but keep the browser active to ensure the profile
+  // does not begin destruction.
+  AddTabAtIndex(1, GURL("about:blank"), ui::PAGE_TRANSITION_TYPED);
+  browser()->tab_strip_model()->CloseWebContentsAt(0,
+                                                   TabStripModel::CLOSE_NONE);
+
+  auto records = GetAllAccessRecords();
+  auto cookies = GetAllCookies();
+  unsigned expected_cookie_records =
+      2 * kEmbeddedPageCookieCount + kTopLevelPageCookieCount;
+  unsigned expected_origin_storage_records = 3 * kOriginStorageTypes.size();
+  EXPECT_EQ(records.size(),
+            expected_cookie_records + expected_origin_storage_records);
+
+  CheckContainsCookieAndRecord(cookies, records, top_level_origin(), "embedder",
+                               kTopLevelHost, "/",
+                               /*compare_host_only*/ true);
+  CheckContainsCookieAndRecord(cookies, records, top_level_origin(),
+                               "session_only", kEmbeddedHost, "/",
+                               /*compare_host_only*/ true);
+  CheckContainsCookieAndRecord(cookies, records, top_level_origin(),
+                               "persistent", kEmbeddedHost, "/",
+                               /*compare_host_only*/ true);
+  CheckContainsCookieAndRecord(cookies, records, embedded_origin(),
+                               "persistent", kEmbeddedHost, "/",
+                               /*compare_host_only*/ true);
+  CheckContainsCookieAndRecord(cookies, records, embedded_origin(),
+                               "session_only", kEmbeddedHost, "/",
+                               /*compare_host_only*/ true);
+  CheckContainsOriginStorageRecords(records, kOriginStorageTypes,
+                                    top_level_origin(), top_level_origin(),
+                                    /* compare__host_only */ true);
+  CheckContainsOriginStorageRecords(records, kOriginStorageTypes,
+                                    embedded_origin(), top_level_origin(),
+                                    /* compare__host_only */ true);
+  CheckContainsOriginStorageRecords(records, kOriginStorageTypes,
+                                    embedded_origin(), embedded_origin(),
+                                    /* compare_host_only */ true);
+}
+
 class AccessContextAuditSessionRestoreBrowserTest
     : public AccessContextAuditBrowserTest {
  public:

chrome/browser/browsing_data/access_context_audit_service.cc

@@ -22,29 +22,34 @@ AccessContextAuditService::CookieAccessHelper::CookieAccessHelper(
   DCHECK(service);
   deletion_observer_.Add(service);
 }
-AccessContextAuditService::CookieAccessHelper::~CookieAccessHelper() = default;
+
+AccessContextAuditService::CookieAccessHelper::~CookieAccessHelper() {
+  FlushCookieRecords();
+}
 
 void AccessContextAuditService::CookieAccessHelper::OnCookieDeleted(
     const net::CanonicalCookie& cookie) {
-  seen_cookies_.erase(cookie);
+  accessed_cookies_.erase(cookie);
 }
 
 void AccessContextAuditService::CookieAccessHelper::RecordCookieAccess(
     const net::CookieList& accessed_cookies,
     const url::Origin& top_frame_origin) {
-  net::CookieList new_cookies;
-  for (const auto& cookie : accessed_cookies) {
-    if (!seen_cookies_.count(cookie)) {
-      new_cookies.push_back(cookie);
-      seen_cookies_.insert(cookie);
-    }
+  if (top_frame_origin != last_seen_top_frame_origin_) {
+    FlushCookieRecords();
+    last_seen_top_frame_origin_ = top_frame_origin;
   }
-  if (!new_cookies.empty())
-    service_->RecordCookieAccess(new_cookies, top_frame_origin);
+
+  for (const auto& cookie : accessed_cookies)
+    accessed_cookies_.insert(cookie);
 }
 
-void AccessContextAuditService::CookieAccessHelper::ClearSeenCookies() {
-  seen_cookies_.clear();
+void AccessContextAuditService::CookieAccessHelper::FlushCookieRecords() {
+  if (accessed_cookies_.empty())
+    return;
+
+  service_->RecordCookieAccess(accessed_cookies_, last_seen_top_frame_origin_);
+  accessed_cookies_.clear();
 }
 
 AccessContextAuditService::AccessContextAuditService(Profile* profile)
@@ -84,7 +89,7 @@ bool AccessContextAuditService::Init(
 }
 
 void AccessContextAuditService::RecordCookieAccess(
-    const net::CookieList& accessed_cookies,
+    const canonical_cookie::CookieHashSet& accessed_cookies,
     const url::Origin& top_frame_origin) {
   // Opaque top frame origins are not supported.
   if (top_frame_origin.opaque())
@@ -129,6 +134,9 @@ void AccessContextAuditService::GetAllAccessRecords(
   if (!user_visible_tasks_in_progress++)
     database_task_runner_->UpdatePriority(base::TaskPriority::USER_VISIBLE);
 
+  for (auto& helper : cookie_access_helpers_)
+    helper.FlushCookieRecords();
+
   database_task_runner_->PostTaskAndReplyWithResult(
       FROM_HERE,
       base::BindOnce(&AccessContextAuditDatabase::GetAllRecords, database_),
@@ -160,6 +168,7 @@ void AccessContextAuditService::RemoveAllRecordsForOriginKeyedStorage(
 }
 
 void AccessContextAuditService::Shutdown() {
+  DCHECK(!cookie_access_helpers_.might_have_observers());
   ClearSessionOnlyRecords();
 }
 

chrome/browser/browsing_data/access_context_audit_service.h

@@ -33,34 +33,37 @@ class AccessContextAuditService
   void RemoveObserver(CookieAccessHelper* helper);
 
   // A helper class used to report cookie accesses to the audit service. Keeps
-  // an internal record of cookie accesses which have already been seen.
-  // Repeated calls to RecordCookieAccess are ignored until the cookie is
-  // observed as deleted, or the set of seen cookies is cleared via
-  // ClearSeenCookies.
+  // an in-memory set of cookie accesses which are flushed to the audit service
+  // when a different top_frame_origin is provided, or when the helper is
+  // destroyed. Helpers should not outlive the audit service, this is DCHECK
+  // enforced on audit service shutdown.
   class CookieAccessHelper : public base::CheckedObserver {
    public:
     explicit CookieAccessHelper(AccessContextAuditService* service);
     ~CookieAccessHelper() override;
 
-    // Selectively forwards cookie accesses to the audit service based on
-    // whether this helper has previously seen the cookie.
+    // Adds the list of |accessed_cookies| to the in memory set of accessed
+    // cookies. If |top_frame_origin| has a different value than previously
+    // provided to this function, then first the set of accessed cookies is
+    // flushed to the database and cleared.
     void RecordCookieAccess(const net::CookieList& accessed_cookies,
                             const url::Origin& top_frame_origin);
 
     // Observer method called by the audit service when a cookie has been
-    // deleted and future accesses should be reported.
+    // deleted and should be removed from the in-memory set of accessed cookies.
     void OnCookieDeleted(const net::CanonicalCookie& cookie);
 
-    // Resets the internal set of seen cookies, resulting in future reported
-    // accesses to those cookies being forwarded to the service for recording.
-    // This should be called at least prior to every top-frame navigation,
-    // calling more frequently increases accuracy of access timestamps but also
-    // increases performance overhead.
-    void ClearSeenCookies();
-
    private:
+    friend class AccessContextAuditService;
+    FRIEND_TEST_ALL_PREFIXES(AccessContextAuditServiceTest, CookieAccessHelper);
+
+    // Clear the in-memory set of accessed cookies after passing them to the
+    // audit service for persisting to disk.
+    void FlushCookieRecords();
+
     AccessContextAuditService* service_;
-    canonical_cookie::CookieHashSet seen_cookies_;
+    canonical_cookie::CookieHashSet accessed_cookies_;
+    url::Origin last_seen_top_frame_origin_;
     ScopedObserver<AccessContextAuditService, CookieAccessHelper>
         deletion_observer_{this};
   };
@@ -134,8 +137,9 @@ class AccessContextAuditService
 
   // Records accesses for all cookies in |details| against |top_frame_origin|.
   // Should only be accessed via the CookieAccessHelper.
-  void RecordCookieAccess(const net::CookieList& accessed_cookies,
-                          const url::Origin& top_frame_origin);
+  void RecordCookieAccess(
+      const canonical_cookie::CookieHashSet& accessed_cookies,
+      const url::Origin& top_frame_origin);
 
   // Removes any records which are session only from the database.
   void ClearSessionOnlyRecords();

chrome/browser/browsing_data/access_context_audit_service_unittest.cc

@@ -687,51 +687,42 @@ TEST_F(AccessContextAuditServiceTest, CookieAccessHelper) {
       base::nullopt /* server_time */);
 
   // Record access to the cookie via a helper.
-  AccessContextAuditService::CookieAccessHelper helper(service());
-  helper.RecordCookieAccess({*test_cookie}, kTopFrameOrigin);
-
-  // Confirm cookie access has been recorded.
-  auto records = GetAllAccessRecords();
-  EXPECT_EQ(1u, records.size());
-  CheckContainsCookieRecord(test_cookie.get(), kTopFrameOrigin, kAccessTime1,
-                            records);
+  auto helper = std::make_unique<AccessContextAuditService::CookieAccessHelper>(
+      service());
+  helper->RecordCookieAccess({*test_cookie}, kTopFrameOrigin);
 
   // Reaccess the cookie at a later time.
   const base::Time kAccessTime2 =
       clock()->Now() + base::TimeDelta::FromMinutes(1);
   clock()->SetNow(kAccessTime2);
-  helper.RecordCookieAccess({*test_cookie}, kTopFrameOrigin);
-
-  // The only record should match the first access.
-  records = GetAllAccessRecords();
-  EXPECT_EQ(1u, records.size());
-  CheckContainsCookieRecord(test_cookie.get(), kTopFrameOrigin, kAccessTime1,
-                            records);
+  helper->RecordCookieAccess({*test_cookie}, kTopFrameOrigin);
 
-  // Clear seen cookies on the helper and reaccess the cookie, and confirm that
-  // the database record is also updated.
-  helper.ClearSeenCookies();
-  helper.RecordCookieAccess({*test_cookie}, kTopFrameOrigin);
-
-  records = GetAllAccessRecords();
+  // The only record should match the second access.
+  auto records = GetAllAccessRecords();
   EXPECT_EQ(1u, records.size());
   CheckContainsCookieRecord(test_cookie.get(), kTopFrameOrigin, kAccessTime2,
                             records);
 
   // Inform the audit service that the cookie has been deleted, which should
-  // cause the helper to clear it from the set of accessed cookies.
+  // cause the helper to clear it from the set of accessed cookies and remove
+  // the database record.
   service()->OnCookieChange(
       net::CookieChangeInfo(*test_cookie, net::CookieAccessResult(),
                             net::CookieChangeCause::EXPLICIT));
 
-  // Update the access time and reaccess the cookie, this should update the
-  // record in the database as the deletion should have cleared it from the set
-  // of seen cookies.
+  // Flush the helper and ensure that no cookie access is recorded.
+  helper->FlushCookieRecords();
+  records = GetAllAccessRecords();
+  EXPECT_EQ(0u, records.size());
+
+  // Record a cookie access and delete the helper, the access should be flushed
+  // to the service.
   const base::Time kAccessTime3 =
       clock()->Now() + base::TimeDelta::FromMinutes(1);
   clock()->SetNow(kAccessTime3);
-  helper.RecordCookieAccess({*test_cookie}, kTopFrameOrigin);
+  helper->RecordCookieAccess({*test_cookie}, kTopFrameOrigin);
 
+  helper.reset();
   records = GetAllAccessRecords();
   EXPECT_EQ(1u, records.size());
   CheckContainsCookieRecord(test_cookie.get(), kTopFrameOrigin, kAccessTime3,

chrome/browser/content_settings/page_specific_content_settings_delegate.cc

@@ -265,15 +265,6 @@ void PageSpecificContentSettingsDelegate::OnWebDatabaseAccessAllowed(
       web_contents());
 #endif  // !defined(OS_ANDROID)
 }
-void PageSpecificContentSettingsDelegate::DidStartNavigation(
-    content::NavigationHandle* navigation_handle) {
-#if !defined(OS_ANDROID)
-  if (cookie_access_helper_ && navigation_handle->IsInMainFrame() &&
-      !navigation_handle->IsSameDocument()) {
-    cookie_access_helper_->ClearSeenCookies();
-  }
-#endif  // !defined(OS_ANDROID)
-}
 
 void PageSpecificContentSettingsDelegate::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {

chrome/browser/content_settings/page_specific_content_settings_delegate.h

@@ -94,8 +94,6 @@ class PageSpecificContentSettingsDelegate
   void OnWebDatabaseAccessAllowed(const url::Origin& origin) override;
 
   // content::WebContentsObserver:
-  void DidStartNavigation(
-      content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;