Shutdown StoragePartition before ProfileIOData is being shut down

- Expose a new ShutdownStoragePartitions method on BrowserContext - Calls the new method in the subclasses of BrowserContext before actual destruction / shutdown starts Content modules that are hung on StoragePartition often try to dereference ResourceContext (which is owned by ProfileIOData in chromium cases) even after ResourceContext is destroyed, so it could easily lead to U-A-F. This happens because StoragePartitions are destroyed in BrowserContext's dtor, while ProfileIOData and any other objects that are managed/owned by BrowserContext's subclasses are usually destroyed earlier than that (i.e. their destructors run before the base class's one). This change tries to add a new explicit method, BrowserContext::ShutdownStoragePartitions, and let embedders call it before the embedders are going to destroy/shutdown their own objects (which include ResourceContext), so that content modules can safely assume they can dereference those objects while its StoragePartition's still there. BUG=529896 Review-Url: https://codereview.chromium.org/2159133002 Cr-Commit-Position: refs/heads/master@{#407781}
Denger-Network · Jul 26, 2016 · f6ed359 · f6ed359
1 parent 34a048b
commit f6ed359
Show file tree

Hide file tree

Showing 17 changed files with 53 additions and 53 deletions.
diff --git a/blimp/engine/common/blimp_browser_context.cc b/blimp/engine/common/blimp_browser_context.cc
@@ -80,6 +80,7 @@ BlimpBrowserContext::BlimpBrowserContext(bool off_the_record,
 }
 
 BlimpBrowserContext::~BlimpBrowserContext() {
+  ShutdownStoragePartitions();
   if (resource_context_) {
     content::BrowserThread::DeleteSoon(content::BrowserThread::IO, FROM_HERE,
                                        resource_context_.release());

diff --git a/chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc b/chrome/browser/browsing_data/browsing_data_appcache_helper_unittest.cc
@@ -12,6 +12,7 @@
 #include "base/stl_util.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -43,18 +44,24 @@ class CannedBrowsingDataAppCacheHelperTest : public testing::Test {
   CannedBrowsingDataAppCacheHelperTest()
       : thread_bundle_(content::TestBrowserThreadBundle::REAL_IO_THREAD) {}
 
+  void TearDown() override {
+    // Make sure we run all pending tasks on IO thread before testing
+    // profile is destructed.
+    content::RunAllPendingInMessageLoop(content::BrowserThread::IO);
+  }
+
+ protected:
   content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
 };
 
 TEST_F(CannedBrowsingDataAppCacheHelperTest, SetInfo) {
-  TestingProfile profile;
-
   GURL manifest1("http://example1.com/manifest.xml");
   GURL manifest2("http://example2.com/path1/manifest.xml");
   GURL manifest3("http://example2.com/path2/manifest.xml");
 
   scoped_refptr<CannedBrowsingDataAppCacheHelper> helper(
-      new CannedBrowsingDataAppCacheHelper(&profile));
+      new CannedBrowsingDataAppCacheHelper(&profile_));
   helper->AddAppCache(manifest1);
   helper->AddAppCache(manifest2);
   helper->AddAppCache(manifest3);
@@ -82,12 +89,10 @@ TEST_F(CannedBrowsingDataAppCacheHelperTest, SetInfo) {
 }
 
 TEST_F(CannedBrowsingDataAppCacheHelperTest, Unique) {
-  TestingProfile profile;
-
   GURL manifest("http://example.com/manifest.xml");
 
   scoped_refptr<CannedBrowsingDataAppCacheHelper> helper(
-      new CannedBrowsingDataAppCacheHelper(&profile));
+      new CannedBrowsingDataAppCacheHelper(&profile_));
   helper->AddAppCache(manifest);
   helper->AddAppCache(manifest);
 
@@ -106,12 +111,10 @@ TEST_F(CannedBrowsingDataAppCacheHelperTest, Unique) {
 }
 
 TEST_F(CannedBrowsingDataAppCacheHelperTest, Empty) {
-  TestingProfile profile;
-
   GURL manifest("http://example.com/manifest.xml");
 
   scoped_refptr<CannedBrowsingDataAppCacheHelper> helper(
-      new CannedBrowsingDataAppCacheHelper(&profile));
+      new CannedBrowsingDataAppCacheHelper(&profile_));
 
   ASSERT_TRUE(helper->empty());
   helper->AddAppCache(manifest);
@@ -121,14 +124,12 @@ TEST_F(CannedBrowsingDataAppCacheHelperTest, Empty) {
 }
 
 TEST_F(CannedBrowsingDataAppCacheHelperTest, Delete) {
-  TestingProfile profile;
-
   GURL manifest1("http://example.com/manifest1.xml");
   GURL manifest2("http://foo.example.com/manifest2.xml");
   GURL manifest3("http://bar.example.com/manifest3.xml");
 
   scoped_refptr<CannedBrowsingDataAppCacheHelper> helper(
-      new CannedBrowsingDataAppCacheHelper(&profile));
+      new CannedBrowsingDataAppCacheHelper(&profile_));
 
   EXPECT_TRUE(helper->empty());
   helper->AddAppCache(manifest1);
@@ -142,13 +143,11 @@ TEST_F(CannedBrowsingDataAppCacheHelperTest, Delete) {
 }
 
 TEST_F(CannedBrowsingDataAppCacheHelperTest, IgnoreExtensionsAndDevTools) {
-  TestingProfile profile;
-
   GURL manifest1("chrome-extension://abcdefghijklmnopqrstuvwxyz/manifest.xml");
   GURL manifest2("chrome-devtools://abcdefghijklmnopqrstuvwxyz/manifest.xml");
 
   scoped_refptr<CannedBrowsingDataAppCacheHelper> helper(
-      new CannedBrowsingDataAppCacheHelper(&profile));
+      new CannedBrowsingDataAppCacheHelper(&profile_));
 
   ASSERT_TRUE(helper->empty());
   helper->AddAppCache(manifest1);

diff --git a/chrome/browser/profiles/off_the_record_profile_impl.cc b/chrome/browser/profiles/off_the_record_profile_impl.cc
@@ -193,6 +193,10 @@ OffTheRecordProfileImpl::~OffTheRecordProfileImpl() {
   // Clears any data the network stack contains that may be related to the
   // OTR session.
   g_browser_process->io_thread()->ChangedToOnTheRecord();
+
+  // This must be called before ProfileIOData::ShutdownOnUIThread but after
+  // other profile-related destroy notifications are dispatched.
+  ShutdownStoragePartitions();
 }
 
 void OffTheRecordProfileImpl::InitIoData() {

diff --git a/chrome/browser/profiles/off_the_record_profile_io_data.h b/chrome/browser/profiles/off_the_record_profile_io_data.h
@@ -108,8 +108,6 @@ class OffTheRecordProfileIOData : public ProfileIOData {
   };
 
  private:
-  friend class base::RefCountedThreadSafe<OffTheRecordProfileIOData>;
-
   explicit OffTheRecordProfileIOData(Profile::ProfileType profile_type);
   ~OffTheRecordProfileIOData() override;
 

diff --git a/chrome/browser/profiles/profile_impl.cc b/chrome/browser/profiles/profile_impl.cc
@@ -717,6 +717,10 @@ ProfileImpl::~ProfileImpl() {
   // This causes the Preferences file to be written to disk.
   if (prefs_loaded)
     SetExitType(EXIT_NORMAL);
+
+  // This must be called before ProfileIOData::ShutdownOnUIThread but after
+  // other profile-related destroy notifications are dispatched.
+  ShutdownStoragePartitions();
 }
 
 std::string ProfileImpl::GetProfileUserName() const {

diff --git a/chrome/browser/profiles/profile_impl_io_data.h b/chrome/browser/profiles/profile_impl_io_data.h
@@ -147,8 +147,6 @@ class ProfileImplIOData : public ProfileIOData {
   };
 
  private:
-  friend class base::RefCountedThreadSafe<ProfileImplIOData>;
-
   struct LazyParams {
     LazyParams();
     ~LazyParams();

diff --git a/chrome/test/base/testing_profile.cc b/chrome/test/base/testing_profile.cc
@@ -532,6 +532,11 @@ TestingProfile::~TestingProfile() {
 
   if (pref_proxy_config_tracker_.get())
     pref_proxy_config_tracker_->DetachFromPrefService();
+
+  // Shutdown storage partitions before we post a task to delete
+  // the resource context.
+  ShutdownStoragePartitions();
+
   // Failing a post == leaks == heapcheck failure. Make that an immediate test
   // failure.
   if (resource_context_) {

diff --git a/chromecast/browser/cast_browser_context.cc b/chromecast/browser/cast_browser_context.cc
@@ -57,6 +57,7 @@ CastBrowserContext::CastBrowserContext(
 }
 
 CastBrowserContext::~CastBrowserContext() {
+  ShutdownStoragePartitions();
   content::BrowserThread::DeleteSoon(
       content::BrowserThread::IO,
       FROM_HERE,

diff --git a/content/browser/browser_context.cc b/content/browser/browser_context.cc
@@ -486,10 +486,18 @@ BrowserContext::~BrowserContext() {
       << "Attempting to destroy a BrowserContext that never called "
       << "Initialize()";
 
+  DCHECK(!GetUserData(kStoragePartitionMapKeyName))
+      << "StoragePartitionMap is not shut down properly";
+
   RemoveBrowserContextFromUserIdMap(this);
 
   if (GetUserData(kDownloadManagerKeyName))
     GetDownloadManager(this)->Shutdown();
 }
 
+void BrowserContext::ShutdownStoragePartitions() {
+  if (GetUserData(kStoragePartitionMapKeyName))
+    RemoveUserData(kStoragePartitionMapKeyName);
+}
+
 }  // namespace content
diff --git a/content/browser/service_worker/service_worker_context_wrapper.cc b/content/browser/service_worker/service_worker_context_wrapper.cc
@@ -132,7 +132,6 @@ ServiceWorkerContextWrapper::ServiceWorkerContextWrapper(
 
 ServiceWorkerContextWrapper::~ServiceWorkerContextWrapper() {
   DCHECK(!resource_context_);
-  DCHECK(!request_context_getter_);
 }
 
 void ServiceWorkerContextWrapper::Init(
@@ -163,25 +162,9 @@ void ServiceWorkerContextWrapper::Shutdown() {
 }
 
 void ServiceWorkerContextWrapper::InitializeResourceContext(
-    ResourceContext* resource_context,
-    scoped_refptr<net::URLRequestContextGetter> request_context_getter) {
+    ResourceContext* resource_context) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   resource_context_ = resource_context;
-  request_context_getter_ = request_context_getter;
-  // Can be null in tests.
-  if (request_context_getter_)
-    request_context_getter_->AddObserver(this);
-}
-
-void ServiceWorkerContextWrapper::OnContextShuttingDown() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
-  // OnContextShuttingDown is called when the ProfileIOData (ResourceContext) is
-  // shutting down, so call ShutdownOnIO() to clear resource_context_.
-  // This doesn't seem to be called when using content_shell, so we still must
-  // also call ShutdownOnIO() in Shutdown(), which is called when the storage
-  // partition is destroyed.
-  ShutdownOnIO();
 }
 
 void ServiceWorkerContextWrapper::DeleteAndStartOver() {
@@ -815,10 +798,6 @@ void ServiceWorkerContextWrapper::InitInternal(
 
 void ServiceWorkerContextWrapper::ShutdownOnIO() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  // Can be null in tests.
-  if (request_context_getter_)
-    request_context_getter_->RemoveObserver(this);
-  request_context_getter_ = nullptr;
   resource_context_ = nullptr;
   context_core_.reset();
 }

diff --git a/content/browser/service_worker/service_worker_context_wrapper.h b/content/browser/service_worker/service_worker_context_wrapper.h
@@ -17,7 +17,6 @@
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/service_worker_context.h"
-#include "net/url_request/url_request_context_getter_observer.h"
 
 namespace base {
 class FilePath;
@@ -48,7 +47,6 @@ class StoragePartitionImpl;
 // is what is used internally in the service worker lib.
 class CONTENT_EXPORT ServiceWorkerContextWrapper
     : NON_EXPORTED_BASE(public ServiceWorkerContext),
-      public net::URLRequestContextGetterObserver,
       public base::RefCountedThreadSafe<ServiceWorkerContextWrapper> {
  public:
   using StatusCallback = base::Callback<void(ServiceWorkerStatusCode)>;
@@ -71,12 +69,7 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
   void Shutdown();
 
   // Must be called on the IO thread.
-  void InitializeResourceContext(
-      ResourceContext* resource_context,
-      scoped_refptr<net::URLRequestContextGetter> request_context_getter);
-
-  // For net::URLRequestContextGetterObserver
-  void OnContextShuttingDown() override;
+  void InitializeResourceContext(ResourceContext* resource_context);
 
   // Deletes all files on disk and restarts the system asynchronously. This
   // leaves the system in a disabled state until it's done. This should be
@@ -277,8 +270,6 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
   // The ResourceContext associated with this context.
   ResourceContext* resource_context_;
 
-  scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
-
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerContextWrapper);
 };
 

diff --git a/content/browser/storage_partition_impl.h b/content/browser/storage_partition_impl.h
@@ -34,6 +34,9 @@ class CONTENT_EXPORT  StoragePartitionImpl
     : public StoragePartition,
       public NON_EXPORTED_BASE(mojom::StoragePartitionService) {
  public:
+  // It is guaranteed that storage partitions are destructed before the
+  // browser context starts shutting down its corresponding IO thread residents
+  // (e.g. resource context).
   ~StoragePartitionImpl() override;
 
   // Quota managed data uses a different bitmask for types than

diff --git a/content/browser/storage_partition_impl_map.cc b/content/browser/storage_partition_impl_map.cc
@@ -602,8 +602,7 @@ void StoragePartitionImplMap::PostCreateInitialization(
         BrowserThread::IO, FROM_HERE,
         base::Bind(&ServiceWorkerContextWrapper::InitializeResourceContext,
                    partition->GetServiceWorkerContext(),
-                   browser_context_->GetResourceContext(),
-                   base::RetainedRef(partition->GetURLRequestContext())));
+                   browser_context_->GetResourceContext()));
 
     // We do not call InitializeURLRequestContext() for media contexts because,
     // other than the HTTP cache, the media contexts share the same backing

diff --git a/content/public/browser/browser_context.h b/content/public/browser/browser_context.h
@@ -169,6 +169,13 @@ class CONTENT_EXPORT BrowserContext : public base::SupportsUserData {
 
   ~BrowserContext() override;
 
+  // Shuts down the storage partitions associated to this browser context.
+  // This must be called before the browser context is actually destroyed
+  // and before a clean-up task for its corresponding IO thread residents (e.g.
+  // ResourceContext) is posted, so that the classes that hung on
+  // StoragePartition can have time to do necessary cleanups on IO thread.
+  void ShutdownStoragePartitions();
+
   // Creates a delegate to initialize a HostZoomMap and persist its information.
   // This is called during creation of each StoragePartition.
   virtual std::unique_ptr<ZoomLevelDelegate> CreateZoomLevelDelegate(

diff --git a/content/public/test/test_browser_context.cc b/content/public/test/test_browser_context.cc
@@ -50,6 +50,7 @@ TestBrowserContext::TestBrowserContext() {
 }
 
 TestBrowserContext::~TestBrowserContext() {
+  ShutdownStoragePartitions();
 }
 
 base::FilePath TestBrowserContext::TakePath() {

diff --git a/content/shell/browser/shell_browser_context.cc b/content/shell/browser/shell_browser_context.cc
@@ -63,6 +63,7 @@ ShellBrowserContext::ShellBrowserContext(bool off_the_record,
 }
 
 ShellBrowserContext::~ShellBrowserContext() {
+  ShutdownStoragePartitions();
   if (resource_context_) {
     BrowserThread::DeleteSoon(
       BrowserThread::IO, FROM_HERE, resource_context_.release());

diff --git a/headless/lib/browser/headless_browser_context_impl.cc b/headless/lib/browser/headless_browser_context_impl.cc
@@ -76,6 +76,7 @@ HeadlessBrowserContextImpl::HeadlessBrowserContextImpl(
 }
 
 HeadlessBrowserContextImpl::~HeadlessBrowserContextImpl() {
+  ShutdownStoragePartitions();
   if (resource_context_) {
     content::BrowserThread::DeleteSoon(content::BrowserThread::IO, FROM_HERE,
                                        resource_context_.release());