Delete bookmarks on profile deletion

When a profile is deleted, all browsing_data is removed and the folder
is marked for deletion on shutdown. Bookmarks are currently not deleted
until the folder is deleted. As bookmarks can contains sensitive data
they should also be removed immediately.
This also changes SigninManagerAndroidTest to use
ChromeBrowsingDataRemoverDelegate.

Bug: 803643, 748484
Change-Id: Icebfcec44d61d8b38939a8cee8dc4a92b6f9e258
Reviewed-on: https://chromium-review.googlesource.com/957040
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543358}

chrome/browser/android/signin/signin_manager_android.cc

@@ -106,10 +106,6 @@ class ProfileDataRemover : public content::BrowsingDataRemover::Observer {
     remover_->RemoveObserver(this);
 
     if (all_data_) {
-      BookmarkModel* model =
-          BookmarkModelFactory::GetForBrowserContext(profile_);
-      model->RemoveAllUserBookmarks();
-
       // All the Profile data has been wiped. Clear the last signed in username
       // as well, so that the next signin doesn't trigger the account
       // change dialog.

chrome/browser/android/signin/signin_manager_android_unittest.cc

@@ -13,17 +13,40 @@
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/browsing_data/browsing_data_cache_storage_helper.h"
 #include "chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_factory.h"
+#include "chrome/browser/download/chrome_download_manager_delegate.h"
+#include "chrome/browser/download/download_core_service_factory.h"
+#include "chrome/browser/download/download_core_service_impl.h"
+#include "chrome/browser/offline_pages/offline_page_model_factory.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/test/bookmark_test_helpers.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/offline_pages/core/stub_offline_page_model.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browsing_data_remover.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+
+class MockOfflinePageModel : public offline_pages::StubOfflinePageModel {
+ public:
+  void DeleteCachedPagesByURLPredicate(
+      const offline_pages::UrlPredicate& predicate,
+      const offline_pages::DeletePageCallback& callback) override {
+    callback.Run(DeletePageResult::SUCCESS);
+  }
+};
+
+std::unique_ptr<KeyedService> BuildOfflinePageModel(
+    content::BrowserContext* context) {
+  return std::make_unique<MockOfflinePageModel>();
+}
+
+}  // namespace
+
 class SigninManagerAndroidTest : public ::testing::Test {
  public:
   SigninManagerAndroidTest()
@@ -33,18 +56,22 @@ class SigninManagerAndroidTest : public ::testing::Test {
   void SetUp() override {
     ASSERT_TRUE(profile_manager_.SetUp());
     profile_ = profile_manager_.CreateTestingProfile("Testing Profile");
+    // TODO(crbug.com/748484): Remove requirement for this delegate in
+    // unit_tests.
+    DownloadCoreServiceFactory::GetForBrowserContext(profile_)
+        ->SetDownloadManagerDelegateForTesting(
+            std::make_unique<ChromeDownloadManagerDelegate>(profile_));
   }
 
   TestingProfile* profile() { return profile_; }
 
-  static std::unique_ptr<KeyedService> GetEmptyBrowsingDataRemoverDelegate(
-      content::BrowserContext* context) {
-    return nullptr;
-  }
-
   // Adds two testing bookmarks to |profile_|.
   bookmarks::BookmarkModel* AddTestBookmarks() {
     profile_->CreateBookmarkModel(true);
+    // Creating a BookmarkModel also a creates a StubOfflinePageModel.
+    // We need to replace this with a mock that responds to deletions.
+    offline_pages::OfflinePageModelFactory::GetInstance()->SetTestingFactory(
+        profile_, BuildOfflinePageModel);
     bookmarks::BookmarkModel* bookmark_model =
         BookmarkModelFactory::GetForBrowserContext(profile_);
     bookmarks::test::WaitForBookmarkModelToLoad(bookmark_model);
@@ -61,17 +88,6 @@ class SigninManagerAndroidTest : public ::testing::Test {
 
   // Calls SigninManager::WipeData(|all_data|) and waits for its completion.
   void WipeData(bool all_data) {
-    // ChromeBrowsingDataRemoverDelegate deletes a lot of data storage backends
-    // that will not work correctly in a unittest. Remove it. Note that
-    // bookmarks deletion is currently not performed in the delegate, so this
-    // test remains valid.
-    // TODO(crbug.com/748484): Make ChromeBrowsingDataRemoverDelegate usable
-    // in unittests.
-    ChromeBrowsingDataRemoverDelegateFactory::GetInstance()
-        ->SetTestingFactoryAndUse(
-            profile(),
-            SigninManagerAndroidTest::GetEmptyBrowsingDataRemoverDelegate);
-
     std::unique_ptr<base::RunLoop> run_loop(new base::RunLoop());
     SigninManagerAndroid::WipeData(profile(), all_data,
                                    run_loop->QuitClosure());

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc

@@ -777,6 +777,23 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
 #endif
   }
 
+  //////////////////////////////////////////////////////////////////////////////
+  // DATA_TYPE_BOOKMARKS
+  if (remove_mask & DATA_TYPE_BOOKMARKS) {
+    auto* bookmark_model = BookmarkModelFactory::GetForBrowserContext(profile_);
+    if (bookmark_model) {
+      if (delete_begin_.is_null() &&
+          (delete_end_.is_null() || delete_end_.is_max())) {
+        bookmark_model->RemoveAllUserBookmarks();
+      } else {
+        // Bookmark deletion is only implemented to remove all data after a
+        // profile deletion. A full implementation would need to traverse the
+        // whole tree and check timestamps against delete_begin and delete_end.
+        NOTIMPLEMENTED();
+      }
+    }
+  }
+
   //////////////////////////////////////////////////////////////////////////////
   // DATA_TYPE_DURABLE_PERMISSION
   if (remove_mask & DATA_TYPE_DURABLE_PERMISSION) {

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h

@@ -75,6 +75,7 @@ class ChromeBrowsingDataRemoverDelegate
     DATA_TYPE_EXTERNAL_PROTOCOL_DATA = DATA_TYPE_EMBEDDER_BEGIN << 7,
     DATA_TYPE_HOSTED_APP_DATA_TEST_ONLY = DATA_TYPE_EMBEDDER_BEGIN << 8,
     DATA_TYPE_CONTENT_SETTINGS = DATA_TYPE_EMBEDDER_BEGIN << 9,
+    DATA_TYPE_BOOKMARKS = DATA_TYPE_EMBEDDER_BEGIN << 10,
 
     // Group datatypes.
 
@@ -97,10 +98,9 @@ class ChromeBrowsingDataRemoverDelegate
 
     // Datatypes that can be deleted partially per URL / origin / domain,
     // whichever makes sense.
-    FILTERABLE_DATA_TYPES =
-        DATA_TYPE_SITE_DATA |
-        content::BrowsingDataRemover::DATA_TYPE_CACHE |
-        content::BrowsingDataRemover::DATA_TYPE_DOWNLOADS,
+    FILTERABLE_DATA_TYPES = DATA_TYPE_SITE_DATA |
+                            content::BrowsingDataRemover::DATA_TYPE_CACHE |
+                            content::BrowsingDataRemover::DATA_TYPE_DOWNLOADS,
 
     // Includes all the available remove options. Meant to be used by clients
     // that wish to wipe as much data as possible from a Profile, to make it
@@ -112,7 +112,8 @@ class ChromeBrowsingDataRemoverDelegate
                      DATA_TYPE_HISTORY |    //
                      DATA_TYPE_PASSWORDS |
                      content::BrowsingDataRemover::DATA_TYPE_MEDIA_LICENSES |
-                     DATA_TYPE_CONTENT_SETTINGS,
+                     DATA_TYPE_CONTENT_SETTINGS |  //
+                     DATA_TYPE_BOOKMARKS,
 
     // Includes all available remove options. Meant to be used when the Profile
     // is scheduled to be deleted, and all possible data should be wiped from

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc

@@ -1521,6 +1521,23 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest, ExpireBookmarkFavicons) {
   EXPECT_TRUE(favicon_tester.HasExpiredFaviconForPageURL(bookmarked_page));
 }
 
+TEST_F(ChromeBrowsingDataRemoverDelegateTest, DeleteBookmarks) {
+  GURL bookmarked_page("http://a");
+
+  TestingProfile* profile = GetProfile();
+  profile->CreateBookmarkModel(true);
+  bookmarks::BookmarkModel* bookmark_model =
+      BookmarkModelFactory::GetForBrowserContext(profile);
+  bookmarks::test::WaitForBookmarkModelToLoad(bookmark_model);
+  bookmark_model->AddURL(bookmark_model->bookmark_bar_node(), 0,
+                         base::ASCIIToUTF16("a"), bookmarked_page);
+  EXPECT_EQ(1, bookmark_model->bookmark_bar_node()->child_count());
+  BlockUntilBrowsingDataRemoved(
+      base::Time(), base::Time::Max(),
+      ChromeBrowsingDataRemoverDelegate::DATA_TYPE_BOOKMARKS, false);
+  EXPECT_EQ(0, bookmark_model->bookmark_bar_node()->child_count());
+}
+
 // TODO(crbug.com/589586): Disabled, since history is not yet marked as
 // a filterable datatype.
 TEST_F(ChromeBrowsingDataRemoverDelegateTest,