weblayer: Remove profile data before deletion

Commit writes and remove data before deleting the entire profile
directory. There are already no WebContents associated with the profile.
This is all to avoid racing with code that tries to write the profile
during deletion.

Bug: 1065585
Change-Id: Ic77fde85bccb7c0f79824bfc491ea6994dffdcab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2135053Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756869}

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/ProfileTest.java

@@ -12,7 +12,6 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import org.chromium.base.test.util.CallbackHelper;
-import org.chromium.base.test.util.DisabledTest;
 import org.chromium.content_public.browser.test.util.Criteria;
 import org.chromium.content_public.browser.test.util.CriteriaHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
@@ -120,7 +119,6 @@ public class ProfileTest {
 
     @Test
     @SmallTest
-    @DisabledTest
     public void testEnumerateAllProfileNames() throws Exception {
         final String profileName = "TestEnumerateAllProfileNames";
         final WebLayer weblayer = mActivityTestRule.getWebLayer();

weblayer/browser/java/org/chromium/weblayer_private/ProfileImpl.java

@@ -56,11 +56,7 @@ public final class ProfileImpl extends IProfile.Stub {
         mDownloadCallbackProxy = new DownloadCallbackProxy(mName, mNativeProfile);
     }
 
-    @Override
-    public void destroy() {
-        StrictModeWorkaround.apply();
-        if (mBeingDeleted) return;
-
+    private void destroyDependentJavaObjects() {
         if (mDownloadCallbackProxy != null) {
             mDownloadCallbackProxy.destroy();
             mDownloadCallbackProxy = null;
@@ -70,7 +66,14 @@ public final class ProfileImpl extends IProfile.Stub {
             mCookieManager.destroy();
             mCookieManager = null;
         }
+    }
+
+    @Override
+    public void destroy() {
+        StrictModeWorkaround.apply();
+        if (mBeingDeleted) return;
 
+        destroyDependentJavaObjects();
         deleteNativeProfile();
         maybeRunDestroyCallback();
     }
@@ -90,15 +93,19 @@ public final class ProfileImpl extends IProfile.Stub {
     public void destroyAndDeleteDataFromDisk(IObjectWrapper completionCallback) {
         StrictModeWorkaround.apply();
         checkNotDestroyed();
-        final Runnable callback = ObjectWrapper.unwrap(completionCallback, Runnable.class);
         assert mNativeProfile != 0;
-        mBeingDeleted = ProfileImplJni.get().deleteDataFromDisk(mNativeProfile, () -> {
-            deleteNativeProfile();
-            if (callback != null) callback.run();
-        });
-        if (!mBeingDeleted) {
+        if (ProfileImplJni.get().getNumBrowserImpl(mNativeProfile) > 0) {
             throw new IllegalStateException("Profile still in use: " + mName);
         }
+
+        final Runnable callback = ObjectWrapper.unwrap(completionCallback, Runnable.class);
+
+        mBeingDeleted = true;
+        destroyDependentJavaObjects();
+        ProfileImplJni.get().destroyAndDeleteDataFromDisk(mNativeProfile, () -> {
+            if (callback != null) callback.run();
+        });
+        mNativeProfile = 0;
         maybeRunDestroyCallback();
     }
 
@@ -195,7 +202,8 @@ public final class ProfileImpl extends IProfile.Stub {
         void enumerateAllProfileNames(Callback<String[]> callback);
         long createProfile(String name, ProfileImpl caller);
         void deleteProfile(long profile);
-        boolean deleteDataFromDisk(long nativeProfileImpl, Runnable completionCallback);
+        int getNumBrowserImpl(long nativeProfileImpl);
+        void destroyAndDeleteDataFromDisk(long nativeProfileImpl, Runnable completionCallback);
         void clearBrowsingData(long nativeProfileImpl, @ImplBrowsingDataType int[] dataTypes,
                 long fromMillis, long toMillis, Runnable callback);
         void setDownloadDirectory(long nativeProfileImpl, String directory);

weblayer/browser/profile_impl.cc

@@ -4,6 +4,7 @@
 
 #include "weblayer/browser/profile_impl.h"
 
+#include <memory>
 #include <string>
 #include <utility>
 #include <vector>
@@ -19,7 +20,9 @@
 #include "base/task/thread_pool.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
+#include "components/prefs/pref_service.h"
 #include "components/web_cache/browser/web_cache_manager.h"
+#include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/browsing_data_remover.h"
 #include "content/public/browser/device_service.h"
@@ -229,18 +232,6 @@ void ProfileImpl::DownloadsInitialized() {
 #endif
 }
 
-bool ProfileImpl::DeleteDataFromDisk(base::OnceClosure done_callback) {
-  if (num_browser_impl_ > 0)
-    return false;
-  base::ThreadPool::PostTaskAndReply(
-      FROM_HERE,
-      {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
-       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
-      base::BindOnce(&NukeProfileFromDisk, name_, data_path_),
-      std::move(done_callback));
-  return true;
-}
-
 void ProfileImpl::ClearBrowsingData(
     const std::vector<BrowsingDataType>& data_types,
     base::Time from_time,
@@ -286,6 +277,31 @@ CookieManager* ProfileImpl::GetCookieManager() {
   return cookie_manager_.get();
 }
 
+// static
+void ProfileImpl::NukeDataAfterRemovingData(
+    std::unique_ptr<ProfileImpl> profile,
+    base::OnceClosure done_callback) {
+  // Need PostTask to avoid reentrancy for deleting |browser_context_|.
+  base::PostTask(FROM_HERE, {content::BrowserThread::UI},
+                 base::BindOnce(&ProfileImpl::DoNukeData, std::move(profile),
+                                std::move(done_callback)));
+}
+
+// static
+void ProfileImpl::DoNukeData(std::unique_ptr<ProfileImpl> profile,
+                             base::OnceClosure done_callback) {
+  std::string name = profile->name_;
+  base::FilePath data_path = profile->data_path_;
+
+  profile.reset();
+  base::ThreadPool::PostTaskAndReply(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
+      base::BindOnce(&NukeProfileFromDisk, name, data_path),
+      std::move(done_callback));
+}
+
 void ProfileImpl::ClearRendererCache() {
   for (content::RenderProcessHost::iterator iter =
            content::RenderProcessHost::AllHostsIterator();
@@ -311,10 +327,44 @@ void ProfileImpl::OnLocaleChanged() {
           i18n::GetAcceptLangs()));
 }
 
+// static
 std::unique_ptr<Profile> Profile::Create(const std::string& name) {
   return std::make_unique<ProfileImpl>(name);
 }
 
+// static
+std::unique_ptr<Profile> Profile::DestroyAndDeleteDataFromDisk(
+    std::unique_ptr<Profile> profile,
+    base::OnceClosure done_callback) {
+  std::unique_ptr<ProfileImpl> impl(
+      static_cast<ProfileImpl*>(profile.release()));
+  return ProfileImpl::DestroyAndDeleteDataFromDisk(std::move(impl),
+                                                   std::move(done_callback));
+}
+
+// static
+std::unique_ptr<ProfileImpl> ProfileImpl::DestroyAndDeleteDataFromDisk(
+    std::unique_ptr<ProfileImpl> profile,
+    base::OnceClosure done_callback) {
+  if (profile->num_browser_impl_ > 0)
+    return profile;
+
+  // Try to finish all writes and remove all data before nuking the profile.
+  static_cast<BrowserContextImpl*>(profile->GetBrowserContext())
+      ->pref_service()
+      ->CommitPendingWrite();
+
+  // Unretained is safe here because DataClearer is owned by
+  // BrowserContextImpl which is owned by this.
+  auto* clearer = new DataClearer(
+      profile->GetBrowserContext(),
+      base::BindOnce(&ProfileImpl::NukeDataAfterRemovingData,
+                     std::move(profile), std::move(done_callback)));
+  int remove_all_mask = 0x8fffffff;
+  clearer->ClearData(remove_all_mask, base::Time::Min(), base::Time::Max());
+  return nullptr;
+}
+
 #if defined(OS_ANDROID)
 ProfileImpl::ProfileImpl(
     JNIEnv* env,
@@ -347,12 +397,21 @@ static void JNI_ProfileImpl_EnumerateAllProfileNames(
                      base::android::ScopedJavaGlobalRef<jobject>(callback)));
 }
 
-jboolean ProfileImpl::DeleteDataFromDisk(
+jint ProfileImpl::GetNumBrowserImpl(JNIEnv* env) {
+  return num_browser_impl_;
+}
+
+void ProfileImpl::DestroyAndDeleteDataFromDisk(
     JNIEnv* env,
     const base::android::JavaRef<jobject>& j_completion_callback) {
-  return DeleteDataFromDisk(base::BindOnce(
-      &base::android::RunRunnableAndroid,
-      base::android::ScopedJavaGlobalRef<jobject>(j_completion_callback)));
+  std::unique_ptr<ProfileImpl> ptr(this);
+  std::unique_ptr<ProfileImpl> result =
+      ProfileImpl::DestroyAndDeleteDataFromDisk(
+          std::move(ptr),
+          base::BindOnce(&base::android::RunRunnableAndroid,
+                         base::android::ScopedJavaGlobalRef<jobject>(
+                             j_completion_callback)));
+  CHECK(!result);
 }
 
 void ProfileImpl::ClearBrowsingData(

weblayer/browser/profile_impl.h

@@ -34,6 +34,10 @@ class ProfileImpl : public Profile {
   // |context| must not be null.
   static base::FilePath GetCachePath(content::BrowserContext* context);
 
+  static std::unique_ptr<ProfileImpl> DestroyAndDeleteDataFromDisk(
+      std::unique_ptr<ProfileImpl> profile,
+      base::OnceClosure done_callback);
+
   explicit ProfileImpl(const std::string& name);
   ~ProfileImpl() override;
 
@@ -53,7 +57,6 @@ class ProfileImpl : public Profile {
   DownloadDelegate* download_delegate() { return download_delegate_; }
 
   // Profile implementation:
-  bool DeleteDataFromDisk(base::OnceClosure done_callback) override;
   void ClearBrowsingData(const std::vector<BrowsingDataType>& data_types,
                          base::Time from_time,
                          base::Time to_time,
@@ -67,7 +70,8 @@ class ProfileImpl : public Profile {
               const base::android::JavaParamRef<jstring>& path,
               const base::android::JavaParamRef<jobject>& java_profile);
 
-  jboolean DeleteDataFromDisk(
+  jint GetNumBrowserImpl(JNIEnv* env);
+  void DestroyAndDeleteDataFromDisk(
       JNIEnv* env,
       const base::android::JavaRef<jobject>& j_completion_callback);
   void ClearBrowsingData(
@@ -94,6 +98,10 @@ class ProfileImpl : public Profile {
  private:
   class DataClearer;
 
+  static void NukeDataAfterRemovingData(std::unique_ptr<ProfileImpl> profile,
+                                        base::OnceClosure done_callback);
+  static void DoNukeData(std::unique_ptr<ProfileImpl> profile,
+                         base::OnceClosure done_callback);
   void ClearRendererCache();
 
   // Callback when the system locale has been updated.

weblayer/public/profile.h

@@ -30,12 +30,15 @@ class Profile {
   // underscore.
   static std::unique_ptr<Profile> Create(const std::string& name);
 
-  virtual ~Profile() {}
-
   // Delete all profile's data from disk. If there are any existing usage
-  // of this profile, return false immediately and |done_callback| will not
-  // be called. Otherwise |done_callback| is called when deletion is complete.
-  virtual bool DeleteDataFromDisk(base::OnceClosure done_callback) = 0;
+  // of this profile, return |profile| immediately and |done_callback| will not
+  // be called. Otherwise return nullptr and |done_callback| is called when
+  // deletion is complete.
+  static std::unique_ptr<Profile> DestroyAndDeleteDataFromDisk(
+      std::unique_ptr<Profile> profile,
+      base::OnceClosure done_callback);
+
+  virtual ~Profile() {}
 
   virtual void ClearBrowsingData(
       const std::vector<BrowsingDataType>& data_types,