Add clear on exit logic to conversions storage

What:
Delete data from conversions database on exit for origins which are
configured to be session only.

Why:
This better aligns conversion storage with other site data
implementations.

How:
When the ConversionManager for a given storage partition is tore down,
we queue a clear data task which deletes all conversion data whose
origins match a session only origin.

Note that as is, using ClearData() with the new filter will be
rather un-performant as it will trigger a host settings lookup on
every invocation of the filter. This is being addressed on
https://crrev.com/c/2568367

Bug: 1154453
Change-Id: If6f250ddd3329dfb595db3b83e2601b22a49c2c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2567626
Commit-Queue: John Delaney <johnidel@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838274}

content/browser/conversions/conversion_manager_impl.cc

@@ -22,6 +22,25 @@
 
 namespace content {
 
+namespace {
+
+bool IsOriginSessionOnly(
+    scoped_refptr<storage::SpecialStoragePolicy> storage_policy,
+    const url::Origin& origin) {
+  // TODO(johnidel): This conversion is unfortunate but necessary. Storage
+  // partition clear data logic uses Origin keyed deletion, while the storage
+  // policy uses GURLs. Ideally these would be coalesced.
+  const GURL& url = origin.GetURL();
+  if (storage_policy->IsStorageProtected(url))
+    return false;
+
+  if (storage_policy->IsStorageSessionOnly(url))
+    return true;
+  return false;
+}
+
+}  // namespace
+
 const constexpr base::TimeDelta kConversionManagerQueueReportsInterval =
     base::TimeDelta::FromMinutes(30);
 
@@ -43,14 +62,17 @@ std::unique_ptr<ConversionManagerImpl> ConversionManagerImpl::CreateForTesting(
     std::unique_ptr<ConversionReporter> reporter,
     std::unique_ptr<ConversionPolicy> policy,
     const base::Clock* clock,
-    const base::FilePath& user_data_directory) {
+    const base::FilePath& user_data_directory,
+    scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy) {
   return base::WrapUnique<ConversionManagerImpl>(new ConversionManagerImpl(
-      std::move(reporter), std::move(policy), clock, user_data_directory));
+      std::move(reporter), std::move(policy), clock, user_data_directory,
+      std::move(special_storage_policy)));
 }
 
 ConversionManagerImpl::ConversionManagerImpl(
     StoragePartition* storage_partition,
-    const base::FilePath& user_data_directory)
+    const base::FilePath& user_data_directory,
+    scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy)
     : ConversionManagerImpl(
           std::make_unique<ConversionReporterImpl>(
               storage_partition,
@@ -59,13 +81,15 @@ ConversionManagerImpl::ConversionManagerImpl(
               base::CommandLine::ForCurrentProcess()->HasSwitch(
                   switches::kConversionsDebugMode)),
           base::DefaultClock::GetInstance(),
-          user_data_directory) {}
+          user_data_directory,
+          std::move(special_storage_policy)) {}
 
 ConversionManagerImpl::ConversionManagerImpl(
     std::unique_ptr<ConversionReporter> reporter,
     std::unique_ptr<ConversionPolicy> policy,
     const base::Clock* clock,
-    const base::FilePath& user_data_directory)
+    const base::FilePath& user_data_directory,
+    scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy)
     : debug_mode_(base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kConversionsDebugMode)),
       clock_(clock),
@@ -76,6 +100,7 @@ ConversionManagerImpl::ConversionManagerImpl(
               std::make_unique<ConversionStorageDelegateImpl>(debug_mode_),
               clock_)),
       conversion_policy_(std::move(policy)),
+      special_storage_policy_(std::move(special_storage_policy)),
       weak_factory_(this) {
   // Once the database is loaded, get all reports that may have expired while
   // Chrome was not running and handle these specially. It is safe to post tasks
@@ -92,7 +117,22 @@ ConversionManagerImpl::ConversionManagerImpl(
       &ConversionManagerImpl::GetAndQueueReportsForNextInterval);
 }
 
