weblayer: Mark profile for deletion

It is unsafe and generally not possible to delete the profile data of a
profile that has already been "loaded". The only safe implementation is
to mark the profile data as deleted, wait until the next time browser
process starts, ensure the profile data is not loaded, and find a time
to delete the data. This CL implements this for weblayer.

* Deletion marker are empty files in <data>/profiles_to_delete matching
  the directory names of profiles. If <data>/profiles_to_delete/foo
  exists, then <data>/profiles/foo is marked for deletion.
* If client create with a name (say "foo") matching a deleted profile,
  will try to linearly check for directory with a suffix (eg "foo.1",
  "foo.2", etc) until a directory that is not marked for deletion is
  found. "." is not part of a valid profile name, so there is no risk of
  collision.
* Now DestroyAndDeleteDataFromDisk will mark the profile for deletion,
  and only remove data with BrowsingDataRemover. It will not attempt to
  delete the profile directory.
* Nuke and Mark are run in the same SequencedTaskRunner to ensure Nuke
  is called first in a process before any mark. This ensures Nuke will
  never remove a newly marked Profile during the current process's
  lifetime.
* Nuke will delete the data and cache directories first, and only remove
  the marker if both succeeds. This ensures there is no race in reusing
  a profile path.

Add weblayer_unittests and add some unit tests for
profile_disk_operations.

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

BUILD.gn

@@ -202,6 +202,7 @@ group("gn_all") {
     deps += [
       "//weblayer/shell:weblayer_shell",
       "//weblayer/test:weblayer_browsertests",
+      "//weblayer/test:weblayer_unittests",
     ]
     if (is_android) {
       deps += [ "//weblayer/browser/android/javatests:weblayer_instrumentation_test_apk" ]

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

@@ -158,29 +158,11 @@ public class CookieManagerTest {
     }
 
     private boolean setCookie(String value) throws Exception {
-        Boolean[] resultHolder = new Boolean[1];
-        CallbackHelper callbackHelper = new CallbackHelper();
-        TestThreadUtils.runOnUiThreadBlocking(() -> {
-            mCookieManager.setCookie(mBaseUri, value, (Boolean result) -> {
-                resultHolder[0] = result;
-                callbackHelper.notifyCalled();
-            });
-        });
-        callbackHelper.waitForFirst();
-        return resultHolder[0];
+        return mActivityTestRule.setCookie(mCookieManager, mBaseUri, value);
     }
 
     private String getCookie() throws Exception {
-        String[] resultHolder = new String[1];
-        CallbackHelper callbackHelper = new CallbackHelper();
-        TestThreadUtils.runOnUiThreadBlocking(() -> {
-            mCookieManager.getCookie(mBaseUri, (String result) -> {
-                resultHolder[0] = result;
-                callbackHelper.notifyCalled();
-            });
-        });
-        callbackHelper.waitForFirst();
-        return resultHolder[0];
+        return mActivityTestRule.getCookie(mCookieManager, mBaseUri);
     }
 
     private static class CookieChangedCallbackHelper extends CookieChangedCallback {

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

@@ -9,6 +9,7 @@ import android.app.Instrumentation.ActivityMonitor;
 import android.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
+import android.net.Uri;
 import android.os.Bundle;
 import android.support.test.InstrumentationRegistry;
 import android.support.test.rule.ActivityTestRule;
@@ -29,6 +30,7 @@ import org.chromium.content_public.browser.test.util.CriteriaHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.net.test.EmbeddedTestServer;
 import org.chromium.net.test.EmbeddedTestServerRule;
+import org.chromium.weblayer.CookieManager;
 import org.chromium.weblayer.NavigationController;
 import org.chromium.weblayer.Tab;
 import org.chromium.weblayer.WebLayer;
@@ -284,4 +286,30 @@ public class InstrumentationActivityTestRule extends ActivityTestRule<Instrument
     public Fragment getFragment() {
         return TestThreadUtils.runOnUiThreadBlockingNoException(() -> getActivity().getFragment());
     }
+
+    public boolean setCookie(CookieManager cookieManager, Uri uri, String value) throws Exception {
+        Boolean[] resultHolder = new Boolean[1];
+        CallbackHelper callbackHelper = new CallbackHelper();
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            cookieManager.setCookie(uri, value, (Boolean result) -> {
+                resultHolder[0] = result;
+                callbackHelper.notifyCalled();
+            });
+        });
+        callbackHelper.waitForFirst();
+        return resultHolder[0];
+    }
+
+    public String getCookie(CookieManager cookieManager, Uri uri) throws Exception {
+        String[] resultHolder = new String[1];
+        CallbackHelper callbackHelper = new CallbackHelper();
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            cookieManager.getCookie(uri, (String result) -> {
+                resultHolder[0] = result;
+                callbackHelper.notifyCalled();
+            });
+        });
+        callbackHelper.waitForFirst();
+        return resultHolder[0];
+    }
 }

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

