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

chrome/browser/supervised_user/supervised_user_service_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ skipping 28 matching lines @@
 #endif
 
 using content::MessageLoopRunner;
 
 namespace {
 
 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) {
-    Reset();
-    url_filter_->AddObserver(this);
-  }
-
-  ~SupervisedUserURLFilterObserver() {
-    url_filter_->RemoveObserver(this);
-  }
-
   void Wait() {
-    message_loop_runner_->Run();
+    run_loop_->Run();
     Reset();
   }
 
-  // SupervisedUserURLFilter::Observer
-  void OnSiteListUpdated() override { message_loop_runner_->Quit(); }
+ protected:
+  AsyncTestHelper() {
+    // |quit_called_| will be initialied in Reset().

[Marc Treib, 2015/01/28 09:01:14]

typo: initialiZed

[Bernhard Bauer, 2015/01/28 10:49:58]

Done.

+    Reset();
+  }
+
+  ~AsyncTestHelper() {
+    EXPECT_FALSE(quit_called_);
+  }
+
+  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);
   }
 
-  SupervisedUserURLFilter* url_filter_;
-  scoped_refptr<MessageLoopRunner> message_loop_runner_;
+  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_;
+  }
+
+ 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 {
  public:
   AsyncResultHolder() : result_(false) {}
   ~AsyncResultHolder() {}
 
   void SetResult(bool result) {
     result_ = result;
     run_loop_.Quit();
@@ skipping 197 matching lines @@
 
   void SetUp() override {
     ExtensionServiceTestBase::SetUp();
     ExtensionServiceTestBase::ExtensionServiceInitParams params =
         CreateDefaultInitParams();
     params.profile_is_supervised = is_supervised_;
     InitializeExtensionService(params);
     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:
   scoped_refptr<extensions::Extension> MakeThemeExtension() {
     scoped_ptr<base::DictionaryValue> source(new base::DictionaryValue());
     source->SetString(extensions::manifest_keys::kName, "Theme");
     source->Set(extensions::manifest_keys::kTheme, new base::DictionaryValue());
     source->SetString(extensions::manifest_keys::kVersion, "1.0");
     extensions::ExtensionBuilder builder;
     scoped_refptr<extensions::Extension> extension =
         builder.SetManifest(source.Pass()).Build();
     return extension;
   }
 
   scoped_refptr<extensions::Extension> MakeExtension(bool by_custodian) {
     scoped_ptr<base::DictionaryValue> manifest = extensions::DictionaryBuilder()
       .Set(extensions::manifest_keys::kName, "Extension")
       .Set(extensions::manifest_keys::kVersion, "1.0")
       .Build();
     int creation_flags = extensions::Extension::NO_FLAGS;
     if (by_custodian)
       creation_flags |= extensions::Extension::WAS_INSTALLED_BY_CUSTODIAN;
     extensions::ExtensionBuilder builder;
     scoped_refptr<extensions::Extension> extension =
         builder.SetManifest(manifest.Pass()).AddFlags(creation_flags).Build();
     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
     : public SupervisedUserServiceExtensionTestBase {
  public:
   SupervisedUserServiceExtensionTestUnsupervised()
       : SupervisedUserServiceExtensionTestBase(false) {}
 };
 
 class SupervisedUserServiceExtensionTest
@@ skipping 17 matching lines @@
   base::string16 error_2;
   EXPECT_TRUE(
       supervised_user_service->UserMayModifySettings(extension.get(),
                                                      &error_2));
   EXPECT_EQ(base::string16(), error_2);
 }
 
 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();
   base::string16 error_1;
   EXPECT_TRUE(supervised_user_service->UserMayLoad(theme.get(), &error_1));
   EXPECT_TRUE(error_1.empty());
   EXPECT_TRUE(
       supervised_user_service->UserMayModifySettings(theme.get(), &error_1));
   EXPECT_TRUE(error_1.empty());
 
@@ skipping 25 matching lines @@
   EXPECT_FALSE(supervised_user_service->GetDebugPolicyProviderName().empty());
 #endif
 }
 
 TEST_F(SupervisedUserServiceExtensionTest, NoContentPacks) {
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_.get());
   SupervisedUserURLFilter* url_filter =
       supervised_user_service->GetURLFilterForUIThread();
 
+  // ASSERT_EQ instead of ASSERT_TRUE([...].empty()) so that the error
+  // message contains the size in case of failure.
+  ASSERT_EQ(0u, site_list_observer_.site_lists().size());
+
   GURL url("http://youtube.com");
-  // ASSERT_EQ instead of ASSERT_TRUE(site_lists_.empty()) so that the error
-  // message contains the size in case of failure.
-  ASSERT_EQ(0u, site_lists_.size());
   EXPECT_EQ(SupervisedUserURLFilter::ALLOW,
             url_filter->GetFilteringBehaviorForURL(url));
 }
 
 TEST_F(SupervisedUserServiceExtensionTest, InstallContentPacks) {
   SupervisedUserService* supervised_user_service =
       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");
   EXPECT_EQ(SupervisedUserURLFilter::ALLOW,
             url_filter->GetFilteringBehaviorForURL(example_url));
 
   profile_->GetPrefs()->SetInteger(
       prefs::kDefaultSupervisedUserFilteringBehavior,
       SupervisedUserURLFilter::BLOCK);
   EXPECT_EQ(SupervisedUserURLFilter::BLOCK,
             url_filter->GetFilteringBehaviorForURL(example_url));
 
   profile_->GetPrefs()->SetInteger(
       prefs::kDefaultSupervisedUserFilteringBehavior,
       SupervisedUserURLFilter::WARN);
   EXPECT_EQ(SupervisedUserURLFilter::WARN,
             url_filter->GetFilteringBehaviorForURL(example_url));
 
   // Load a whitelist.
   base::FilePath test_data_dir;
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir));
   SupervisedUserWhitelistService* whitelist_service =
       supervised_user_service->GetWhitelistService();
   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,
             url_filter->GetFilteringBehaviorForURL(moose_url));
 
   // Load a second whitelist.
   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,
             url_filter->GetFilteringBehaviorForURL(moose_url));
 
   // 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,
             url_filter->GetFilteringBehaviorForURL(moose_url));
 }
 #endif  // defined(ENABLE_EXTENSIONS)