-ConversionManagerImpl::~ConversionManagerImpl() = default;
+ConversionManagerImpl::~ConversionManagerImpl() {
+  // Browser contexts are not required to have a special storage policy.
+  if (!special_storage_policy_ ||
+      !special_storage_policy_->HasSessionOnlyOrigins()) {
+    return;
+  }
+
+  // Delete stored data for all session only origins given by
+  // |special_storage_policy|.
+  base::RepeatingCallback<bool(const url::Origin&)>
+      session_only_origin_predicate = base::BindRepeating(
+          &IsOriginSessionOnly, std::move(special_storage_policy_));
+  conversion_storage_context_->ClearData(base::Time::Min(), base::Time::Max(),
+                                         session_only_origin_predicate,
+                                         base::DoNothing());
+}
 
 void ConversionManagerImpl::HandleImpression(
     const StorableImpression& impression) {

content/browser/conversions/conversion_manager_impl.h

@@ -18,6 +18,7 @@
 #include "content/browser/conversions/conversion_manager.h"
 #include "content/browser/conversions/conversion_policy.h"
 #include "content/browser/conversions/conversion_storage_context.h"
+#include "storage/browser/quota/special_storage_policy.h"
 
 namespace base {
 
@@ -75,10 +76,13 @@ class CONTENT_EXPORT ConversionManagerImpl : public ConversionManager {
       std::unique_ptr<ConversionReporter> reporter,
       std::unique_ptr<ConversionPolicy> policy,
       const base::Clock* clock,
-      const base::FilePath& user_data_directory);
+      const base::FilePath& user_data_directory,
+      scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy);
 
-  ConversionManagerImpl(StoragePartition* storage_partition,
-                        const base::FilePath& user_data_directory);
+  ConversionManagerImpl(
+      StoragePartition* storage_partition,
+      const base::FilePath& user_data_directory,
+      scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy);
   ConversionManagerImpl(const ConversionManagerImpl& other) = delete;
   ConversionManagerImpl& operator=(const ConversionManagerImpl& other) = delete;
   ~ConversionManagerImpl() override;
@@ -100,10 +104,12 @@ class CONTENT_EXPORT ConversionManagerImpl : public ConversionManager {
                  base::OnceClosure done) override;
 
  private:
-  ConversionManagerImpl(std::unique_ptr<ConversionReporter> reporter,
-                        std::unique_ptr<ConversionPolicy> policy,
-                        const base::Clock* clock,
-                        const base::FilePath& user_data_directory);
+  ConversionManagerImpl(
+      std::unique_ptr<ConversionReporter> reporter,
+      std::unique_ptr<ConversionPolicy> policy,
+      const base::Clock* clock,
+      const base::FilePath& user_data_directory,
+      scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy);
 
   // Retrieves reports from storage whose |report_time| <= |max_report_time|,
   // and calls |handler_function| on them.
@@ -162,6 +168,9 @@ class CONTENT_EXPORT ConversionManagerImpl : public ConversionManager {
   // attribution models. Unique ptr so it can be overridden for testing.
   std::unique_ptr<ConversionPolicy> conversion_policy_;
 
+  // Storage policy for the browser context |this| is in. May be nullptr.
+  scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy_;
+
   base::WeakPtrFactory<ConversionManagerImpl> weak_factory_;
 };
 

content/browser/conversions/conversion_manager_impl_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/bind.h"
 #include "base/callback_forward.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/run_loop.h"
 #include "base/sequenced_task_runner.h"
 #include "base/test/bind.h"
@@ -24,6 +25,7 @@
 #include "content/browser/conversions/storable_conversion.h"
 #include "content/browser/conversions/storable_impression.h"
 #include "content/public/test/browser_task_environment.h"
+#include "storage/browser/test/mock_special_storage_policy.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
@@ -110,7 +112,9 @@ constexpr base::TimeDelta kImpressionExpiry = base::TimeDelta::FromDays(30);
 class ConversionManagerImplTest : public testing::Test {
  public:
   ConversionManagerImplTest()
-      : task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {
+      : task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME),
+        mock_storage_policy_(
+            base::MakeRefCounted<storage::MockSpecialStoragePolicy>()) {
     EXPECT_TRUE(dir_.CreateUniqueTempDir());
     CreateManager();
   }
@@ -120,7 +124,32 @@ class ConversionManagerImplTest : public testing::Test {
     test_reporter_ = reporter.get();
     conversion_manager_ = ConversionManagerImpl::CreateForTesting(
         std::move(reporter), std::make_unique<ConstantStartupDelayPolicy>(),
-        task_environment_.GetMockClock(), dir_.GetPath());
+        task_environment_.GetMockClock(), dir_.GetPath(), mock_storage_policy_);
+  }
+
+  void ExpectNumStoredImpressions(size_t expected_num_impressions) {
+    // There should be one impression and one conversion.
+    base::RunLoop impression_loop;
+    auto get_impressions_callback = base::BindLambdaForTesting(
+        [&](std::vector<StorableImpression> impressions) {
+          EXPECT_EQ(expected_num_impressions, impressions.size());
+          impression_loop.Quit();
+        });
+    conversion_manager_->GetActiveImpressionsForWebUI(
+        std::move(get_impressions_callback));
+    impression_loop.Run();
+  }
+
+  void ExpectNumStoredReports(size_t expected_num_reports) {
+    base::RunLoop report_loop;
+    auto reports_callback =
+        base::BindLambdaForTesting([&](std::vector<ConversionReport> reports) {
+          EXPECT_EQ(expected_num_reports, reports.size());
+          report_loop.Quit();
+        });
+    conversion_manager_->GetReportsForWebUI(std::move(reports_callback),
+                                            base::Time::Max());
+    report_loop.Run();
   }
 
   const base::Clock& clock() { return *task_environment_.GetMockClock(); }
@@ -130,6 +159,7 @@ class ConversionManagerImplTest : public testing::Test {
   BrowserTaskEnvironment task_environment_;
   std::unique_ptr<ConversionManagerImpl> conversion_manager_;
   TestConversionReporter* test_reporter_ = nullptr;
+  scoped_refptr<storage::MockSpecialStoragePolicy> mock_storage_policy_;
 };
 
 TEST_F(ConversionManagerImplTest, ImpressionRegistered_ReturnedToWebUI) {
@@ -371,4 +401,63 @@ TEST_F(ConversionManagerImplTest, NonExpiredReportsQueuedAtStartup_NotDelayed) {
             test_reporter_->last_report_time());
 }
 
+TEST_F(ConversionManagerImplTest, SessionOnlyOrigins_DataDeletedAtShutdown) {
+  GURL session_only_origin("https://sessiononly.example");
+  auto impression =
+      ImpressionBuilder(clock().Now())
+          .SetImpressionOrigin(url::Origin::Create(session_only_origin))
+          .Build();
+
+  mock_storage_policy_->AddSessionOnly(session_only_origin);
+
+  conversion_manager_->HandleImpression(impression);
+  conversion_manager_->HandleConversion(DefaultConversion());
+
+  ExpectNumStoredImpressions(1u);
+  ExpectNumStoredReports(1u);
+
+  // Reset the manager to simulate shutdown.
+  conversion_manager_.reset();
+  CreateManager();
+
+  ExpectNumStoredImpressions(0u);
+  ExpectNumStoredReports(0u);
+}
+
+TEST_F(ConversionManagerImplTest,
+       SessionOnlyOrigins_DeletedIfAnyOriginMatches) {
+  url::Origin session_only_origin =
+      url::Origin::Create(GURL("https://sessiononly.example"));
+  // Create impressions which each have the session only origin as one of
+  // impression/conversion/reporting origin.
+  auto impression1 = ImpressionBuilder(clock().Now())
+                         .SetImpressionOrigin(session_only_origin)
+                         .Build();
+  auto impression2 = ImpressionBuilder(clock().Now())
+                         .SetReportingOrigin(session_only_origin)
+                         .Build();
+  auto impression3 = ImpressionBuilder(clock().Now())
+                         .SetConversionOrigin(session_only_origin)
+                         .Build();
+
+  // Create one  impression which is not session only.
+  auto impression4 = ImpressionBuilder(clock().Now()).Build();
+
+  mock_storage_policy_->AddSessionOnly(session_only_origin.GetURL());
+
+  conversion_manager_->HandleImpression(impression1);
+  conversion_manager_->HandleImpression(impression2);
+  conversion_manager_->HandleImpression(impression3);
+  conversion_manager_->HandleImpression(impression4);
+
+  ExpectNumStoredImpressions(4u);
+
+  // Reset the manager to simulate shutdown.
+  conversion_manager_.reset();
+  CreateManager();
+
+  // All session-only impressions should be deleted.
+  ExpectNumStoredImpressions(1u);
+}
+
 }  // namespace content