@@ -4,7 +4,9 @@
 
 package org.chromium.weblayer.test;
 
+import android.net.Uri;
 import android.support.test.filters.SmallTest;
+import android.webkit.ValueCallback;
 
 import org.junit.Assert;
 import org.junit.Rule;
@@ -16,6 +18,7 @@ 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;
 import org.chromium.weblayer.Callback;
+import org.chromium.weblayer.CookieManager;
 import org.chromium.weblayer.Profile;
 import org.chromium.weblayer.WebLayer;
 import org.chromium.weblayer.shell.InstrumentationActivity;
@@ -143,6 +146,88 @@ public class ProfileTest {
         Assert.assertFalse(Arrays.asList(enumerateAllProfileNames(weblayer)).contains(profileName));
     }
 
+    private Profile launchAndDestroyActivity(
+            String profileName, ValueCallback<InstrumentationActivity> callback) {
+        final InstrumentationActivity activity = mActivityTestRule.launchWithProfile(profileName);
+        final Profile profile = TestThreadUtils.runOnUiThreadBlockingNoException(
+                () -> activity.getBrowser().getProfile());
+
+        callback.onReceiveValue(activity);
+
+        TestThreadUtils.runOnUiThreadBlocking(() -> activity.finish());
+        CriteriaHelper.pollUiThread(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                return activity.isDestroyed();
+            }
+        });
+        return profile;
+    }
+
+    private void destroyAndDeleteDataFromDisk(Profile profile) throws Exception {
+        final CallbackHelper callbackHelper = new CallbackHelper();
+        TestThreadUtils.runOnUiThreadBlocking(
+                () -> profile.destroyAndDeleteDataFromDisk(callbackHelper::notifyCalled));
+        callbackHelper.waitForFirst();
+    }
+
+    @Test
+    @SmallTest
+    public void testReuseProfile() throws Exception {
+        final String profileName = "ReusedProfile";
+        final Uri uri = Uri.parse("https://foo.bar");
+        final String expectedCookie = "foo=bar";
+        {
+            // Create profile and activity and set a cookie.
+            launchAndDestroyActivity(profileName, (InstrumentationActivity activity) -> {
+                CookieManager cookieManager = TestThreadUtils.runOnUiThreadBlockingNoException(
+                        () -> { return activity.getBrowser().getProfile().getCookieManager(); });
+                try {
+                    boolean result =
+                            mActivityTestRule.setCookie(cookieManager, uri, expectedCookie);
+                    Assert.assertTrue(result);
+                } catch (Exception e) {
+                    throw new RuntimeException(e);
+                }
+            });
+        }
+
+        {
+            // Without deleting proifle data, create profile and activity again, and check the
+            // cookie is there.
+            Profile profile =
+                    launchAndDestroyActivity(profileName, (InstrumentationActivity activity) -> {
+                        CookieManager cookieManager =
+                                TestThreadUtils.runOnUiThreadBlockingNoException(() -> {
+                                    return activity.getBrowser().getProfile().getCookieManager();
+                                });
+                        try {
+                            String cookie = mActivityTestRule.getCookie(cookieManager, uri);
+                            Assert.assertEquals(expectedCookie, cookie);
+                        } catch (Exception e) {
+                            throw new RuntimeException(e);
+                        }
+                    });
+
+            destroyAndDeleteDataFromDisk(profile);
+        }
+
+        {
+            // After deleting profile data, create profile and activity again, and check the cookie
+            // is deleted as well.
+            launchAndDestroyActivity(profileName, (InstrumentationActivity activity) -> {
+                CookieManager cookieManager = TestThreadUtils.runOnUiThreadBlockingNoException(
+                        () -> { return activity.getBrowser().getProfile().getCookieManager(); });
+                try {
+                    String cookie = mActivityTestRule.getCookie(cookieManager, uri);
+                    Assert.assertEquals("", cookie);
+                } catch (Exception e) {
+                    throw new RuntimeException(e);
+                }
+            });
+        }
+    }
+
     private static String[] enumerateAllProfileNames(final WebLayer weblayer) throws Exception {
         final String[][] holder = new String[1][];
         final CallbackHelper callbackHelper = new CallbackHelper();

weblayer/browser/profile_disk_operations.cc

@@ -7,20 +7,31 @@
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/path_service.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "build/build_config.h"
 #include "weblayer/common/weblayer_paths.h"
 
 namespace weblayer {
 
+// Variables named |name| is a string passed in from the embedder to identify a
+// profile. It can only contain alphanumeric and underscore.
+//
+// Variables named |dir_name| generally refers to the directory name of the
+// profile. It may be the |name| exactly, or it may be <name>.<number>, if a
+// profile is created with a name matching a profile marked for deletion.
+// |dir_name| is an implementation detail of this file and should not be exposed
+// as a concept out of this file.
+
 namespace {
 
-bool IsNameValid(const std::string& name) {
-  for (char c : name) {
-    if (!(base::IsAsciiDigit(c) || base::IsAsciiAlpha(c) || c == '_'))
-      return false;
-  }
-  return true;
+// Cannot be part of a valid name. This prevents the client ever specifying a
+// name that collides a different one with a suffix.
+constexpr char kSuffixDelimiter = '.';
+
+bool IsValidProfileNameChar(char c) {
+  return base::IsAsciiDigit(c) || base::IsAsciiAlpha(c) || c == '_';
 }
 
 // Return the data path directory to profiles.
@@ -30,22 +41,48 @@ base::FilePath GetProfileRootDataDir() {
   return path.AppendASCII("profiles");
 }
 
+base::FilePath GetProfileMarkerRootDataDir() {
+  base::FilePath path;
+  CHECK(base::PathService::Get(DIR_USER_DATA, &path));
+  path = path.AppendASCII("profiles_to_delete");
+  base::CreateDirectory(path);
+  return path;
+}
+
+base::FilePath GetDataPathFromDirName(const std::string& dir_name) {
+  return GetProfileRootDataDir().AppendASCII(dir_name.c_str());
+}
+
+#if defined(OS_POSIX)
+base::FilePath GetCachePathFromDirName(const std::string& dir_name) {
+  base::FilePath cache_path;
+  CHECK(base::PathService::Get(base::DIR_CACHE, &cache_path));
+  cache_path = cache_path.AppendASCII("profiles").AppendASCII(dir_name.c_str());
+  return cache_path;
+}
+#endif  // OS_POSIX
+
 }  // namespace
 
 ProfileInfo CreateProfileInfo(const std::string& name) {
-  CHECK(IsNameValid(name));
+  CHECK(internal::IsProfileNameValid(name));
   if (name.empty())
     return {name, base::FilePath(), base::FilePath()};
 
-  base::FilePath data_path = GetProfileRootDataDir().AppendASCII(name.c_str());
-  if (!base::PathExists(data_path))
-    base::CreateDirectory(data_path);
+  std::string dir_name = name;
+  int suffix = 0;
+  while (internal::IsProfileMarkedForDeletion(dir_name)) {
+    suffix++;
+    dir_name = name;
+    dir_name.append(1, kSuffixDelimiter).append(base::NumberToString(suffix));
+  }
+
+  base::FilePath data_path = GetDataPathFromDirName(dir_name);
+  base::CreateDirectory(data_path);
   base::FilePath cache_path = data_path;
 #if defined(OS_POSIX)
-  CHECK(base::PathService::Get(base::DIR_CACHE, &cache_path));
-  cache_path = cache_path.AppendASCII("profiles").AppendASCII(name.c_str());
-  if (!base::PathExists(cache_path))
-    base::CreateDirectory(cache_path);
+  cache_path = GetCachePathFromDirName(dir_name);
+  base::CreateDirectory(cache_path);
 #endif
   return {name, data_path, cache_path};
 }
@@ -61,13 +98,25 @@ base::FilePath ComputeBrowserPersisterDataBaseDir(const ProfileInfo& info) {
   return base_path;
 }
 
-void NukeProfileFromDisk(const ProfileInfo& info) {
+void MarkProfileAsDeleted(const ProfileInfo& info) {
+  if (info.name.empty())
+    return;
+
+  base::FilePath data_root_path = GetProfileRootDataDir();
+  base::FilePath marker_path = GetProfileMarkerRootDataDir();
+  CHECK(data_root_path.AppendRelativePath(info.data_path, &marker_path));
+  base::File file(marker_path,
+                  base::File::FLAG_CREATE | base::File::FLAG_WRITE);
+}
+
+void TryNukeProfileFromDisk(const ProfileInfo& info) {
   if (info.name.empty()) {
     // Incognito. Just delete session data.
     base::DeleteFileRecursively(ComputeBrowserPersisterDataBaseDir(info));
     return;
   }
 
+  // This may fail, but that is ok since the marker is not deleted.
   base::DeleteFileRecursively(info.data_path);
 #if defined(OS_POSIX)
   base::DeleteFileRecursively(info.cache_path);
@@ -81,11 +130,80 @@ std::vector<std::string> ListProfileNames() {
                                   base::FileEnumerator::DIRECTORIES);
   for (base::FilePath path = enumerator.Next(); !path.empty();
        path = enumerator.Next()) {
-    std::string name = enumerator.GetInfo().GetName().MaybeAsASCII();
-    if (IsNameValid(name))
+    std::string dir_name = enumerator.GetInfo().GetName().MaybeAsASCII();
+    std::string name = internal::CheckDirNameAndExtractName(dir_name);
+    if (!name.empty() && !internal::IsProfileMarkedForDeletion(dir_name))
       profile_names.push_back(name);
   }
   return profile_names;
 }
 
+void NukeProfilesMarkedForDeletion() {
+  base::FilePath marker_root_dir = GetProfileMarkerRootDataDir();
+  base::FileEnumerator enumerator(marker_root_dir, /*recursive=*/false,
+                                  base::FileEnumerator::FILES);
+  for (base::FilePath marker_path = enumerator.Next(); !marker_path.empty();
+       marker_path = enumerator.Next()) {
+    std::string dir_name = enumerator.GetInfo().GetName().MaybeAsASCII();
+    if (!internal::CheckDirNameAndExtractName(dir_name).empty()) {
+      // Delete cache and data directory first before deleting marker.
+      bool delete_success = true;
+#if defined(OS_POSIX)
+      delete_success |=
+          base::DeleteFileRecursively(GetCachePathFromDirName(dir_name));
+#endif  // OS_POSIX
+      delete_success |=
+          base::DeleteFileRecursively(GetDataPathFromDirName(dir_name));
+      // Only delete the marker if deletion is successful.
+      if (delete_success) {
+        base::DeleteFile(marker_path, /*recursive=*/false);
+      }
+    }
+  }
+}
+
+namespace internal {
+
+// Note empty name is valid for the incognito profile.
+bool IsProfileNameValid(const std::string& name) {
+  for (char c : name) {
+    if (!IsValidProfileNameChar(c))
+      return false;
+  }
+  return true;
+}
+
+// If |dir_name| is valid, then return the |name|. Otherwise return the empty
+// string.
+std::string CheckDirNameAndExtractName(const std::string& dir_name) {
+  std::vector<std::string> parts =
+      base::SplitString(dir_name, std::string(1, kSuffixDelimiter),
+                        base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
+  if (parts.size() == 0 || parts.size() > 2)
+    return std::string();
+
+  if (!IsProfileNameValid(parts[0]))
+    return std::string();
+
+  if (parts.size() > 1) {
+    if (parts[1].empty())
+      return std::string();
+
+    for (char c : parts[1]) {
+      if (!base::IsAsciiDigit(c))
+        return std::string();
+    }
+  }
+
+  return parts[0];
+}
+
+bool IsProfileMarkedForDeletion(const std::string& dir_name) {
+  base::FilePath marker =
+      GetProfileMarkerRootDataDir().AppendASCII(dir_name.c_str());
+  return base::PathExists(marker);
+}
+
+}  // namespace internal
+
 }  // namespace weblayer

weblayer/browser/profile_disk_operations.h

@@ -26,15 +26,31 @@ struct ProfileInfo {
 };
 
 // |name| must be a valid profile name. Ensures that both data and cache path
-// directories are created.
+// directories are created. The paths returned may be different from the name
+// to avoid reusing directories that are marked as deleted.
 ProfileInfo CreateProfileInfo(const std::string& name);
 
 base::FilePath ComputeBrowserPersisterDataBaseDir(const ProfileInfo& info);
-void NukeProfileFromDisk(const ProfileInfo& info);
+void MarkProfileAsDeleted(const ProfileInfo& info);
+void TryNukeProfileFromDisk(const ProfileInfo& info);
 
 // Return names of profiles on disk. Invalid profile names are ignored.
+// Profiles marked as deleted are ignored.
 std::vector<std::string> ListProfileNames();
 
+// This should be called before any |MarkProfileAsDeleted| for a single process
+// to avoid races.
+void NukeProfilesMarkedForDeletion();
+
+// Functions exposed for testing.
+namespace internal {
+
+bool IsProfileNameValid(const std::string& name);
+std::string CheckDirNameAndExtractName(const std::string& dir_name);
+bool IsProfileMarkedForDeletion(const std::string& dir_name);
+
+}  // namespace internal
+
 }  // namespace weblayer
 
 #endif  // WEBLAYER_BROWSER_PROFILE_DISK_OPERATIONS_H_

weblayer/browser/profile_disk_operations_unittests.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <algorithm>
+#include <string>
+#include <vector>
+
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/logging.h"
+#include "base/path_service.h"
+#include "base/strings/string_number_conversions.h"
+#include "build/build_config.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "weblayer/browser/profile_disk_operations.h"
+#include "weblayer/common/weblayer_paths.h"
+
+namespace weblayer {
+
+class ProfileDiskOperationsTest : public testing::Test {
+ public:
+  void SetUp() override {
+    CHECK(data_temp_dir_.CreateUniqueTempDir());
+    base::PathService::Override(DIR_USER_DATA, data_temp_dir_.GetPath());
+#if defined(OS_POSIX)
+    CHECK(cache_temp_dir_.CreateUniqueTempDir());
+    base::PathService::Override(base::DIR_CACHE, cache_temp_dir_.GetPath());
+#endif
+  }
+
+ protected:
+  base::ScopedTempDir data_temp_dir_;
+#if defined(OS_POSIX)
+  base::ScopedTempDir cache_temp_dir_;
+#endif
+};
+
+TEST_F(ProfileDiskOperationsTest, IsProfileNameValid) {
+  EXPECT_TRUE(internal::IsProfileNameValid("foo"));
+  EXPECT_TRUE(internal::IsProfileNameValid("123"));
+  EXPECT_TRUE(internal::IsProfileNameValid(std::string()));
+
+  EXPECT_FALSE(internal::IsProfileNameValid("foo.bar"));
+  EXPECT_FALSE(internal::IsProfileNameValid("foo~"));
+  EXPECT_FALSE(internal::IsProfileNameValid("foo-"));
+}
+
+TEST_F(ProfileDiskOperationsTest, CheckDirnameAndExtractName) {
+  EXPECT_EQ(std::string("foo123"),
+            internal::CheckDirNameAndExtractName("foo123"));
+  EXPECT_EQ(std::string("foo"), internal::CheckDirNameAndExtractName("foo.1"));
+  EXPECT_EQ(std::string("foo"), internal::CheckDirNameAndExtractName("foo.2"));
+  EXPECT_EQ(std::string("foo"),
+            internal::CheckDirNameAndExtractName("foo.123"));
+
+  EXPECT_EQ(std::string(), internal::CheckDirNameAndExtractName("foo."));
+  EXPECT_EQ(std::string(), internal::CheckDirNameAndExtractName("foo~"));
+  EXPECT_EQ(std::string(), internal::CheckDirNameAndExtractName("foo~.1"));
+  EXPECT_EQ(std::string(), internal::CheckDirNameAndExtractName("foo.bar"));
+  EXPECT_EQ(std::string(), internal::CheckDirNameAndExtractName("foo.1.2"));
+  EXPECT_EQ(std::string(), internal::CheckDirNameAndExtractName(std::string()));
+  EXPECT_EQ(std::string(), internal::CheckDirNameAndExtractName(".1"));
+}
+
+TEST_F(ProfileDiskOperationsTest, BasicListProfileNames) {
+  std::vector<std::string> names{"foo", "bar", "baz"};
+  for (const auto& name : names) {
+    ProfileInfo info = CreateProfileInfo(name);
+    EXPECT_FALSE(info.data_path.empty());
+    EXPECT_FALSE(info.cache_path.empty());
+  }
+  std::vector<std::string> listed_names = ListProfileNames();
+  EXPECT_EQ(names.size(), listed_names.size());
+  for (const auto& name : names) {
+    auto itr = std::find(listed_names.begin(), listed_names.end(), name);
+    EXPECT_TRUE(itr != listed_names.end());
+  }
+}
+
+TEST_F(ProfileDiskOperationsTest, MarkProfileAsDeleted) {
+  std::vector<std::string> names{"foo", "bar", "baz"};
+  std::vector<ProfileInfo> infos;
+  for (const auto& name : names) {
+    ProfileInfo info = CreateProfileInfo(name);
+    infos.push_back(info);
+    EXPECT_FALSE(info.data_path.empty());
+    EXPECT_FALSE(info.cache_path.empty());
+  }
+  for (const auto& info : infos) {
+    MarkProfileAsDeleted(info);
+    EXPECT_TRUE(internal::IsProfileMarkedForDeletion(
+        info.data_path.BaseName().MaybeAsASCII()));
+  }
+  std::vector<std::string> listed_names = ListProfileNames();
+  EXPECT_TRUE(listed_names.empty());
+}
+
+TEST_F(ProfileDiskOperationsTest, ReuseProfileName) {
+  constexpr int kRepeat = 3;
+  for (int i = 0; i < kRepeat; ++i) {
+    ProfileInfo info = CreateProfileInfo("foo");
+    MarkProfileAsDeleted(info);
+    EXPECT_TRUE(internal::IsProfileMarkedForDeletion(
+        info.data_path.BaseName().MaybeAsASCII()));
+
+    std::string expected_base_name("foo");
+    if (i != 0) {
+      expected_base_name += ".";
+      expected_base_name += base::NumberToString(i);
+    }
+    EXPECT_EQ(expected_base_name, info.data_path.BaseName().MaybeAsASCII());
+    EXPECT_EQ(expected_base_name, info.cache_path.BaseName().MaybeAsASCII());
+
+    std::vector<std::string> listed_names = ListProfileNames();
+    EXPECT_TRUE(listed_names.empty());
+  }
+}
+
+TEST_F(ProfileDiskOperationsTest, NukeProfile) {
+  std::vector<ProfileInfo> deleted_infos;
+  constexpr int kRepeat = 3;
+  for (int i = 0; i < kRepeat; ++i) {
+    ProfileInfo info = CreateProfileInfo("foo");
+    MarkProfileAsDeleted(info);
+    deleted_infos.push_back(info);
+  }
+
+  {
+    ProfileInfo info = CreateProfileInfo("bar");
+    MarkProfileAsDeleted(info);
+    deleted_infos.push_back(info);
+  }
+
+  {
+    ProfileInfo info = CreateProfileInfo("baz");
+    MarkProfileAsDeleted(info);
+    deleted_infos.push_back(info);
+  }
+
+  ProfileInfo kept_info = CreateProfileInfo("kept");
+
+  for (auto& info : deleted_infos) {
+    EXPECT_TRUE(base::PathExists(info.data_path));
+    EXPECT_TRUE(base::PathExists(info.cache_path));
+  }
+  EXPECT_TRUE(base::PathExists(kept_info.data_path));
+  EXPECT_TRUE(base::PathExists(kept_info.cache_path));
+
+  NukeProfilesMarkedForDeletion();
+
+  for (auto& info : deleted_infos) {
+    EXPECT_FALSE(base::PathExists(info.data_path));
+    EXPECT_FALSE(base::PathExists(info.cache_path));
+  }
+  EXPECT_TRUE(base::PathExists(kept_info.data_path));
+  EXPECT_TRUE(base::PathExists(kept_info.cache_path));
+
+  ProfileInfo info = CreateProfileInfo("bar");
+  EXPECT_EQ(std::string("bar"), info.data_path.BaseName().MaybeAsASCII());
+  EXPECT_EQ(std::string("bar"), info.cache_path.BaseName().MaybeAsASCII());
+}
+
+}  // namespace weblayer

weblayer/browser/profile_impl.cc

@@ -49,8 +49,18 @@ namespace weblayer {
 
 namespace {
 
-#if defined(OS_ANDROID)
+bool g_first_profile_created = false;
+
+// TaskRunner used by MarkProfileAsDeleted and NukeProfilesMarkedForDeletion to
+// esnure that Nuke happens before any Mark in this process.
+base::SequencedTaskRunner* GetBackgroundDiskOperationTaskRunner() {
+  static const base::NoDestructor<scoped_refptr<base::SequencedTaskRunner>>
+      task_runner(base::ThreadPool::CreateSingleThreadTaskRunner(
+          {base::MayBlock(), base::TaskPriority::BEST_EFFORT}));
+  return task_runner.get()->get();
+}
 
+#if defined(OS_ANDROID)
 void PassFilePathsToJavaCallback(
     const base::android::ScopedJavaGlobalRef<jobject>& callback,
     const std::vector<std::string>& file_paths) {
@@ -104,6 +114,13 @@ ProfileImpl::ProfileImpl(const std::string& name)
     base::ScopedAllowBlocking allow_blocking;
     info_ = CreateProfileInfo(name);
   }
+
+  if (!g_first_profile_created) {
+    g_first_profile_created = true;
+    GetBackgroundDiskOperationTaskRunner()->PostTask(
+        FROM_HERE, base::BindOnce(&NukeProfilesMarkedForDeletion));
+  }
+
   // Ensure WebCacheManager is created so that it starts observing
   // OnRenderProcessHostCreated events.
   web_cache::WebCacheManager::GetInstance();
@@ -198,13 +215,10 @@ void ProfileImpl::NukeDataAfterRemovingData(
 void ProfileImpl::DoNukeData(std::unique_ptr<ProfileImpl> profile,
                              base::OnceClosure done_callback) {
   ProfileInfo info = profile->info_;
-
   profile.reset();
-  base::ThreadPool::PostTaskAndReply(
-      FROM_HERE,
-      {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
-       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
-      base::BindOnce(&NukeProfileFromDisk, info), std::move(done_callback));
+  GetBackgroundDiskOperationTaskRunner()->PostTaskAndReply(
+      FROM_HERE, base::BindOnce(&TryNukeProfileFromDisk, info),
+      std::move(done_callback));
 }
 
 void ProfileImpl::ClearRendererCache() {
@@ -254,6 +268,16 @@ std::unique_ptr<ProfileImpl> ProfileImpl::DestroyAndDeleteDataFromDisk(
   if (profile->num_browser_impl_ > 0)
     return profile;
 
+  GetBackgroundDiskOperationTaskRunner()->PostTaskAndReply(
+      FROM_HERE, base::BindOnce(&MarkProfileAsDeleted, profile->info_),
+      base::BindOnce(&ProfileImpl::OnProfileMarked, std::move(profile),
+                     std::move(done_callback)));
+  return nullptr;
+}
+
+// static
+void ProfileImpl::OnProfileMarked(std::unique_ptr<ProfileImpl> profile,
+                                  base::OnceClosure done_callback) {
   // Try to finish all writes and remove all data before nuking the profile.
   static_cast<BrowserContextImpl*>(profile->GetBrowserContext())
       ->pref_service()
@@ -267,7 +291,6 @@ std::unique_ptr<ProfileImpl> ProfileImpl::DestroyAndDeleteDataFromDisk(
                      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)

weblayer/browser/profile_impl.h

@@ -99,6 +99,8 @@ class ProfileImpl : public Profile {
  private:
   class DataClearer;
 
+  static void OnProfileMarked(std::unique_ptr<ProfileImpl> profile,
+                              base::OnceClosure done_callback);
   static void NukeDataAfterRemovingData(std::unique_ptr<ProfileImpl> profile,
                                         base::OnceClosure done_callback);
   static void DoNukeData(std::unique_ptr<ProfileImpl> profile,

weblayer/test/BUILD.gn

@@ -182,3 +182,20 @@ test("weblayer_browsertests") {
     ]
   }
 }
+
+source_set("run_all_unittests") {
+  testonly = true
+  sources = [ "run_all_unittests.cc" ]
+  public_deps = [
+    "//base/test:test_support",
+    "//content/test:test_support",
+  ]
+}
+
+test("weblayer_unittests") {
+  deps = [
+    ":run_all_unittests",
+    "//weblayer:weblayer_lib_base",
+  ]
+  sources = [ "../browser/profile_disk_operations_unittests.cc" ]
+}

weblayer/test/run_all_unittests.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/bind.h"
+#include "base/test/launcher/unit_test_launcher.h"
+#include "content/public/test/content_test_suite_base.h"
+#include "content/public/test/unittest_test_suite.h"
+
+namespace weblayer {
+
+class WebLayerTestSuite : public content::ContentTestSuiteBase {
+ public:
+  WebLayerTestSuite(int argc, char** argv) : ContentTestSuiteBase(argc, argv) {}
+  ~WebLayerTestSuite() override = default;
+
+  WebLayerTestSuite(const WebLayerTestSuite&) = delete;
+  WebLayerTestSuite& operator=(const WebLayerTestSuite&) = delete;
+};
+
+}  // namespace weblayer
+
+int main(int argc, char** argv) {
+  content::UnitTestTestSuite test_suite(
+      new weblayer::WebLayerTestSuite(argc, argv));
+  return base::LaunchUnitTests(argc, argv,
+                               base::BindOnce(&content::UnitTestTestSuite::Run,
+                                              base::Unretained(&test_suite)));
+}