[AppCache] Move binding ownership to the profile-owned service.

This change fixes lifetime issues where the mojo service, created with
mojo::MakeStrongBinding, stayed alive after the Profile, AppCache
service, and things like the URLRequestContext have been destroyed.

This is a little scary, as the AppCacheDispatcherHost and general
AppCache code holds lots of raw pointers, and I'm sure this was the
cause of other strange bugs.

Prior to r522566 (when IPCs were converted to Mojo), the dispatcher
object was owned by the RenderProcessHostImpl, and created during
message filter creation. The filters were explicitly destroyed,
which made the lifetime deterministic.

Bug: 800391
Change-Id: Ie56839de3202be5c63080cf31e7a1391eb7ce405
Reviewed-on: https://chromium-review.googlesource.com/922919Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537144}

content/browser/appcache/appcache_dispatcher_host.cc

@@ -12,7 +12,6 @@
 #include "content/browser/bad_message.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/browser_side_navigation_policy.h"
-#include "mojo/public/cpp/bindings/strong_binding.h"
 
 namespace content {
 
@@ -33,13 +32,13 @@ AppCacheDispatcherHost::~AppCacheDispatcherHost() = default;
 void AppCacheDispatcherHost::Create(ChromeAppCacheService* appcache_service,
                                     int process_id,
                                     mojom::AppCacheBackendRequest request) {
-  mojo::MakeStrongBinding(
+  appcache_service->Bind(
       std::make_unique<AppCacheDispatcherHost>(appcache_service, process_id),
       std::move(request));
 }
 
 void AppCacheDispatcherHost::RegisterHost(int32_t host_id) {
-  if (appcache_service_.get()) {
+  if (appcache_service_) {
     // PlzNavigate
     // The AppCacheHost could have been precreated in which case we want to
     // register it with the backend here.
@@ -58,7 +57,7 @@ void AppCacheDispatcherHost::RegisterHost(int32_t host_id) {
 }
 
 void AppCacheDispatcherHost::UnregisterHost(int32_t host_id) {
-  if (appcache_service_.get()) {
+  if (appcache_service_) {
     if (!backend_impl_.UnregisterHost(host_id)) {
       mojo::ReportBadMessage("ACDH_UNREGISTER");
     }
@@ -67,7 +66,7 @@ void AppCacheDispatcherHost::UnregisterHost(int32_t host_id) {
 
 void AppCacheDispatcherHost::SetSpawningHostId(int32_t host_id,
                                                int spawning_host_id) {
-  if (appcache_service_.get()) {
+  if (appcache_service_) {
     if (!backend_impl_.SetSpawningHostId(host_id, spawning_host_id))
       mojo::ReportBadMessage("ACDH_SET_SPAWNING");
   }
@@ -77,7 +76,7 @@ void AppCacheDispatcherHost::SelectCache(int32_t host_id,
                                          const GURL& document_url,
                                          int64_t cache_document_was_loaded_from,
                                          const GURL& opt_manifest_url) {
-  if (appcache_service_.get()) {
+  if (appcache_service_) {
     if (!backend_impl_.SelectCache(host_id, document_url,
                                    cache_document_was_loaded_from,
                                    opt_manifest_url)) {
@@ -90,7 +89,7 @@ void AppCacheDispatcherHost::SelectCache(int32_t host_id,
 
 void AppCacheDispatcherHost::SelectCacheForSharedWorker(int32_t host_id,
                                                         int64_t appcache_id) {
-  if (appcache_service_.get()) {
+  if (appcache_service_) {
     if (!backend_impl_.SelectCacheForSharedWorker(host_id, appcache_id))
       mojo::ReportBadMessage("ACDH_SELECT_CACHE_FOR_SHARED_WORKER");
   } else {
@@ -102,7 +101,7 @@ void AppCacheDispatcherHost::MarkAsForeignEntry(
     int32_t host_id,
     const GURL& document_url,
     int64_t cache_document_was_loaded_from) {
-  if (appcache_service_.get()) {
+  if (appcache_service_) {
     if (!backend_impl_.MarkAsForeignEntry(host_id, document_url,
                                           cache_document_was_loaded_from)) {
       mojo::ReportBadMessage("ACDH_MARK_AS_FOREIGN_ENTRY");
@@ -114,7 +113,7 @@ void AppCacheDispatcherHost::GetResourceList(int32_t host_id,
                                              GetResourceListCallback callback) {
   std::vector<AppCacheResourceInfo> params;
   std::vector<mojom::AppCacheResourceInfoPtr> out;
-  if (appcache_service_.get()) {
+  if (appcache_service_) {
     backend_impl_.GetResourceList(host_id, &params);
 
     // Box up params for output.
@@ -128,7 +127,7 @@ void AppCacheDispatcherHost::GetResourceList(int32_t host_id,
 
 void AppCacheDispatcherHost::GetStatus(int32_t host_id,
                                        GetStatusCallback callback) {
-  if (appcache_service_.get()) {
+  if (appcache_service_) {
     if (backend_impl_.GetStatusWithCallback(host_id, &callback)) {
       return;
     } else {
@@ -140,7 +139,7 @@ void AppCacheDispatcherHost::GetStatus(int32_t host_id,
 
 void AppCacheDispatcherHost::StartUpdate(int32_t host_id,
                                          StartUpdateCallback callback) {
-  if (appcache_service_.get()) {
+  if (appcache_service_) {
     if (backend_impl_.StartUpdateWithCallback(host_id, &callback)) {
       return;
     } else {
@@ -152,7 +151,7 @@ void AppCacheDispatcherHost::StartUpdate(int32_t host_id,
 
 void AppCacheDispatcherHost::SwapCache(int32_t host_id,
                                        SwapCacheCallback callback) {
-  if (appcache_service_.get()) {
+  if (appcache_service_) {
     if (backend_impl_.SwapCacheWithCallback(host_id, &callback)) {
       return;
     } else {

content/browser/appcache/appcache_dispatcher_host.h

@@ -56,7 +56,8 @@ class AppCacheDispatcherHost : public mojom::AppCacheBackend {
   void GetResourceList(int32_t host_id,
                        GetResourceListCallback callback) override;
 
-  scoped_refptr<ChromeAppCacheService> appcache_service_;
+  // This object is owned by the |ChromeAppCacheService|, so this is safe.
+  ChromeAppCacheService* appcache_service_;
   AppCacheFrontendProxy frontend_proxy_;
   AppCacheBackendImpl backend_impl_;
 

content/browser/appcache/chrome_appcache_service.cc

@@ -43,6 +43,16 @@ void ChromeAppCacheService::InitializeOnIOThread(
   set_special_storage_policy(special_storage_policy.get());
 }
 
+void ChromeAppCacheService::Bind(
+    std::unique_ptr<mojom::AppCacheBackend> backend,
+    mojom::AppCacheBackendRequest request) {
+  bindings_.AddBinding(std::move(backend), std::move(request));
+}
+
+void ChromeAppCacheService::Shutdown() {
+  bindings_.CloseAllBindings();
+}
+
 bool ChromeAppCacheService::CanLoadAppCache(const GURL& manifest_url,
                                             const GURL& first_party) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);

content/browser/appcache/chrome_appcache_service.h

@@ -11,7 +11,9 @@
 #include "base/sequenced_task_runner_helpers.h"
 #include "content/browser/appcache/appcache_policy.h"
 #include "content/browser/appcache/appcache_service_impl.h"
+#include "content/common/appcache.mojom.h"
 #include "content/common/content_export.h"
+#include "mojo/public/cpp/bindings/strong_binding_set.h"
 #include "storage/browser/quota/special_storage_policy.h"
 
 namespace base {
@@ -53,6 +55,11 @@ class CONTENT_EXPORT ChromeAppCacheService
       net::URLRequestContextGetter* request_context_getter,
       scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy);
 
+  void Bind(std::unique_ptr<mojom::AppCacheBackend> backend,
+            mojom::AppCacheBackendRequest request);
+
+  void Shutdown();
+
   // AppCachePolicy overrides
   bool CanLoadAppCache(const GURL& manifest_url,
                        const GURL& first_party) override;
@@ -72,6 +79,7 @@ class CONTENT_EXPORT ChromeAppCacheService
 
   ResourceContext* resource_context_;
   base::FilePath cache_path_;
+  mojo::StrongBindingSet<mojom::AppCacheBackend> bindings_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeAppCacheService);
 };

content/browser/storage_partition_impl.cc

@@ -511,6 +511,12 @@ StoragePartitionImpl::~StoragePartitionImpl() {
   if (GetPaymentAppContext())
     GetPaymentAppContext()->Shutdown();
 
+  if (GetAppCacheService()) {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::BindOnce(&ChromeAppCacheService::Shutdown, appcache_service_));
+  }
+
   BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE,
                             std::move(network_context_owner_));
 }