Refactor use of StoragePartition::ClearData* methods.

This refactor is to remove the passed URLRequestContextGetter as an
argument, preperatory to shifting cookie deletion in StoragePartitionImpl
over to Mojo interfaces.

Bug: 721395
Change-Id: I4568cb5ffd98591605a7c5d8b3850c5d8cd5d9b7
Reviewed-on: https://chromium-review.googlesource.com/754518
Commit-Queue: Randy Smith <rdsmith@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523520}

android_webview/browser/aw_quota_manager_bridge.cc

@@ -233,8 +233,7 @@ void AwQuotaManagerBridge::DeleteOriginOnUiThread(
           StoragePartition::REMOVE_DATA_MASK_FILE_SYSTEMS |
           StoragePartition::REMOVE_DATA_MASK_INDEXEDDB |
           StoragePartition::REMOVE_DATA_MASK_WEBSQL,
-      StoragePartition::QUOTA_MANAGED_STORAGE_MASK_TEMPORARY, GURL(origin),
-      storage_partition->GetURLRequestContext());
+      StoragePartition::QUOTA_MANAGED_STORAGE_MASK_TEMPORARY, GURL(origin));
 }
 
 void AwQuotaManagerBridge::GetOrigins(JNIEnv* env,

chrome/browser/extensions/data_deleter.cc

@@ -4,6 +4,9 @@
 
 #include "chrome/browser/extensions/data_deleter.h"
 
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/task_runner.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_special_storage_policy.h"
 #include "chrome/browser/profiles/profile.h"
@@ -19,6 +22,8 @@
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest_handlers/app_isolation_info.h"
+#include "net/cookies/cookie_store.h"
+#include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 
 using base::WeakPtr;
@@ -30,6 +35,21 @@ namespace extensions {
 
 namespace {
 
+bool DoesCookieMatchHost(const std::string& host,
+                         const net::CanonicalCookie& cookie) {
+  return cookie.IsHostCookie() && cookie.IsDomainMatch(host);
+}
+
+void ClearCookiesOnIOThread(scoped_refptr<net::URLRequestContextGetter> context,
+                            const GURL& origin) {
+  net::CookieStore* cookie_store =
+      context->GetURLRequestContext()->cookie_store();
+  cookie_store->DeleteAllCreatedBetweenWithPredicateAsync(
+      base::Time(), base::Time::Max(),
+      base::BindRepeating(&DoesCookieMatchHost, origin.host()),
+      net::CookieStore::DeleteCallback());
+}
+
 // Helper function that deletes data of a given |storage_origin| in a given
 // |partition|.
 void DeleteOrigin(Profile* profile,
@@ -49,17 +69,26 @@ void DeleteOrigin(Profile* profile,
     // preserve this code path without checking for isolation because it's
     // simpler than special casing.  This code should go away once we merge
     // the various URLRequestContexts (http://crbug.com/159193).
+
     partition->ClearDataForOrigin(
         ~StoragePartition::REMOVE_DATA_MASK_SHADER_CACHE,
-        StoragePartition::QUOTA_MANAGED_STORAGE_MASK_ALL, origin,
-        profile->GetRequestContextForExtensions());
+        StoragePartition::QUOTA_MANAGED_STORAGE_MASK_ALL, origin);
+
+    // Delete cookies separately from other data so that the request context
+    // for extensions doesn't need to be passed into the StoragePartition.
+    // TODO(rdsmith): Mojoify this call and get rid of the thread hopping.
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::BindOnce(
+            &ClearCookiesOnIOThread,
+            base::WrapRefCounted(profile->GetRequestContextForExtensions()),
+            origin));
   } else {
     // We don't need to worry about the media request context because that
     // shares the same cookie store as the main request context.
     partition->ClearDataForOrigin(
         ~StoragePartition::REMOVE_DATA_MASK_SHADER_CACHE,
-        StoragePartition::QUOTA_MANAGED_STORAGE_MASK_ALL, origin,
-        partition->GetURLRequestContext());
+        StoragePartition::QUOTA_MANAGED_STORAGE_MASK_ALL, origin);
   }
 }
 

content/browser/browsing_data/browsing_data_remover_impl_unittest.cc

