Utilize ProfileInfoCache to support data type counts in profile deletion flow

chrome/browser/profiles/profile_statistics.cc

Index: chrome/browser/profiles/profile_statistics.cc
diff --git a/chrome/browser/profiles/profile_statistics.cc b/chrome/browser/profiles/profile_statistics.cc
index 961e4ccf527a81cd8588a63fe0a2cb5bcbc89b69..4384cc828ada328f64e3dc2e5059b8ecddd9f8b2 100644
--- a/chrome/browser/profiles/profile_statistics.cc
+++ b/chrome/browser/profiles/profile_statistics.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/profiles/profile_statistics.h"
 
+#include <set>
 #include "base/bind.h"
 #include "base/memory/ref_counted.h"
 #include "base/prefs/pref_service.h"
@@ -18,13 +19,12 @@
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/bookmark_model_observer.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/password_manager/core/browser/password_store.h"
 #include "components/password_manager/core/browser/password_store_consumer.h"
 #include "content/public/browser/browser_thread.h"
 
-using content::BrowserThread;
-
 namespace {
 
 struct ProfileStatValue {
@@ -45,18 +45,19 @@ int CountBookmarksFromNode(const bookmarks::BookmarkNode* node) {
 
 class ProfileStatisticsAggregator
     : public base::RefCountedThreadSafe<ProfileStatisticsAggregator> {
-  // This class collects statistical information about the profile and returns
+  // This class is used internally by GetProfileStatistics and
+  // StoreProfileStatisticsToCache.
+  //
+  // The class collects statistical information about the profile and returns
   // the information via a callback function. Currently bookmarks, history,
   // logins and preferences are counted.
   //
   // The class is RefCounted because this is needed for CancelableTaskTracker
   // to function properly. Once all tasks are run (or cancelled) the instance is
   // automatically destructed.
-  //
-  // The class is used internally by GetProfileStatistics function.
 
  public:
-  explicit ProfileStatisticsAggregator(Profile* profile,
+  ProfileStatisticsAggregator(Profile* profile,
       const profiles::ProfileStatisticsCallback& callback,
       base::CancelableTaskTracker* tracker);
 
@@ -78,7 +79,8 @@ class ProfileStatisticsAggregator
   void StatisticsCallbackHistory(history::HistoryCountResult result);
 
   // Bookmark counting.
-  ProfileStatValue CountBookmarks() const;
+  void WaitOrCountBookmarks();
+  void CountBookmarks(bookmarks::BookmarkModel* bookmark_model);
 
   // Preference counting.
   ProfileStatValue CountPrefs() const;
@@ -91,6 +93,57 @@ class ProfileStatisticsAggregator
   const profiles::ProfileStatisticsCallback callback_;
 
   base::CancelableTaskTracker* tracker_;
+  scoped_ptr<base::CancelableTaskTracker> default_tracker_;
+
+  // Bookmark counting
+  class BookmarkModelHelper
+      : public bookmarks::BookmarkModelObserver {
+   public:
+    explicit BookmarkModelHelper(ProfileStatisticsAggregator* parent)
+        : parent_(parent) {}
+
+    void BookmarkModelLoaded(bookmarks::BookmarkModel* model,
+                             bool ids_reassigned)
+        override {
+      // Remove observer before release, otherwise it may become a dangling
+      // reference.
+      model->RemoveObserver(this);
+      parent_->CountBookmarks(model);
+      parent_->Release();
+    }
+
+    void BookmarkNodeMoved(bookmarks::BookmarkModel* model,
+                           const bookmarks::BookmarkNode* old_parent,
+                           int old_index,
+                           const bookmarks::BookmarkNode* new_parent,
+                           int new_index) override {}
+
+    void BookmarkNodeAdded(bookmarks::BookmarkModel* model,
+                           const bookmarks::BookmarkNode* parent,
+                           int index) override {}
+
+    void BookmarkNodeRemoved(bookmarks::BookmarkModel* model,
+        const bookmarks::BookmarkNode* parent,
+        int old_index, const bookmarks::BookmarkNode* node,
+        const std::set<GURL>& no_longer_bookmarked) override {}
+
+    void BookmarkNodeChanged(bookmarks::BookmarkModel* model,
+        const bookmarks::BookmarkNode* node) override {}
+
+    void BookmarkNodeFaviconChanged(bookmarks::BookmarkModel* model,
+        const bookmarks::BookmarkNode* node) override {}
+
+    void BookmarkNodeChildrenReordered(bookmarks::BookmarkModel* model,
+        const bookmarks::BookmarkNode* node) override {}
+
+    void BookmarkAllUserNodesRemoved(bookmarks::BookmarkModel* model,
+        const std::set<GURL>& removed_urls) override {}
+
+
+   private:
+    ProfileStatisticsAggregator* parent_ = nullptr;
+  };
+  scoped_ptr<BookmarkModelHelper> bookmark_model_helper_;
 
   // Password counting.
   class PasswordStoreConsumerHelper
@@ -123,17 +176,22 @@ ProfileStatisticsAggregator::ProfileStatisticsAggregator(
       callback_(callback),
       tracker_(tracker),
       password_store_consumer_helper_(this) {
+  if (!tracker_) {
+    default_tracker_.reset(new base::CancelableTaskTracker);
+    tracker_ = default_tracker_.get();
+  }
   Init();
 }
 
 void ProfileStatisticsAggregator::Init() {
+  DCHECK(profile_);
+
   // Initiate bookmark counting (async). Post to UI thread.
-  tracker_->PostTaskAndReplyWithResult(
-      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI).get(),
-          FROM_HERE,
-          base::Bind(&ProfileStatisticsAggregator::CountBookmarks, this),
-          base::Bind(&ProfileStatisticsAggregator::StatisticsCallback,
-                     this, profiles::kProfileStatisticsBookmarks));
+  tracker_->PostTask(
+      content::BrowserThread::GetMessageLoopProxyForThread(
+          content::BrowserThread::UI).get(),
+      FROM_HERE,
+      base::Bind(&ProfileStatisticsAggregator::WaitOrCountBookmarks, this));
 
   // Initiate history counting (async).
   history::HistoryService* history_service =
@@ -163,11 +221,12 @@ void ProfileStatisticsAggregator::Init() {
 
   // Initiate preference counting (async). Post to UI thread.
   tracker_->PostTaskAndReplyWithResult(
-      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI).get(),
-          FROM_HERE,
-          base::Bind(&ProfileStatisticsAggregator::CountPrefs, this),
-          base::Bind(&ProfileStatisticsAggregator::StatisticsCallback,
-                     this, profiles::kProfileStatisticsSettings));
+      content::BrowserThread::GetMessageLoopProxyForThread(
+          content::BrowserThread::UI).get(),
+      FROM_HERE,
+      base::Bind(&ProfileStatisticsAggregator::CountPrefs, this),
+      base::Bind(&ProfileStatisticsAggregator::StatisticsCallback,
+                 this, profiles::kProfileStatisticsSettings));
 }
 
 void ProfileStatisticsAggregator::StatisticsCallback(
@@ -177,7 +236,13 @@ void ProfileStatisticsAggregator::StatisticsCallback(
   datum.count = result.count;
   datum.success = result.success;
   profile_category_stats_.push_back(datum);
-  callback_.Run(profile_category_stats_);
+  if (!callback_.is_null())
+    callback_.Run(profile_category_stats_);
+
+  if (result.success) {
+    profiles::SetProfileStatisticsInCache(profile_->GetPath(), datum.category,
+                                          result.count);
+  }
 }
 
 void ProfileStatisticsAggregator::StatisticsCallbackSuccess(
@@ -205,21 +270,30 @@ void ProfileStatisticsAggregator::StatisticsCallbackHistory(
                      result_converted);
 }
 
-ProfileStatValue ProfileStatisticsAggregator::CountBookmarks() const {
+void ProfileStatisticsAggregator::CountBookmarks(
+    bookmarks::BookmarkModel* bookmark_model) {
+  int count = CountBookmarksFromNode(bookmark_model->bookmark_bar_node()) +
+              CountBookmarksFromNode(bookmark_model->other_node()) +
+              CountBookmarksFromNode(bookmark_model->mobile_node());
+
+  StatisticsCallbackSuccess(profiles::kProfileStatisticsBookmarks, count);
+}
+
+void ProfileStatisticsAggregator::WaitOrCountBookmarks() {
   bookmarks::BookmarkModel* bookmark_model =
       BookmarkModelFactory::GetForProfileIfExists(profile_);

[lwchkg, 2015/12/24 07:14:32]

The crash happens here inside GetForProfileIfExists(profile_). I guess it is a
bug anyway, so I'll investigate and sumbit another CL if appropriate. WDYT?

 
-  ProfileStatValue result;
   if (bookmark_model) {
-    result.count = CountBookmarksFromNode(bookmark_model->bookmark_bar_node()) +
-                   CountBookmarksFromNode(bookmark_model->other_node()) +
-                   CountBookmarksFromNode(bookmark_model->mobile_node());
-    result.success = true;
+    if (bookmark_model->loaded()) {
+      CountBookmarks(bookmark_model);
+    } else {
+      AddRef();
+      bookmark_model_helper_.reset(new BookmarkModelHelper(this));
+      bookmark_model->AddObserver(bookmark_model_helper_.get());
+    }
   } else {
-    result.count = 0;
-    result.success = false;
+    StatisticsCallbackFailure(profiles::kProfileStatisticsBookmarks);
   }
-  return result;
 }
 
 ProfileStatValue ProfileStatisticsAggregator::CountPrefs() const {
@@ -261,9 +335,16 @@ const char kProfileStatisticsPasswords[] = "Passwords";
 const char kProfileStatisticsBookmarks[] = "Bookmarks";
 const char kProfileStatisticsSettings[] = "Settings";
 
-void GetProfileStatistics(Profile* profile,
-    const ProfileStatisticsCallback& callback,
-    base::CancelableTaskTracker* tracker) {
+void GatherProfileStatistics(Profile* profile,
+                             const ProfileStatisticsCallback& callback,
+                             base::CancelableTaskTracker* tracker) {
+  DCHECK(profile);
+  if (profile->IsOffTheRecord()) {
+    LOG(WARNING) <<

[lwchkg, 2015/12/22 13:26:00]

The code here is just a draft. I need your comments to finalize it. Two
questions:
(1) should I use CHECK or LOG for this check?
(2) what exact conditions should I check against? Is that the code below, or
just a subset of it?
!profile->IsGuestSession() && !profile->IsSystemProfile() &&
!profile->IsNewProfile() && !profile->IsOffTheRecord()

[Mike Lerman, 2015/12/23 20:25:50]

(1)
Definitely not CHECK(). This isn't worth crashing over in the field, which
CHECK() will do. Similarly, I don't think LOG is necessary, as we don't need
in-the-field logging over this.

Will this crash if called for an incognito profile? If it will crash later, then
I'm all for a DCHECK, which will help diagnose issues down the road. If it won't
crash later, then I think a DLOG is fine.

(2)
First off, IsOffTheRecord() will be true whenever IsGuestSession() is true, so
there's no need to get IsGuestSession() separately.
Second, I think we just need to check IsOffTheRecord and IsSystemProfile. While
IsNewProfile isn't something we want to have happen (from the ProfileManager),
there's nothing innately wrong with doing so. But you can include it if you feel
more comfortable.

[lwchkg, 2015/12/24 07:14:32]

DCHECK and DLOG won't help here because the crash happens in browser tests.

Anyway, I don't understand why incognito profiles does crash, which shouldn't. I
expect some or all statistics fail though. Most likely the check is not
necessary after the bug is fixed. WDYT?
Thanks. Then IsOffTheRecord and IsSystemProfile is what we want (if the check is
needed at all.)

[lwchkg, 2015/12/29 04:51:43]

Now I understand that incognito profiles crashed.

The reason is as follows: in browser tests incognito profiles get deallocated
before the profile statistics tasks get executed. So crash happens due to
dangling reference. After a few hours of search I've found that incognito
profiles are deallocated by the following code:
 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

The good news is no bug is found.

+        "An off-the-record profile has been passed to GatherProfileStatistics.";
+    return;
+  }
+
   scoped_refptr<ProfileStatisticsAggregator> aggregator =
       new ProfileStatisticsAggregator(profile, callback, tracker);
 }
@@ -308,8 +389,12 @@ void SetProfileStatisticsInCache(const base::FilePath& profile_path,
   if (!g_browser_process || !g_browser_process->local_state())
     return;
 
-  ProfileInfoCache& profile_info_cache =
-      g_browser_process->profile_manager()->GetProfileInfoCache();
+  // profile_manager() may return a null pointer.
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  if (!profile_manager)
+    return;
+
+  ProfileInfoCache& profile_info_cache = profile_manager->GetProfileInfoCache();
   ProfileAttributesEntry* entry = nullptr;
   if (!profile_info_cache.GetProfileAttributesWithPath(profile_path, &entry))
     return;