Don't notify existing observers in SupervisedUserWhitelistService::AddSiteListsChangedCallback().

The additional callbacks caused the SupervisedUserUrlFilter to update its filter unnecessarily, which in turn flakily quit the observer run loop too early.

This CL fixes the flakiness by using the main thread message loop for the SupervisedUserUrlFilter (removing non-determinism), spreading out the synchronization points for site list updates and URL filter updates, and adding additional sanity checks about observed changes.

BUG=452071

Review URL: https://codereview.chromium.org/875423002

Cr-Commit-Position: refs/heads/master@{#313483}

chrome/browser/supervised_user/supervised_user_service_unittest.cc

@@ -46,34 +46,108 @@ void OnProfileDownloadedFail(const base::string16& full_name) {
   ASSERT_TRUE(false) << "Profile download should not have succeeded.";
 }
 
-class SupervisedUserURLFilterObserver :
-    public SupervisedUserURLFilter::Observer {
+// Base class for helper objects that wait for certain events to happen.
+// This class will ensure that calls to QuitRunLoop() (triggered by a subclass)
+// are balanced with Wait() calls.
+class AsyncTestHelper {
  public:
-  explicit SupervisedUserURLFilterObserver(SupervisedUserURLFilter* url_filter)
-      : url_filter_(url_filter) {
+  void Wait() {
+    run_loop_->Run();
     Reset();
-    url_filter_->AddObserver(this);
   }
 
-  ~SupervisedUserURLFilterObserver() {
-    url_filter_->RemoveObserver(this);
+ protected:
+  AsyncTestHelper() {
+    // |quit_called_| will be initialized in Reset().
+    Reset();
   }
 
-  void Wait() {
-    message_loop_runner_->Run();
-    Reset();
+  ~AsyncTestHelper() {
+    EXPECT_FALSE(quit_called_);
   }
 
-  // SupervisedUserURLFilter::Observer
-  void OnSiteListUpdated() override { message_loop_runner_->Quit(); }
+  void QuitRunLoop() {
+    // QuitRunLoop() can not be called more than once between calls to Wait().
+    ASSERT_FALSE(quit_called_);
+    quit_called_ = true;
+    run_loop_->Quit();
+  }
 
  private:
   void Reset() {
-    message_loop_runner_ = new MessageLoopRunner;
+    quit_called_ = false;
+    run_loop_.reset(new base::RunLoop);
+  }
+
+  scoped_ptr<base::RunLoop> run_loop_;
+  bool quit_called_;
+
+  DISALLOW_COPY_AND_ASSIGN(AsyncTestHelper);
+};
+
+class SupervisedUserURLFilterObserver
+    : public AsyncTestHelper,
+      public SupervisedUserURLFilter::Observer {
+ public:
+  SupervisedUserURLFilterObserver() : scoped_observer_(this) {}
+  ~SupervisedUserURLFilterObserver() {}
+
+  void Init(SupervisedUserURLFilter* url_filter) {
+    scoped_observer_.Add(url_filter);
+  }
+
+  // SupervisedUserURLFilter::Observer
+  void OnSiteListUpdated() override {
+    QuitRunLoop();
+  }
+
+ private:
+  ScopedObserver<SupervisedUserURLFilter, SupervisedUserURLFilter::Observer>
+      scoped_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(SupervisedUserURLFilterObserver);
+};
+
+class SiteListObserver : public AsyncTestHelper {
+ public:
+  SiteListObserver() {}
+  ~SiteListObserver() {}
+
+  void Init(SupervisedUserWhitelistService* service) {
+    service->AddSiteListsChangedCallback(base::Bind(
+        &SiteListObserver::OnSiteListsChanged, base::Unretained(this)));
+
+    // The initial call to AddSiteListsChangedCallback will call
+    // OnSiteListsChanged(), so we balance it out by calling Wait().
+    Wait();
+  }
+
+  const std::vector<scoped_refptr<SupervisedUserSiteList>>& site_lists() {
+    return site_lists_;
+  }
+
+  const std::vector<SupervisedUserSiteList::Site>& sites() {
+    return sites_;
   }
 
-  SupervisedUserURLFilter* url_filter_;
-  scoped_refptr<MessageLoopRunner> message_loop_runner_;
+ private:
+  void OnSiteListsChanged(
+      const std::vector<scoped_refptr<SupervisedUserSiteList>>& site_lists) {
+    site_lists_ = site_lists;
+    sites_.clear();
+    for (const scoped_refptr<SupervisedUserSiteList>& site_list : site_lists) {
+      const std::vector<SupervisedUserSiteList::Site>& sites =
+          site_list->sites();
+      sites_.insert(sites_.end(), sites.begin(), sites.end());
+    }
+
+    QuitRunLoop();
+  }
+
+  std::vector<scoped_refptr<SupervisedUserSiteList>> site_lists_;
+  std::vector<SupervisedUserSiteList::Site> sites_;
+
+  DISALLOW_COPY_AND_ASSIGN(SiteListObserver);
 };
 
 class AsyncResultHolder {
@@ -291,9 +365,20 @@ class SupervisedUserServiceExtensionTestBase
     SupervisedUserService* service =
         SupervisedUserServiceFactory::GetForProfile(profile_.get());
     service->Init();
-    service->GetWhitelistService()->AddSiteListsChangedCallback(
-        base::Bind(&SupervisedUserServiceExtensionTestBase::OnSiteListsChanged,
-                   base::Unretained(this)));
+    site_list_observer_.Init(service->GetWhitelistService());
+
+    SupervisedUserURLFilter* url_filter = service->GetURLFilterForUIThread();
+    url_filter->SetBlockingTaskRunnerForTesting(
+        base::MessageLoopProxy::current());
+    url_filter_observer_.Init(url_filter);
+
+    // Wait for the initial update to finish.
+    url_filter_observer_.Wait();
+  }
+
+  void TearDown() override {
+    // Flush the message loop, to ensure all posted tasks run.
+    base::RunLoop().RunUntilIdle();
   }
 
  protected:
@@ -322,21 +407,10 @@ class SupervisedUserServiceExtensionTestBase
     return extension;
   }
 
-  void OnSiteListsChanged(
-      const std::vector<scoped_refptr<SupervisedUserSiteList> >& site_lists) {
-    site_lists_ = site_lists;
-    sites_.clear();
-    for (const scoped_refptr<SupervisedUserSiteList>& site_list : site_lists) {
-      const std::vector<SupervisedUserSiteList::Site>& sites =
-          site_list->sites();
-      sites_.insert(sites_.end(), sites.begin(), sites.end());
-    }
-  }
-
   bool is_supervised_;
   extensions::ScopedCurrentChannel channel_;
-  std::vector<scoped_refptr<SupervisedUserSiteList> > site_lists_;
-  std::vector<SupervisedUserSiteList::Site> sites_;
+  SiteListObserver site_list_observer_;
+  SupervisedUserURLFilterObserver url_filter_observer_;
 };
 
 class SupervisedUserServiceExtensionTestUnsupervised
@@ -374,11 +448,7 @@ TEST_F(SupervisedUserServiceExtensionTestUnsupervised,
 TEST_F(SupervisedUserServiceExtensionTest, ExtensionManagementPolicyProvider) {
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_.get());
-  SupervisedUserURLFilterObserver observer(
-      supervised_user_service->GetURLFilterForUIThread());
   ASSERT_TRUE(profile_->IsSupervised());
-  // Wait for the initial update to finish (otherwise we'll get leaks).
-  observer.Wait();
 
   // Check that a supervised user can install a theme.
   scoped_refptr<extensions::Extension> theme = MakeThemeExtension();
@@ -424,10 +494,11 @@ TEST_F(SupervisedUserServiceExtensionTest, NoContentPacks) {
   SupervisedUserURLFilter* url_filter =
       supervised_user_service->GetURLFilterForUIThread();
 
-  GURL url("http://youtube.com");
-  // ASSERT_EQ instead of ASSERT_TRUE(site_lists_.empty()) so that the error
+  // ASSERT_EQ instead of ASSERT_TRUE([...].empty()) so that the error
   // message contains the size in case of failure.
-  ASSERT_EQ(0u, site_lists_.size());
+  ASSERT_EQ(0u, site_list_observer_.site_lists().size());
+
+  GURL url("http://youtube.com");
   EXPECT_EQ(SupervisedUserURLFilter::ALLOW,
             url_filter->GetFilteringBehaviorForURL(url));
 }
@@ -437,8 +508,6 @@ TEST_F(SupervisedUserServiceExtensionTest, InstallContentPacks) {
       SupervisedUserServiceFactory::GetForProfile(profile_.get());
   SupervisedUserURLFilter* url_filter =
       supervised_user_service->GetURLFilterForUIThread();
-  SupervisedUserURLFilterObserver observer(url_filter);
-  observer.Wait();
 
   GURL example_url("http://example.com");
   GURL moose_url("http://moose.org");
@@ -465,14 +534,16 @@ TEST_F(SupervisedUserServiceExtensionTest, InstallContentPacks) {
   base::FilePath whitelist_path =
       test_data_dir.AppendASCII("whitelists/content_pack/site_list.json");
   whitelist_service->LoadWhitelistForTesting("aaaa", whitelist_path);
-  observer.Wait();
+  site_list_observer_.Wait();
 
-  ASSERT_EQ(1u, site_lists_.size());
-  ASSERT_EQ(3u, sites_.size());
-  EXPECT_EQ(base::ASCIIToUTF16("YouTube"), sites_[0].name);
-  EXPECT_EQ(base::ASCIIToUTF16("Homestar Runner"), sites_[1].name);
-  EXPECT_EQ(base::string16(), sites_[2].name);
+  ASSERT_EQ(1u, site_list_observer_.site_lists().size());
+  ASSERT_EQ(3u, site_list_observer_.sites().size());
+  EXPECT_EQ(base::ASCIIToUTF16("YouTube"), site_list_observer_.sites()[0].name);
+  EXPECT_EQ(base::ASCIIToUTF16("Homestar Runner"),
+            site_list_observer_.sites()[1].name);
+  EXPECT_EQ(base::string16(), site_list_observer_.sites()[2].name);
 
+  url_filter_observer_.Wait();
   EXPECT_EQ(SupervisedUserURLFilter::ALLOW,
             url_filter->GetFilteringBehaviorForURL(example_url));
   EXPECT_EQ(SupervisedUserURLFilter::WARN,
@@ -482,20 +553,21 @@ TEST_F(SupervisedUserServiceExtensionTest, InstallContentPacks) {
   whitelist_path =
       test_data_dir.AppendASCII("whitelists/content_pack_2/site_list.json");
   whitelist_service->LoadWhitelistForTesting("bbbb", whitelist_path);
-  observer.Wait();
+  site_list_observer_.Wait();
 
-  ASSERT_EQ(2u, site_lists_.size());
-  ASSERT_EQ(4u, sites_.size());
+  ASSERT_EQ(2u, site_list_observer_.site_lists().size());
+  ASSERT_EQ(4u, site_list_observer_.sites().size());
 
   // The site lists might be returned in any order, so we put them into a set.
   std::set<std::string> site_names;
-  for (const SupervisedUserSiteList::Site& site : sites_)
+  for (const SupervisedUserSiteList::Site& site : site_list_observer_.sites())
     site_names.insert(base::UTF16ToUTF8(site.name));
   EXPECT_EQ(1u, site_names.count("YouTube"));
   EXPECT_EQ(1u, site_names.count("Homestar Runner"));
   EXPECT_EQ(1u, site_names.count(std::string()));
   EXPECT_EQ(1u, site_names.count("Moose"));
 
+  url_filter_observer_.Wait();
   EXPECT_EQ(SupervisedUserURLFilter::ALLOW,
             url_filter->GetFilteringBehaviorForURL(example_url));
   EXPECT_EQ(SupervisedUserURLFilter::ALLOW,
@@ -503,12 +575,13 @@ TEST_F(SupervisedUserServiceExtensionTest, InstallContentPacks) {
 
   // Unload the first whitelist.
   whitelist_service->UnloadWhitelist("aaaa");
-  observer.Wait();
+  site_list_observer_.Wait();
 
-  ASSERT_EQ(1u, site_lists_.size());
-  ASSERT_EQ(1u, sites_.size());
-  EXPECT_EQ(base::ASCIIToUTF16("Moose"), sites_[0].name);
+  ASSERT_EQ(1u, site_list_observer_.site_lists().size());
+  ASSERT_EQ(1u, site_list_observer_.sites().size());
+  EXPECT_EQ(base::ASCIIToUTF16("Moose"), site_list_observer_.sites()[0].name);
 
+  url_filter_observer_.Wait();
   EXPECT_EQ(SupervisedUserURLFilter::WARN,
             url_filter->GetFilteringBehaviorForURL(example_url));
   EXPECT_EQ(SupervisedUserURLFilter::ALLOW,

chrome/browser/supervised_user/supervised_user_url_filter.cc

@@ -54,7 +54,6 @@ const char* kFilteredSchemes[] = {
   "wss"
 };
 
-
 // This class encapsulates all the state that is required during construction of
 // a new SupervisedUserURLFilter::Contents.
 class FilterBuilder {
@@ -90,7 +89,6 @@ FilterBuilder::~FilterBuilder() {
 }
 
 bool FilterBuilder::AddPattern(const std::string& pattern, int site_id) {
-  DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   std::string scheme;
   std::string host;
   uint16 port;
@@ -137,15 +135,12 @@ void FilterBuilder::AddSiteList(
 }
 
 scoped_ptr<SupervisedUserURLFilter::Contents> FilterBuilder::Build() {
-  DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   contents_->url_matcher.AddConditionSets(all_conditions_);
   return contents_.Pass();
 }
 
 scoped_ptr<SupervisedUserURLFilter::Contents> CreateWhitelistFromPatterns(
     const std::vector<std::string>& patterns) {
-  DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
-
   FilterBuilder builder;
   for (const std::string& pattern : patterns) {
     // TODO(bauerb): We should create a fake site for the whitelist.
@@ -158,8 +153,6 @@ scoped_ptr<SupervisedUserURLFilter::Contents> CreateWhitelistFromPatterns(
 scoped_ptr<SupervisedUserURLFilter::Contents>
 LoadWhitelistsOnBlockingPoolThread(
     const std::vector<scoped_refptr<SupervisedUserSiteList> >& site_lists) {
-  DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
-
   FilterBuilder builder;
   for (const scoped_refptr<SupervisedUserSiteList>& site_list : site_lists)
     builder.AddSiteList(site_list);
@@ -172,7 +165,8 @@ LoadWhitelistsOnBlockingPoolThread(
 SupervisedUserURLFilter::SupervisedUserURLFilter()
     : default_behavior_(ALLOW),
       contents_(new Contents()),
-      blacklist_(NULL) {
+      blacklist_(nullptr),
+      blocking_task_runner_(BrowserThread::GetBlockingPool()) {
   // Detach from the current thread so we can be constructed on a different
   // thread than the one where we're used.
   DetachFromThread();
@@ -396,7 +390,7 @@ void SupervisedUserURLFilter::LoadWhitelists(
   DCHECK(CalledOnValidThread());
 
   base::PostTaskAndReplyWithResult(
-      BrowserThread::GetBlockingPool(),
+      blocking_task_runner_.get(),
       FROM_HERE,
       base::Bind(&LoadWhitelistsOnBlockingPoolThread, site_lists),
       base::Bind(&SupervisedUserURLFilter::SetContents, this));
@@ -415,7 +409,7 @@ void SupervisedUserURLFilter::SetFromPatterns(
   DCHECK(CalledOnValidThread());
 
   base::PostTaskAndReplyWithResult(
-      BrowserThread::GetBlockingPool(),
+      blocking_task_runner_.get(),
       FROM_HERE,
       base::Bind(&CreateWhitelistFromPatterns, patterns),
       base::Bind(&SupervisedUserURLFilter::SetContents, this));
@@ -464,6 +458,11 @@ void SupervisedUserURLFilter::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void SupervisedUserURLFilter::SetBlockingTaskRunnerForTesting(
+    const scoped_refptr<base::TaskRunner>& task_runner) {
+  blocking_task_runner_ = task_runner;
+}
+
 void SupervisedUserURLFilter::SetContents(scoped_ptr<Contents> contents) {
   DCHECK(CalledOnValidThread());
   contents_ = contents.Pass();

chrome/browser/supervised_user/supervised_user_url_filter.h

@@ -159,6 +159,10 @@ class SupervisedUserURLFilter
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // Sets a different task runner for testing.
+  void SetBlockingTaskRunnerForTesting(
+      const scoped_refptr<base::TaskRunner>& task_runner);
+
  private:
   friend class base::RefCountedThreadSafe<SupervisedUserURLFilter>;
   ~SupervisedUserURLFilter();
@@ -191,6 +195,8 @@ class SupervisedUserURLFilter
 
   scoped_ptr<SupervisedUserAsyncURLChecker> async_url_checker_;
 
+  scoped_refptr<base::TaskRunner> blocking_task_runner_;
+
   DISALLOW_COPY_AND_ASSIGN(SupervisedUserURLFilter);
 };
 

chrome/browser/supervised_user/supervised_user_whitelist_service.cc

@@ -86,7 +86,10 @@ void SupervisedUserWhitelistService::Init() {
 void SupervisedUserWhitelistService::AddSiteListsChangedCallback(
     const SiteListsChangedCallback& callback) {
   site_lists_changed_callbacks_.push_back(callback);
-  NotifyWhitelistsChanged();
+
+  std::vector<scoped_refptr<SupervisedUserSiteList>> whitelists;
+  GetLoadedWhitelists(&whitelists);
+  callback.Run(whitelists);
 }
 
 void SupervisedUserWhitelistService::LoadWhitelistForTesting(
@@ -287,10 +290,15 @@ void SupervisedUserWhitelistService::RegisterWhitelist(const std::string& id,
                  weak_ptr_factory_.GetWeakPtr(), id));
 }
 
-void SupervisedUserWhitelistService::NotifyWhitelistsChanged() {
-  std::vector<scoped_refptr<SupervisedUserSiteList> > whitelists;
+void SupervisedUserWhitelistService::GetLoadedWhitelists(
+    std::vector<scoped_refptr<SupervisedUserSiteList>>* whitelists) {
   for (const auto& whitelist : loaded_whitelists_)
-    whitelists.push_back(whitelist.second);
+    whitelists->push_back(whitelist.second);
+}
+
+void SupervisedUserWhitelistService::NotifyWhitelistsChanged() {
+  std::vector<scoped_refptr<SupervisedUserSiteList>> whitelists;
+  GetLoadedWhitelists(&whitelists);
 
   for (const auto& callback : site_lists_changed_callbacks_)
     callback.Run(whitelists);

chrome/browser/supervised_user/supervised_user_whitelist_service.h

@@ -98,6 +98,9 @@ class SupervisedUserWhitelistService : public syncer::SyncableService {
                          const std::string& name,
                          bool new_installation);
 
+  void GetLoadedWhitelists(
+      std::vector<scoped_refptr<SupervisedUserSiteList>>* whitelists);
+
   void NotifyWhitelistsChanged();
 
   void OnWhitelistReady(const std::string& id,