AppCache: Add holder for AppCacheQuotaClient in AppCacheServiceImpl.

Before this CL, AppCacheServiceImpl owned (as much as a ref-counted
class can be owned) AppCacheQuotaClient directly, via a scoped_refptr.
This introduces the following complexity:

* AppCacheQuotaClient (a complex class) is constructed on the UI thread,
  and accessed later on the IO thread.

* AppCacheStorageImpl and AppCacheStorageImpl (two complex classes) live
  on the UI thread, but are responsible for thread-hopping to the IO
  thread when accessing AppCacheQuotaClient.

* The owning reference for AppCacheQuotaClient lives in the UI thread,
  which forces AppCacheQuotaClient to inherit from RefCountedThreadSafe.
  This is the main reason why QuotaClient is ref-counted.

This CL introduces a QuotaClientHolder class that's internal to
AppCacheServiceImpl. This class lives on the UI thread, and takes on the
responsibility for all thread-hopping needed to access
AppCacheQuotaClient exclusively on the IO thread.

Changing QuotaClient's ownership model away from ref-counting is left
for future CLs, in the interest of keeping this CL manageable.

This CL does not introduce any behavior changes.

Bug: 1016065
Change-Id: I46d7029d054a674c9252fe4b975fef17cabde010
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2519742Reviewed-by: enne <enne@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824559}

content/browser/appcache/appcache_quota_client.cc