@@ -134,8 +134,7 @@ class StoragePartitionRemovalTestStoragePartition
 
   void ClearDataForOrigin(uint32_t remove_mask,
                           uint32_t quota_storage_remove_mask,
-                          const GURL& storage_origin,
-                          net::URLRequestContextGetter* rq_context) override {}
+                          const GURL& storage_origin) override {}
 
   void ClearData(uint32_t remove_mask,
                  uint32_t quota_storage_remove_mask,

content/browser/devtools/protocol/storage_handler.cc

@@ -307,7 +307,7 @@ Response StorageHandler::ClearDataForOrigin(
 
   partition->ClearDataForOrigin(
       remove_mask, StoragePartition::QUOTA_MANAGED_STORAGE_MASK_ALL,
-      GURL(origin), partition->GetURLRequestContext());
+      GURL(origin));
   return Response::OK();
 }
 

content/browser/storage_partition_impl.cc

@@ -67,16 +67,16 @@ bool DoesCookieMatchHost(const std::string& host,
   return cookie.IsHostCookie() && cookie.IsDomainMatch(host);
 }
 
-void OnClearedCookies(const base::Closure& callback, uint32_t num_deleted) {
+void OnClearedCookies(base::OnceClosure callback, uint32_t num_deleted) {
   // The final callback needs to happen from UI thread.
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        base::BindOnce(&OnClearedCookies, callback, num_deleted));
+        base::BindOnce(&OnClearedCookies, std::move(callback), num_deleted));
     return;
   }
 
-  callback.Run();
+  std::move(callback).Run();
 }
 
 // Cookie matcher and storage_origin are never both populated.
