Fix appcache crash if QuotaManager is destroyed in NotifyAppCacheDestroyed

It's possible the AppCacheQuotaClient's callback is holding the last
reference to QuotaManager, which means QuotaManager gets destroyed when
those callbacks are run. This causes AppCacheQuotaClient to be destroyed
when it is still executing code in NotifyAppCacheDestroyed.

Bug: 1003298
Change-Id: I07afe921c10f34c9bdee8c020b7194e659337281
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1827490
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700381}

content/browser/appcache/appcache_quota_client.cc

@@ -68,7 +68,12 @@ void AppCacheQuotaClient::OnQuotaManagerDestroyed() {
     GetServiceDeleteCallback()->Cancel();
   }
 
-  delete this;
+  // Wait to delete until NotifyAppCacheDestroyed is done running, otherwise
+  // just delete now.
+  if (keep_alive_)
+    keep_alive_ = false;
+  else
+    delete this;
 }
 
 void AppCacheQuotaClient::GetOriginUsage(const url::Origin& origin,
@@ -259,6 +264,9 @@ void AppCacheQuotaClient::NotifyAppCacheReady() {
 
 void AppCacheQuotaClient::NotifyAppCacheDestroyed() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Keep this object alive for the duration of this method.
+  keep_alive_ = true;
+
   service_ = nullptr;
   service_is_destroyed_ = true;
   while (!pending_batch_requests_.empty())
@@ -272,6 +280,16 @@ void AppCacheQuotaClient::NotifyAppCacheDestroyed() {
         .Run(blink::mojom::QuotaStatusCode::kErrorAbort);
     GetServiceDeleteCallback()->Cancel();
   }
+
+  // It's possible one of the pending callbacks was holding the last reference
+  // to QuotaManager, which means QuotaManager gets destroyed when those
+  // callbacks are run. If this happened, OnQuotaManagerDestroyed() will have
+  // set |keep_alive_| to false and we can delete now. Otherwise, let
+  // OnQuotaManagerDestroyed() delete this object when it's run.
+  if (keep_alive_)
+    keep_alive_ = false;
+  else
+    delete this;
 }
 
 }  // namespace content

content/browser/appcache/appcache_quota_client.h

@@ -88,6 +88,9 @@ class AppCacheQuotaClient : public storage::QuotaClient,
   base::WeakPtr<AppCacheServiceImpl> service_;
   bool appcache_is_ready_ = false;
   bool service_is_destroyed_ = false;
+  // This is used to prevent this object from being deleted in
+  // OnQuotaManagerDestroyed() while NotifyAppCacheDestroyed() is still running.
+  bool keep_alive_ = false;
   SEQUENCE_CHECKER(sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(AppCacheQuotaClient);

content/browser/appcache/appcache_quota_client_unittest.cc

@@ -168,7 +168,6 @@ class AppCacheQuotaClientTest : public testing::Test {
   base::WeakPtrFactory<AppCacheQuotaClientTest> weak_factory_{this};
 };
 
-
 TEST_F(AppCacheQuotaClientTest, BasicCreateDestroy) {
   base::WeakPtr<AppCacheQuotaClient> client = CreateClient();
   Call_NotifyAppCacheReady(client);
@@ -176,6 +175,20 @@ TEST_F(AppCacheQuotaClientTest, BasicCreateDestroy) {
   Call_NotifyAppCacheDestroyed(client);
 }
 
+TEST_F(AppCacheQuotaClientTest, QuotaManagerDestroyedInCallback) {
+  base::WeakPtr<AppCacheQuotaClient> client = CreateClient();
+  Call_NotifyAppCacheReady(client);
+  client->DeleteOriginData(kOriginA, kTemp,
+                           base::BindOnce(
+                               [](AppCacheQuotaClientTest* test,
+                                  base::WeakPtr<AppCacheQuotaClient> client,
+                                  blink::mojom::QuotaStatusCode) {
+                                 test->Call_OnQuotaManagerDestroyed(client);
+                               },
+                               this, client));
+  Call_NotifyAppCacheDestroyed(client);
+}
+
 TEST_F(AppCacheQuotaClientTest, EmptyService) {
   base::WeakPtr<AppCacheQuotaClient> client = CreateClient();
   Call_NotifyAppCacheReady(client);