weblayer: makes clearing browsing data remove favicons

This takes the heavy handed approach of deleting *all* favicon
data any time clearing data is requested. To do otherwise would
be challenging as there isn't enough information in the db to
really know which favicons should be deleted in a range. The
chrome side keys off visits to track this information, but we
don't have that.

BUG=1076463
TEST=ProfileBrowserTest.ClearBrowsingDataDeletesFavicons

Change-Id: I033ece1197889ff7f5f5edd864fc2f84e27606fd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2340870
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796182}

components/favicon/core/favicon_database.cc

@@ -67,6 +67,8 @@ namespace favicon {
 //                    table. This is used to determine if it needs to be
 //                    redownloaded from the web. Value 0 denotes that the bitmap
 //                    has been explicitly expired.
+//                    This is used only for ON_VISIT icons, for ON_DEMAND the
+//                    value is always 0.
 //   image_data       PNG encoded data of the favicon.
 //   width            Pixel width of |image_data|.
 //   height           Pixel height of |image_data|.

weblayer/browser/browsing_data_remover_delegate.h

@@ -28,6 +28,7 @@ class BrowsingDataRemoverDelegate : public content::BrowsingDataRemoverDelegate,
 
     // WebLayer-specific datatypes.
     DATA_TYPE_ISOLATED_ORIGINS = DATA_TYPE_EMBEDDER_BEGIN,
+    DATA_TYPE_FAVICONS = DATA_TYPE_EMBEDDER_BEGIN << 1,
   };
 
   explicit BrowsingDataRemoverDelegate(

weblayer/browser/favicon/favicon_backend_wrapper.cc

@@ -4,7 +4,6 @@
 
 #include "weblayer/browser/favicon/favicon_backend_wrapper.h"
 
-#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/sequenced_task_runner.h"
@@ -19,6 +18,7 @@ FaviconBackendWrapper::FaviconBackendWrapper(
       task_runner_(task_runner) {}
 
 void FaviconBackendWrapper::Init(const base::FilePath& db_path) {
+  db_path_ = db_path;
   favicon_backend_ = favicon::FaviconBackend::Create(db_path, this);
   if (!favicon_backend_) {
     LOG(WARNING) << "Could not initialize the favicon database.";
@@ -42,6 +42,13 @@ void FaviconBackendWrapper::Shutdown() {
   commit_timer_.Stop();
 }
 
+void FaviconBackendWrapper::DeleteAndRecreateDatabase() {
+  Shutdown();
+  favicon_backend_.reset();
+  base::DeleteFile(db_path_);
+  Init(db_path_);
+}
+
 std::vector<favicon_base::FaviconRawBitmapResult>
 FaviconBackendWrapper::GetFaviconsForUrl(
     const GURL& page_url,

weblayer/browser/favicon/favicon_backend_wrapper.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <vector>
 
+#include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/ref_counted_delete_on_sequence.h"
 #include "base/timer/timer.h"
@@ -43,6 +44,8 @@ class FaviconBackendWrapper
 
   void Shutdown();
 
+  void DeleteAndRecreateDatabase();
+
   // All of these functions are called by the FaviconServiceImpl. They call
   // through to |favicon_backend_|.
   std::vector<favicon_base::FaviconRawBitmapResult> GetFaviconsForUrl(
@@ -92,6 +95,8 @@ class FaviconBackendWrapper
   // The real implementation of the backend. Is there is a problem initializing
   // the database this will be null.
   std::unique_ptr<favicon::FaviconBackend> favicon_backend_;
+
+  base::FilePath db_path_;
 };
 
 }  // namespace weblayer

weblayer/browser/favicon/favicon_service_impl.cc

@@ -101,6 +101,14 @@ void FaviconServiceImpl::Init(const base::FilePath& db_path) {
       base::BindOnce(&FaviconBackendWrapper::Init, backend_, db_path));
 }
 
+void FaviconServiceImpl::DeleteAndRecreateDatabase(base::OnceClosure callback) {
+  backend_task_runner_->PostTaskAndReply(
+      FROM_HERE,
+      base::BindOnce(&FaviconBackendWrapper::DeleteAndRecreateDatabase,
+                     backend_),
+      std::move(callback));
+}
+
 base::CancelableTaskTracker::TaskId FaviconServiceImpl::GetFaviconForPageUrl(
     const GURL& page_url,
     base::OnceCallback<void(gfx::Image)> callback,

weblayer/browser/favicon/favicon_service_impl.h

@@ -35,6 +35,9 @@ class FaviconServiceImpl : public favicon::CoreFaviconService {
     observer_ = observer;
   }
 
+  // Deletes the database and recreates it, notifying |callback| when done.
+  void DeleteAndRecreateDatabase(base::OnceClosure callback);
+
   // Requests the favicon image for a url (page). The returned image matches
   // that returned from FaviconFetcher.
   base::CancelableTaskTracker::TaskId GetFaviconForPageUrl(

weblayer/browser/profile_browsertest.cc

@@ -59,4 +59,58 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, GetCachedFaviconForPageUrl) {
   run_loop.Run();
 }
 
+IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, ClearBrowsingDataDeletesFavicons) {
+  // Navigate to a page with a favicon.
+  ASSERT_TRUE(embedded_test_server()->Start());
+  TestFaviconFetcherDelegate fetcher_delegate;
+  auto fetcher = shell()->tab()->CreateFaviconFetcher(&fetcher_delegate);
+  const GURL url =
+      embedded_test_server()->GetURL("/simple_page_with_favicon.html");
+  NavigateAndWaitForCompletion(url, shell());
+  fetcher_delegate.WaitForFavicon();
+  EXPECT_FALSE(fetcher_delegate.last_image().IsEmpty());
+  EXPECT_EQ(1, fetcher_delegate.on_favicon_changed_call_count());
+
+  // Delete the favicons.
+  base::RunLoop run_loop;
+  base::Time now = base::Time::Now();
+  ProfileImpl* profile = static_cast<TabImpl*>(shell()->tab())->profile();
+  profile->ClearBrowsingData({BrowsingDataType::COOKIES_AND_SITE_DATA},
+                             now - base::TimeDelta::FromMinutes(5), now,
+                             run_loop.QuitClosure());
+  run_loop.Run();
+
+  // Ask for the cached favicon, there shouldn't be one.
+  base::RunLoop run_loop2;
+  profile->GetCachedFaviconForPageUrl(
+      url, base::BindLambdaForTesting([&](gfx::Image image) {
+        EXPECT_TRUE(image.IsEmpty());
+        run_loop2.Quit();
+      }));
+  run_loop2.Run();
+
+  // Navigate to another page, and verify favicon is downloaded.
+  fetcher_delegate.ClearLastImage();
+  const GURL url2 =
+      embedded_test_server()->GetURL("/simple_page_with_favicon2.html");
+  NavigateAndWaitForCompletion(url2, shell());
+  fetcher_delegate.WaitForFavicon();
+  EXPECT_FALSE(fetcher_delegate.last_image().IsEmpty());
+  EXPECT_EQ(1, fetcher_delegate.on_favicon_changed_call_count());
+
+  // And fetch the favicon one more time.
+  base::RunLoop run_loop3;
+  profile->GetCachedFaviconForPageUrl(
+      url2, base::BindLambdaForTesting([&](gfx::Image image) {
+        EXPECT_FALSE(image.IsEmpty());
+        // The last parameter is the max difference allowed for each color
+        // component. As the image is encoded before saving to disk some
+        // variance is expected.
+        EXPECT_TRUE(gfx::test::AreImagesClose(
+            image, fetcher_delegate.last_image(), 10));
+        run_loop3.Quit();
+      }));
+  run_loop3.Run();
+}
+
 }  // namespace weblayer

weblayer/browser/profile_impl.cc

@@ -126,22 +126,62 @@ class ProfileImpl::DataClearer : public content::BrowsingDataRemover::Observer {
     remover_->AddObserver(this);
   }
 
-  ~DataClearer() override { remover_->RemoveObserver(this); }
-
-  void ClearData(uint64_t mask, base::Time from_time, base::Time to_time) {
+  void ClearData(ProfileImpl* profile,
+                 uint64_t mask,
+                 base::Time from_time,
+                 base::Time to_time) {
+    if (mask & BrowsingDataRemoverDelegate::DATA_TYPE_FAVICONS)
+      ClearFavicons(profile);
     uint64_t origin_types =
         content::BrowsingDataRemover::ORIGIN_TYPE_UNPROTECTED_WEB |
         content::BrowsingDataRemover::ORIGIN_TYPE_PROTECTED_WEB;
     remover_->RemoveAndReply(from_time, to_time, mask, origin_types, this);
   }
 
+  // content::BrowsingDataRemover::Observer:
   void OnBrowsingDataRemoverDone(uint64_t failed_data_types) override {
+    waiting_for_remover_ = false;
+    // Remove the observer now as after this returns the BrowserContext may
+    // be destroyed, which owns |remover_|.
+    remover_->RemoveObserver(this);
+    remover_ = nullptr;
+    RunCallbackAndDeleteThisIfDone();
+  }
+
+ private:
+  // DataClearer deletes itself when removal is done.
+  ~DataClearer() override = default;
+
+  void ClearFavicons(ProfileImpl* profile) {
+    auto* service = FaviconServiceImplFactory::GetForProfile(profile);
+    if (!service)
+      return;
+    waiting_for_favicon_removal_ = true;
+    // The favicon database doesn't track enough information to remove favicons
+    // in a time range. Delete everything.
+    service->DeleteAndRecreateDatabase(base::BindOnce(
+        &DataClearer::OnFaviconsCleared, base::Unretained(this)));
+  }
+
+  // Called when a phase of cleanup completes. If done, deletes this and
+  // notifies |callback_|.
+  void RunCallbackAndDeleteThisIfDone() {
+    if (waiting_for_favicon_removal_ || waiting_for_remover_)
+      return;
+
     std::move(callback_).Run();
     delete this;
   }
 
- private:
-  content::BrowsingDataRemover* const remover_;
+  // Callback when favicons have been cleared.
+  void OnFaviconsCleared() {
+    waiting_for_favicon_removal_ = false;
+    RunCallbackAndDeleteThisIfDone();
+  }
+
+  bool waiting_for_remover_ = true;
+  bool waiting_for_favicon_removal_ = false;
+  content::BrowsingDataRemover* remover_;
   base::OnceCallback<void()> callback_;
 };
 
@@ -242,6 +282,7 @@ void ProfileImpl::ClearBrowsingData(
         remove_mask |= content::BrowsingDataRemover::DATA_TYPE_DOM_STORAGE;
         remove_mask |= content::BrowsingDataRemover::DATA_TYPE_MEDIA_LICENSES;
         remove_mask |= BrowsingDataRemoverDelegate::DATA_TYPE_ISOLATED_ORIGINS;
+        remove_mask |= BrowsingDataRemoverDelegate::DATA_TYPE_FAVICONS;
         break;
       case BrowsingDataType::CACHE:
         remove_mask |= content::BrowsingDataRemover::DATA_TYPE_CACHE;
@@ -251,7 +292,7 @@ void ProfileImpl::ClearBrowsingData(
         NOTREACHED();
     }
   }
-  clearer->ClearData(remove_mask, from_time, to_time);
+  clearer->ClearData(this, remove_mask, from_time, to_time);
 }
 
 void ProfileImpl::SetDownloadDirectory(const base::FilePath& directory) {
@@ -368,14 +409,14 @@ void ProfileImpl::OnProfileMarked(std::unique_ptr<ProfileImpl> profile,
   // Try to finish all writes and remove all data before nuking the profile.
   profile->GetBrowserContext()->pref_service()->CommitPendingWrite();
 
-  // Unretained is safe here because DataClearer is owned by
-  // BrowserContextImpl which is owned by this.
+  ProfileImpl* raw_profile = profile.get();
   auto* clearer = new DataClearer(
       profile->GetBrowserContext(),
       base::BindOnce(&ProfileImpl::NukeDataAfterRemovingData,
                      std::move(profile), std::move(done_callback)));
   uint64_t remove_all_mask = 0xffffffffffffffffull;
-  clearer->ClearData(remove_all_mask, base::Time::Min(), base::Time::Max());
+  clearer->ClearData(raw_profile, remove_all_mask, base::Time::Min(),
+                     base::Time::Max());
 }
 
 #if defined(OS_ANDROID)
@@ -441,9 +482,8 @@ void ProfileImpl::ClearBrowsingData(
   base::android::JavaIntArrayToIntVector(env, j_data_types, &data_type_ints);
   std::vector<BrowsingDataType> data_types;
   data_types.reserve(data_type_ints.size());
-  for (int type : data_type_ints) {
+  for (int type : data_type_ints)
     data_types.push_back(static_cast<BrowsingDataType>(type));
-  }
   ClearBrowsingData(
       data_types,
       base::Time::FromJavaTime(static_cast<int64_t>(j_from_time_millis)),