Fix a use-after-free error in WaitOrCountBookmarks

chrome/browser/profiles/profile_statistics_unittest.cc

Index: chrome/browser/profiles/profile_statistics_unittest.cc
diff --git a/chrome/browser/profiles/profile_statistics_unittest.cc b/chrome/browser/profiles/profile_statistics_unittest.cc
index 9fa316ff224869222a151bfdff1a395e050241d6..4dd1089eb3984a5b72fef6a9f1e8324a904a6531 100644
--- a/chrome/browser/profiles/profile_statistics_unittest.cc
+++ b/chrome/browser/profiles/profile_statistics_unittest.cc
@@ -8,15 +8,68 @@
 #include <vector>
 
 #include "base/files/file_path.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/run_loop.h"
+#include "chrome/browser/bookmarks/bookmark_model_factory.h"
+#include "chrome/browser/bookmarks/chrome_bookmark_client.h"
+#include "chrome/browser/bookmarks/managed_bookmark_service_factory.h"
 #include "chrome/browser/profiles/profile_statistics.h"
+#include "chrome/browser/profiles/profile_statistics_aggregator.h"
 #include "chrome/browser/profiles/profile_statistics_common.h"
+#include "chrome/browser/profiles/profile_statistics_factory.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
+#include "components/bookmarks/browser/bookmark_model.h"
+#include "components/prefs/pref_service.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
+
+scoped_ptr<KeyedService> BuildBookmarkModelWithoutLoad(
+    content::BrowserContext* context) {
+  Profile* profile = Profile::FromBrowserContext(context);
+  scoped_ptr<bookmarks::BookmarkModel> bookmark_model(
+      new bookmarks::BookmarkModel(make_scoped_ptr(new ChromeBookmarkClient(
+          profile, ManagedBookmarkServiceFactory::GetForProfile(profile)))));
+  return std::move(bookmark_model);
+}
+
+void LoadBookmarkModel(Profile* profile,
+                       bookmarks::BookmarkModel* bookmark_model) {
+  bookmark_model->Load(profile->GetPrefs(),
+                       profile->GetPrefs()->GetString(prefs::kAcceptLanguages),
+                       profile->GetPath(),
+                       profile->GetIOTaskRunner(),
+                       content::BrowserThread::GetMessageLoopProxyForThread(
+                           content::BrowserThread::UI));
+}
+
+bookmarks::BookmarkModel* CreateBookmarkModelWithoutLoad(Profile* profile) {
+  return static_cast<bookmarks::BookmarkModel*>(
+      BookmarkModelFactory::GetInstance()->SetTestingFactoryAndUse(
+          profile, BuildBookmarkModelWithoutLoad));
+}
+
+class BookmarkStatHelper {
+ public:
+  BookmarkStatHelper() : num_of_times_called_(0) {}
+
+  void StatsCallback(profiles::ProfileCategoryStats stats) {
+    if (stats.back().category == profiles::kProfileStatisticsBookmarks)
+      ++num_of_times_called_;
+  }
+
+  int GetNumOfTimesCalled() { return num_of_times_called_; }
+
+ private:
+  base::Closure quit_closure_;
+  int num_of_times_called_;
+};
+
 void VerifyStatisticsCache(const base::FilePath& profile_path,
     const std::map<std::string, int>& expected,
     const std::vector<std::string>& categories_to_check) {
@@ -86,3 +139,47 @@ TEST_F(ProfileStatisticsTest, ProfileAttributesStorage) {
     VerifyStatisticsCache(profile_path, expected, categories_to_check);
   }
 }
+
+TEST_F(ProfileStatisticsTest, WaitOrCountBookmarks) {
+  TestingProfile* profile = manager()->CreateTestingProfile("Test 1");
+  ASSERT_TRUE(profile);
+
+  bookmarks::BookmarkModel* bookmark_model =
+      CreateBookmarkModelWithoutLoad(profile);
+  ASSERT_TRUE(bookmark_model);
+
+  // Run ProfileStatisticsAggregator::WaitOrCountBookmarks.
+  ProfileStatisticsAggregator* aggregator;
+  BookmarkStatHelper bookmark_stat_helper;
+  base::RunLoop run_loop_aggregator_destruction;
+  // The following should run inside a scope, so the scoped_refptr gets deleted
+  // immediately.
+  {
+    scoped_refptr<ProfileStatisticsAggregator> aggregator_scoped =

[Mike Lerman, 2016/04/04 20:26:27]

Why delete this aggregator? It won't actually complete anything, its tasks will
never fire.

Don't you want to do similar to the previous version, where you call
gatherStatistics twice, but then LoadBookmarkModel only once, and expect that
the StatHelper is only called once?

[lwchkg, 2016/04/04 23:22:52]

By constructing the aggregator it makes a copy of the scoped_refptr on every
tasks and also on every reply of the out-of-thread tasks.

Keeping an extra copy of the scoped_refptr alive here makes
ProfileStatiaticsAggregator not to destruct after the tasks finish.

Anyway, we are checking that the StatHelper is called not more than once with
bookmarks. So we need some guarantee that all tasks are finished before
checking. This is only possible by monitoring the destruction of
ProfileStatisticsAggregator. (Just waiting for the bookmark task return once
does not guarantee that it is called not more than once.)

It is currently impossible to get a callback function returned by
GatherStatistics, so a slightly modified version of GatherStatistics is executed
instead.

(Of course we can add the callback in GatherStatistics, but to do this we need
to patch a non-test file to have the unpatched code failing the test.)

+        new ProfileStatisticsAggregator(
+                profile,
+                base::Bind(&BookmarkStatHelper::StatsCallback,
+                           base::Unretained(&bookmark_stat_helper)),
+                run_loop_aggregator_destruction.QuitClosure());
+    aggregator = aggregator_scoped.get();
+  }
+  // Wait until ProfileStatisticsAggregator::WaitOrCountBookmarks is run.
+  base::RunLoop run_loop1;
+  run_loop1.RunUntilIdle();
+  EXPECT_EQ(0u, bookmark_stat_helper.GetNumOfTimesCalled());
+
+  // Run ProfileStatisticsAggregator::WaitOrCountBookmarks again.
+  aggregator->AddCallbackAndStartAggregator(
+      profiles::ProfileStatisticsCallback());
+  // Wait until ProfileStatisticsAggregator::WaitOrCountBookmarks is run.
+  base::RunLoop run_loop2;
+  run_loop2.RunUntilIdle();
+  EXPECT_EQ(0u, bookmark_stat_helper.GetNumOfTimesCalled());
+
+  // Load the bookmark model. When the model is loaded (asynchronously), the
+  // observer added by WaitOrCountBookmarks is run.
+  LoadBookmarkModel(profile, bookmark_model);
+
+  run_loop_aggregator_destruction.Run();
+  EXPECT_EQ(1u, bookmark_stat_helper.GetNumOfTimesCalled());
+}