@@ -86,7 +86,7 @@ void ClearCookiesOnIOThread(
     const base::Time end,
     const GURL& storage_origin,
     const StoragePartition::CookieMatcherFunction& cookie_matcher,
-    const base::Closure& callback) {
+    base::OnceClosure callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(cookie_matcher.is_null() || storage_origin.is_empty());
   net::CookieStore* cookie_store =
@@ -94,7 +94,7 @@ void ClearCookiesOnIOThread(
   if (!cookie_matcher.is_null()) {
     cookie_store->DeleteAllCreatedBetweenWithPredicateAsync(
         begin, end, cookie_matcher,
-        base::BindOnce(&OnClearedCookies, callback));
+        base::BindOnce(&OnClearedCookies, std::move(callback)));
     return;
   }
   if (!storage_origin.is_empty()) {
@@ -104,11 +104,11 @@ void ClearCookiesOnIOThread(
     cookie_store->DeleteAllCreatedBetweenWithPredicateAsync(
         begin, end,
         StoragePartitionImpl::CreatePredicateForHostCookies(storage_origin),
-        base::BindOnce(&OnClearedCookies, callback));
+        base::BindOnce(&OnClearedCookies, std::move(callback)));
     return;
   }
   cookie_store->DeleteAllCreatedBetweenAsync(
-      begin, end, base::BindOnce(&OnClearedCookies, callback));
+      begin, end, base::BindOnce(&OnClearedCookies, std::move(callback)));
 }
 
 void CheckQuotaManagedDataDeletionStatus(size_t* deletion_task_count,
@@ -355,6 +355,30 @@ struct StoragePartitionImpl::QuotaManagedDataDeletionHelper {
 // forwarded and updated on each (sub) deletion's callback. The instance is
 // finally destroyed when deletion completes (and |callback| is invoked).
 struct StoragePartitionImpl::DataDeletionHelper {
+  // An instance of this class is used instead of a callback to
+  // DecrementTaskCount when the callback may be destroyed
+  // rather than invoked.  The destruction of this object (which also
+  // occurs if the null callback is called) will automatically decrement
+  // the task count.
+  // Note that this object may be destroyed on any thread, as
+  // DecrementTaskCount() is thread-neutral.
+  // Note that the DataDeletionHelper must outlive this object.  This
+  // should be guaranteed by the fact that the object holds a reference
+  // to the DataDeletionHelper.
+  class OwnsReference {
+   public:
+    OwnsReference(DataDeletionHelper* helper) : helper_(helper) {
+      DCHECK_CURRENTLY_ON(BrowserThread::UI);
+      helper->IncrementTaskCountOnUI();
+    }
+
+    ~OwnsReference() { helper_->DecrementTaskCount(); }
+
+    static void Callback(std::unique_ptr<OwnsReference> reference) {}
+
+    DataDeletionHelper* helper_;
+  };
+
   DataDeletionHelper(uint32_t remove_mask,
                      uint32_t quota_storage_remove_mask,
                      base::OnceClosure callback)
@@ -364,7 +388,7 @@ struct StoragePartitionImpl::DataDeletionHelper {
         task_count(0) {}
 
   void IncrementTaskCountOnUI();
-  void DecrementTaskCountOnUI();
+  void DecrementTaskCount();  // Callable on any thread.
 
   void ClearDataOnUIThread(
       const GURL& storage_origin,
@@ -739,7 +763,6 @@ void StoragePartitionImpl::ClearDataImpl(
     const GURL& storage_origin,
     const OriginMatcherFunction& origin_matcher,
     const CookieMatcherFunction& cookie_matcher,
-    net::URLRequestContextGetter* rq_context,
     const base::Time begin,
     const base::Time end,
     base::OnceClosure callback) {
@@ -747,10 +770,10 @@ void StoragePartitionImpl::ClearDataImpl(
   DataDeletionHelper* helper = new DataDeletionHelper(
       remove_mask, quota_storage_remove_mask, std::move(callback));
   // |helper| deletes itself when done in
-  // DataDeletionHelper::DecrementTaskCountOnUI().
+  // DataDeletionHelper::DecrementTaskCount().
   helper->ClearDataOnUIThread(
-      storage_origin, origin_matcher, cookie_matcher, GetPath(), rq_context,
-      dom_storage_context_.get(), quota_manager_.get(),
+      storage_origin, origin_matcher, cookie_matcher, GetPath(),
+      GetURLRequestContext(), dom_storage_context_.get(), quota_manager_.get(),
       special_storage_policy_.get(), filesystem_context_.get(), begin, end);
 }
 
@@ -865,11 +888,11 @@ void StoragePartitionImpl::DataDeletionHelper::IncrementTaskCountOnUI() {
   ++task_count;
 }
 
-void StoragePartitionImpl::DataDeletionHelper::DecrementTaskCountOnUI() {
+void StoragePartitionImpl::DataDeletionHelper::DecrementTaskCount() {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        base::BindOnce(&DataDeletionHelper::DecrementTaskCountOnUI,
+        base::BindOnce(&DataDeletionHelper::DecrementTaskCount,
                        base::Unretained(this)));
     return;
   }
@@ -898,16 +921,21 @@ void StoragePartitionImpl::DataDeletionHelper::ClearDataOnUIThread(
 
   IncrementTaskCountOnUI();
   base::Closure decrement_callback = base::Bind(
-      &DataDeletionHelper::DecrementTaskCountOnUI, base::Unretained(this));
+      &DataDeletionHelper::DecrementTaskCount, base::Unretained(this));
 
   if (remove_mask & REMOVE_DATA_MASK_COOKIES) {
     // Handle the cookies.
-    IncrementTaskCountOnUI();
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
-        base::BindOnce(&ClearCookiesOnIOThread,
-                       base::WrapRefCounted(rq_context), begin, end,
-                       storage_origin, cookie_matcher, decrement_callback));
+        base::BindOnce(
+            &ClearCookiesOnIOThread, base::WrapRefCounted(rq_context), begin,
+            end, storage_origin, cookie_matcher,
+            // Use OwnsReference instead of Increment/DecrementTaskCount*
+            // to handle the cookie store being destroyed and the callback
+            // thus not being called.
+            base::BindOnce(
+                &OwnsReference::Callback,
+                base::Passed(std::make_unique<OwnsReference>(this)))));
   }
 
   if (remove_mask & REMOVE_DATA_MASK_INDEXEDDB ||
@@ -963,19 +991,17 @@ void StoragePartitionImpl::DataDeletionHelper::ClearDataOnUIThread(
   }
 #endif  // BUILDFLAG(ENABLE_PLUGINS)
 
-  DecrementTaskCountOnUI();
+  DecrementTaskCount();
 }
 
 void StoragePartitionImpl::ClearDataForOrigin(
     uint32_t remove_mask,
     uint32_t quota_storage_remove_mask,
-    const GURL& storage_origin,
-    net::URLRequestContextGetter* request_context_getter) {
+    const GURL& storage_origin) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   ClearDataImpl(remove_mask, quota_storage_remove_mask, storage_origin,
-                OriginMatcherFunction(), CookieMatcherFunction(),
-                request_context_getter, base::Time(), base::Time::Max(),
-                base::Bind(&base::DoNothing));
+                OriginMatcherFunction(), CookieMatcherFunction(), base::Time(),
+                base::Time::Max(), base::BindOnce(&base::DoNothing));
 }
 
 void StoragePartitionImpl::ClearData(
@@ -987,8 +1013,8 @@ void StoragePartitionImpl::ClearData(
     const base::Time end,
     base::OnceClosure callback) {
   ClearDataImpl(remove_mask, quota_storage_remove_mask, storage_origin,
-                origin_matcher, CookieMatcherFunction(), GetURLRequestContext(),
-                begin, end, std::move(callback));
+                origin_matcher, CookieMatcherFunction(), begin, end,
+                std::move(callback));
 }
 
 void StoragePartitionImpl::ClearData(
@@ -1000,8 +1026,7 @@ void StoragePartitionImpl::ClearData(
     const base::Time end,
     base::OnceClosure callback) {
   ClearDataImpl(remove_mask, quota_storage_remove_mask, GURL(), origin_matcher,
-                cookie_matcher, GetURLRequestContext(), begin, end,
-                std::move(callback));
+                cookie_matcher, begin, end, std::move(callback));
 }
 
 void StoragePartitionImpl::ClearHttpAndMediaCaches(

content/browser/storage_partition_impl.h

@@ -93,11 +93,9 @@ class CONTENT_EXPORT StoragePartitionImpl
   ZoomLevelDelegate* GetZoomLevelDelegate() override;
 #endif  // !defined(OS_ANDROID)
   PlatformNotificationContextImpl* GetPlatformNotificationContext() override;
-  void ClearDataForOrigin(
-      uint32_t remove_mask,
-      uint32_t quota_storage_remove_mask,
-      const GURL& storage_origin,
-      net::URLRequestContextGetter* request_context_getter) override;
+  void ClearDataForOrigin(uint32_t remove_mask,
+                          uint32_t quota_storage_remove_mask,
+                          const GURL& storage_origin) override;
   void ClearData(uint32_t remove_mask,
                  uint32_t quota_storage_remove_mask,
                  const GURL& storage_origin,
@@ -217,7 +215,6 @@ class CONTENT_EXPORT StoragePartitionImpl
                      const GURL& remove_origin,
                      const OriginMatcherFunction& origin_matcher,
                      const CookieMatcherFunction& cookie_matcher,
-                     net::URLRequestContextGetter* rq_context,
                      const base::Time begin,
                      const base::Time end,
                      base::OnceClosure callback);

content/public/browser/storage_partition.h

@@ -128,8 +128,7 @@ class CONTENT_EXPORT StoragePartition {
   // is fixed.
   virtual void ClearDataForOrigin(uint32_t remove_mask,
                                   uint32_t quota_storage_remove_mask,
-                                  const GURL& storage_origin,
-                                  net::URLRequestContextGetter* rq_context) = 0;
+                                  const GURL& storage_origin) = 0;
 
   // A callback type to check if a given origin matches a storage policy.
   // Can be passed empty/null where used, which means the origin will always

content/public/test/test_storage_partition.cc

@@ -85,8 +85,7 @@ ZoomLevelDelegate* TestStoragePartition::GetZoomLevelDelegate() {
 void TestStoragePartition::ClearDataForOrigin(
     uint32_t remove_mask,
     uint32_t quota_storage_remove_mask,
-    const GURL& storage_origin,
-    net::URLRequestContextGetter* rq_context) {}
+    const GURL& storage_origin) {}
 
 void TestStoragePartition::ClearData(
     uint32_t remove_mask,

content/public/test/test_storage_partition.h

@@ -126,8 +126,7 @@ class TestStoragePartition : public StoragePartition {
 
   void ClearDataForOrigin(uint32_t remove_mask,
                           uint32_t quota_storage_remove_mask,
-                          const GURL& storage_origin,
-                          net::URLRequestContextGetter* rq_context) override;
+                          const GURL& storage_origin) override;
 
   void ClearData(uint32_t remove_mask,
                  uint32_t quota_storage_remove_mask,