content/browser/conversions/conversion_storage_context.cc

@@ -15,10 +15,14 @@ namespace {
 // The shared-task runner for all conversion storage operations. Note that
 // different ConversionStorageContext perform operations on the same task
 // runner. This prevents any potential races when a given context is destroyed
-// and recreated for the same backing storage.
+// and recreated for the same backing storage. This uses
+// BLOCK_SHUTDOWN as some data deletion operations may be running when the
+// browser is closed, and we want to ensure all data is deleted correctly.
 base::LazyThreadPoolSequencedTaskRunner g_storage_task_runner =
     LAZY_THREAD_POOL_SEQUENCED_TASK_RUNNER_INITIALIZER(
-        base::TaskTraits(base::TaskPriority::BEST_EFFORT, base::MayBlock()));
+        base::TaskTraits(base::TaskPriority::BEST_EFFORT,
+                         base::MayBlock(),
+                         base::TaskShutdownBehavior::BLOCK_SHUTDOWN));
 
 }  // namespace
 

content/browser/conversions/conversion_storage_sql.cc

@@ -12,6 +12,7 @@
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/optional.h"
 #include "base/time/default_clock.h"
@@ -434,6 +435,10 @@ void ConversionStorageSql::ClearData(
     return;
   }
 
+  // Measure the time it takes to perform a clear with a filter separately from
+  // the above histogram.
+  SCOPED_UMA_HISTOGRAM_TIMER("Conversions.Storage.ClearDataWithFilterDuration");
+
   // TODO(csharrison, johnidel): This query can be split up and optimized by
   // adding indexes on the impression_time and conversion_time columns.
   // See this comment for more information:

content/browser/storage_partition_impl.cc

@@ -1303,7 +1303,8 @@ void StoragePartitionImpl::Initialize(
   // The Conversion Measurement API is not available in Incognito mode.
   if (!is_in_memory_ &&
       base::FeatureList::IsEnabled(features::kConversionMeasurement)) {
-    conversion_manager_ = std::make_unique<ConversionManagerImpl>(this, path);
+    conversion_manager_ = std::make_unique<ConversionManagerImpl>(
+        this, path, special_storage_policy_);
   }
 
   GeneratedCodeCacheSettings settings =

tools/metrics/histograms/histograms_xml/others/histograms.xml

@@ -3105,6 +3105,18 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram name="Conversions.Storage.ClearDataWithFilterDuration" units="ms"
+    expires_after="M95">
+  <owner>johnidel@chromium.org</owner>
+  <owner>csharrison@chromium.org</owner>
+  <summary>
+    Records the time it took to do perform a clear operation on the database
+    when supplied an origin filter. Recorded when the conversion database
+    finishes performing a clear operation. Note that filters may vary in size
+    and computation complexity, skewing this metric aribtrarily.
+  </summary>
+</histogram>
+
 <histogram name="Conversions.TimeFromConversionToReportSend" units="hours"
     expires_after="M95">
   <owner>johnidel@chromium.org</owner>