@@ -48,9 +48,7 @@ void RunDeleteOnIO(const base::Location& from_here,
 
 AppCacheQuotaClient::AppCacheQuotaClient(
     base::WeakPtr<AppCacheServiceImpl> service)
-    : service_(std::move(service)) {
-  DETACH_FROM_SEQUENCE(sequence_checker_);
-}
+    : service_(std::move(service)) {}
 
 AppCacheQuotaClient::~AppCacheQuotaClient() {
   DCHECK(pending_batch_requests_.empty());

content/browser/appcache/appcache_service_impl.cc

@@ -15,6 +15,8 @@
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted_delete_on_sequence.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/task/thread_pool.h"
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -374,6 +376,97 @@ AppCacheStorageReference::AppCacheStorageReference(
     : storage_(std::move(storage)) {}
 AppCacheStorageReference::~AppCacheStorageReference() = default;
 
+// QuotaClientHolder -------
+
+// Lives on the UI thread, manages an AppCacheQuotaClient on the IO thread.
+class AppCacheServiceImpl::QuotaClientHolder
+    : public base::RefCountedDeleteOnSequence<QuotaClientHolder> {
+ public:
+  REQUIRE_ADOPTION_FOR_REFCOUNTED_TYPE();
+
+  explicit QuotaClientHolder();
+
+  QuotaClientHolder(QuotaClientHolder&) = delete;
+  QuotaClientHolder& operator=(QuotaClientHolder&) = delete;
+
+  void Initialize(scoped_refptr<storage::QuotaManagerProxy> quota_manager_proxy,
+                  base::WeakPtr<AppCacheServiceImpl> appcache_service);
+
+  void NotifyStorageReady();
+
+ private:
+  friend class base::RefCountedDeleteOnSequence<QuotaClientHolder>;
+  friend class base::DeleteHelper<QuotaClientHolder>;
+  ~QuotaClientHolder();
+
+  void InitializeOnIOThread(
+      scoped_refptr<storage::QuotaManagerProxy> quota_manager_proxy,
+      base::WeakPtr<AppCacheServiceImpl> appcache_service);
+
+  void NotifyStorageReadyOnIOThread();
+
+  // This reference must only be accessed on the IO thread.
+  //
+  // Can be null in tests that don't set up a QuotaManager. Always non-null in
+  // shipping code.
+  scoped_refptr<AppCacheQuotaClient> quota_client_;
+};
+
+AppCacheServiceImpl::QuotaClientHolder::QuotaClientHolder()
+    : base::RefCountedDeleteOnSequence<QuotaClientHolder>(
+          GetIOThreadTaskRunner({})) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+}
+
+AppCacheServiceImpl::QuotaClientHolder::~QuotaClientHolder() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (quota_client_)
+    quota_client_->NotifyServiceDestroyed();
+}
+
+void AppCacheServiceImpl::QuotaClientHolder::Initialize(
+    scoped_refptr<storage::QuotaManagerProxy> quota_manager_proxy,
+    base::WeakPtr<AppCacheServiceImpl> appcache_service) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  GetIOThreadTaskRunner({})->PostTask(
+      FROM_HERE,
+      base::BindOnce(&QuotaClientHolder::InitializeOnIOThread,
+                     base::RetainedRef(this), std::move(quota_manager_proxy),
+                     std::move(appcache_service)));
+}
+
+void AppCacheServiceImpl::QuotaClientHolder::NotifyStorageReady() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  GetIOThreadTaskRunner({})->PostTask(
+      FROM_HERE,
+      base::BindOnce(&QuotaClientHolder::NotifyStorageReadyOnIOThread,
+                     base::RetainedRef(this)));
+}
+
+void AppCacheServiceImpl::QuotaClientHolder::InitializeOnIOThread(
+    scoped_refptr<storage::QuotaManagerProxy> quota_manager_proxy,
+    base::WeakPtr<AppCacheServiceImpl> appcache_service) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // Some tests don't set up a QuotaManager.
+  if (!quota_manager_proxy.get())
+    return;
+
+  quota_client_ =
+      base::MakeRefCounted<AppCacheQuotaClient>(std::move(appcache_service));
+  quota_manager_proxy->RegisterClient(quota_client_,
+                                      storage::QuotaClientType::kAppcache,
+                                      {blink::mojom::StorageType::kTemporary});
+}
+
+void AppCacheServiceImpl::QuotaClientHolder::NotifyStorageReadyOnIOThread() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (quota_client_)
+    quota_client_->NotifyStorageReady();
+}
+
 // AppCacheServiceImpl -------
 
 AppCacheServiceImpl::AppCacheServiceImpl(
@@ -385,13 +478,9 @@ AppCacheServiceImpl::AppCacheServiceImpl(
       appcache_policy_(nullptr),
       quota_manager_proxy_(std::move(quota_manager_proxy)),
       force_keep_session_state_(false),
-      partition_(std::move(partition)) {
-  if (quota_manager_proxy_.get()) {
-    quota_client_ = base::MakeRefCounted<AppCacheQuotaClient>(AsWeakPtr());
-    quota_manager_proxy_->RegisterClient(
-        quota_client_, storage::QuotaClientType::kAppcache,
-        {blink::mojom::StorageType::kTemporary});
-  }
+      partition_(std::move(partition)),
+      quota_client_holder_(base::MakeRefCounted<QuotaClientHolder>()) {
+  quota_client_holder_->Initialize(quota_manager_proxy_, AsWeakPtr());
 }
 
 AppCacheServiceImpl::~AppCacheServiceImpl() {
@@ -401,11 +490,6 @@ AppCacheServiceImpl::~AppCacheServiceImpl() {
   for (auto& helper : pending_helpers_)
     helper.first->Cancel();
   pending_helpers_.clear();
-  if (quota_client_) {
-    GetIOThreadTaskRunner({})->PostTask(
-        FROM_HERE, base::BindOnce(&AppCacheQuotaClient::NotifyServiceDestroyed,
-                                  std::move(quota_client_)));
-  }
 
   // Destroy storage_ first; ~AppCacheStorageImpl accesses other data members
   // (special_storage_policy_).
@@ -464,6 +548,10 @@ void AppCacheServiceImpl::Reinitialize() {
   Initialize(cache_directory_);
 }
 
+void AppCacheServiceImpl::NotifyStorageReady() {
+  quota_client_holder_->NotifyStorageReady();
+}
+
 void AppCacheServiceImpl::GetAllAppCacheInfo(
     AppCacheInfoCollection* collection,
     net::CompletionOnceCallback callback) {

content/browser/appcache/appcache_service_impl.h

@@ -13,6 +13,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/sequenced_task_runner.h"
 #include "base/time/time.h"
@@ -110,6 +111,9 @@ class CONTENT_EXPORT AppCacheServiceImpl : public AppCacheService {
   // without relaunching the browser.
   void ScheduleReinitialize();
 
+  // Called by AppCacheStorageImpl when it is fully initialized.
+  void NotifyStorageReady();
+
   // AppCacheService
   void GetAllAppCacheInfo(AppCacheInfoCollection* collection,
                           net::CompletionOnceCallback callback) override;
@@ -147,9 +151,6 @@ class CONTENT_EXPORT AppCacheServiceImpl : public AppCacheService {
     return quota_manager_proxy_.get();
   }
 
-  // This QuotaClient should only be used on the IO thread.
-  AppCacheQuotaClient* quota_client() const { return quota_client_.get(); }
-
   AppCacheStorage* storage() const { return storage_.get(); }
 
   base::WeakPtr<AppCacheServiceImpl> AsWeakPtr() {
@@ -191,7 +192,6 @@ class CONTENT_EXPORT AppCacheServiceImpl : public AppCacheService {
   base::FilePath cache_directory_;
   scoped_refptr<base::SequencedTaskRunner> db_task_runner_;
   AppCachePolicy* appcache_policy_;
-  scoped_refptr<AppCacheQuotaClient> quota_client_;
   std::unique_ptr<AppCacheStorage> storage_;
   scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy_;
   scoped_refptr<storage::QuotaManagerProxy> quota_manager_proxy_;
@@ -206,11 +206,17 @@ class CONTENT_EXPORT AppCacheServiceImpl : public AppCacheService {
   base::WeakPtr<StoragePartitionImpl> partition_;
 
  private:
+  class QuotaClientHolder;
+
   // The (process id, host id) pair that identifies one AppCacheHost.
   using AppCacheHostProcessMap =
       std::map<base::UnguessableToken, std::unique_ptr<AppCacheHost>>;
   AppCacheHostProcessMap hosts_;
 
+  // The QuotaClientHolder is constructed and accessed on the UI thread, and
+  // destroyed on the IO thread.
+  const scoped_refptr<QuotaClientHolder> quota_client_holder_;
+
   base::WeakPtrFactory<AppCacheServiceImpl> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(AppCacheServiceImpl);

content/browser/appcache/appcache_storage.cc

@@ -12,7 +12,7 @@
 #include "content/browser/appcache/appcache_disk_cache_ops.h"
 #include "content/browser/appcache/appcache_response_info.h"
 #include "content/browser/appcache/appcache_service_impl.h"
-#include "storage/browser/quota/quota_client.h"
+#include "storage/browser/quota/quota_client_type.h"
 #include "storage/browser/quota/quota_manager_proxy.h"
 #include "third_party/blink/public/mojom/quota/quota_types.mojom.h"
 

content/browser/appcache/appcache_storage_impl.cc

@@ -307,12 +307,7 @@ void AppCacheStorageImpl::InitTask::RunCompleted() {
         kDelay);
   }
 
-  if (storage_->service()->quota_client()) {
-    GetIOThreadTaskRunner({})->PostTask(
-        FROM_HERE,
-        base::BindOnce(&AppCacheQuotaClient::NotifyStorageReady,
-                       base::RetainedRef(storage_->service()->quota_client())));
-  }
+  storage_->service()->NotifyStorageReady();
 }
 
 // DisableDatabaseTask -------