[Sync] Migrate SyncInternalsMessageHandler off CallJavascriptFunctionUnsafe.

chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc

Index: chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc
diff --git a/chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc b/chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc
index 466b6e2fb3b21cf5c9468ae4890ca4b714b86de3..9824aecddbf43dcd1f02c488f25465dbc7e0e325 100644
--- a/chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc
+++ b/chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc
@@ -12,47 +12,84 @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/browser_sync/browser_sync_switches.h"
 #include "components/sync/driver/about_sync_util.h"
+#include "components/sync/driver/fake_sync_service.h"
 #include "components/sync/driver/sync_service.h"
+#include "components/sync/js/js_test_util.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_web_ui.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::DictionaryValue;
+using base::ListValue;
+using base::Value;
+using syncer::SyncService;
+using syncer::SyncServiceObserver;
+using syncer::TypeDebugInfoObserver;
+
 namespace {
 
 class TestableSyncInternalsMessageHandler : public SyncInternalsMessageHandler {
  public:
-  explicit TestableSyncInternalsMessageHandler(
+  TestableSyncInternalsMessageHandler(
       content::WebUI* web_ui,
-      std::unique_ptr<AboutSyncDataExtractor> about_sync_data_extractor)
-      : SyncInternalsMessageHandler(std::move(about_sync_data_extractor)) {
+      SyncServiceProvider sync_service_provider,
+      AboutSyncDataDelegate about_sync_data_delegate)
+      : SyncInternalsMessageHandler(std::move(sync_service_provider),
+                                    std::move(about_sync_data_delegate)) {
     set_web_ui(web_ui);
   }
 };
 
-class FakeExtractor : public AboutSyncDataExtractor {
+class TestSyncService : public syncer::FakeSyncService {
  public:
-  std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
-      syncer::SyncService* service,
-      SigninManagerBase* signin) override {
-    call_count_++;
-    last_service_ = service;
-    last_signin_ = signin;
-    std::unique_ptr<base::DictionaryValue> dictionary(
-        new base::DictionaryValue());
-    dictionary->SetString("fake_key", "fake_value");
-    return dictionary;
+  void AddObserver(SyncServiceObserver* observer) override {
+    ++add_observer_count_;
+  }
+
+  void RemoveObserver(SyncServiceObserver* observer) override {
+    ++remove_observer_count_;
+  }
+
+  void AddTypeDebugInfoObserver(TypeDebugInfoObserver* observer) override {
+    ++add_type_debug_info_observer_count_;
   }
 
-  int call_count() const { return call_count_; }
-  syncer::SyncService* last_service() const { return last_service_; }
-  SigninManagerBase* last_signin() const { return last_signin_; }
+  void RemoveTypeDebugInfoObserver(TypeDebugInfoObserver* observer) override {
+    ++remove_type_debug_info_observer_count_;
+  }
+
+  base::WeakPtr<syncer::JsController> GetJsController() override {
+    return js_controller_.AsWeakPtr();
+  }
+
+  void GetAllNodes(const base::Callback<void(std::unique_ptr<base::ListValue>)>&
+                       callback) override {
+    get_all_nodes_callback_ = std::move(callback);
+  }
+
+  int add_observer_count() const { return add_observer_count_; }
+  int remove_observer_count() const { return remove_observer_count_; }
+  int add_type_debug_info_observer_count() const {
+    return add_type_debug_info_observer_count_;
+  }
+  int remove_type_debug_info_observer_count() const {
+    return remove_type_debug_info_observer_count_;
+  }
+  base::Callback<void(std::unique_ptr<base::ListValue>)>
+  get_all_nodes_callback() {
+    return std::move(get_all_nodes_callback_);
+  }
 
  private:
-  int call_count_ = 0;
-  syncer::SyncService* last_service_ = nullptr;
-  SigninManagerBase* last_signin_ = nullptr;
+  int add_observer_count_ = 0;
+  int remove_observer_count_ = 0;
+  int add_type_debug_info_observer_count_ = 0;
+  int remove_type_debug_info_observer_count_ = 0;
+  syncer::MockJsController js_controller_;
+  base::Callback<void(std::unique_ptr<base::ListValue>)>
+      get_all_nodes_callback_;
 };
 
 class SyncInternalsMessageHandlerTest : public ::testing::Test {
@@ -62,9 +99,24 @@ class SyncInternalsMessageHandlerTest : public ::testing::Test {
     web_contents_.reset(content::WebContents::Create(
         content::WebContents::CreateParams(&profile_, site_instance_.get())));
     web_ui_.set_web_contents(web_contents_.get());
-    fake_extractor_ = new FakeExtractor();
+    test_sync_service_ = base::MakeUnique<TestSyncService>();
     handler_.reset(new TestableSyncInternalsMessageHandler(
-        &web_ui_, std::unique_ptr<FakeExtractor>(fake_extractor_)));
+        &web_ui_,
+        base::BindRepeating(&SyncInternalsMessageHandlerTest::sync_service,
+                            base::Unretained(this)),
+        base::BindRepeating(
+            &SyncInternalsMessageHandlerTest::ConstructAboutInformation,
+            base::Unretained(this))));
+  }
+
+  std::unique_ptr<DictionaryValue> ConstructAboutInformation(
+      SyncService* service,
+      version_info::Channel channel) {
+    ++about_sync_data_delegate_call_count_;
+    last_delegate_sync_service_ = service;
+    auto dictionary = base::MakeUnique<DictionaryValue>();
+    dictionary->SetString("fake_key", "fake_value");
+    return dictionary;
   }
 
   void ValidateAboutInfoCall() {
@@ -76,16 +128,16 @@ class SyncInternalsMessageHandlerTest : public ::testing::Test {
 
     EXPECT_EQ(syncer::sync_ui_util::kDispatchEvent, call_data.function_name());
 
-    const base::Value* arg1 = call_data.arg1();
+    const Value* arg1 = call_data.arg1();
     ASSERT_TRUE(arg1);
     std::string event_type;
     EXPECT_TRUE(arg1->GetAsString(&event_type));
     EXPECT_EQ(syncer::sync_ui_util::kOnAboutInfoUpdated, event_type);
 
-    const base::Value* arg2 = call_data.arg2();
+    const Value* arg2 = call_data.arg2();
     ASSERT_TRUE(arg2);
 
-    const base::DictionaryValue* root_dictionary = nullptr;
+    const DictionaryValue* root_dictionary = nullptr;
     ASSERT_TRUE(arg2->GetAsDictionary(&root_dictionary));
 
     std::string fake_value;
@@ -93,37 +145,170 @@ class SyncInternalsMessageHandlerTest : public ::testing::Test {
     EXPECT_EQ("fake_value", fake_value);
   }
 
-  SyncInternalsMessageHandler* handler() { return handler_.get(); }
-  FakeExtractor* fake_extractor() { return fake_extractor_; }
+  void ValidateEmptyAboutInfoCall() {
+    EXPECT_TRUE(web_ui_.call_data().empty());
+  }
+
+  TestSyncService* test_sync_service() { return test_sync_service_.get(); }
+
+  TestableSyncInternalsMessageHandler* handler() { return handler_.get(); }
+
+  int CallCountWithName(const std::string& function_name) {
+    int count = 0;
+    for (const auto& call_data : web_ui_.call_data()) {
+      if (call_data->function_name() == function_name) {
+        count++;
+      }
+    }
+    return count;
+  }
+
+  int about_sync_data_delegate_call_count() const {
+    return about_sync_data_delegate_call_count_;
+  }
+
+  const SyncService* last_delegate_sync_service() const {
+    return last_delegate_sync_service_;
+  }
+
+  void ResetSyncService() { test_sync_service_.reset(); }
+
+  void ResetHandler() { handler_.reset(); }
 
  private:
+  SyncService* sync_service() { return test_sync_service_.get(); }
+
   content::TestBrowserThreadBundle thread_bundle_;
   TestingProfile profile_;
   content::TestWebUI web_ui_;
   scoped_refptr<content::SiteInstance> site_instance_;
   std::unique_ptr<content::WebContents> web_contents_;
-  std::unique_ptr<SyncInternalsMessageHandler> handler_;
+  std::unique_ptr<TestSyncService> test_sync_service_;
+  std::unique_ptr<TestableSyncInternalsMessageHandler> handler_;
 
-  // Non-owning pointer to the about information the handler uses. This
-  // extractor is owned by the handler.
-  FakeExtractor* fake_extractor_;
+  int about_sync_data_delegate_call_count_ = 0;
+  SyncService* last_delegate_sync_service_ = nullptr;
 };
 
-}  // namespace
+TEST_F(SyncInternalsMessageHandlerTest, AddRemoveObservers) {
+  ListValue empty_list;
+
+  EXPECT_EQ(0, test_sync_service()->add_observer_count());
+  handler()->HandleRegisterForEvents(&empty_list);
+  EXPECT_EQ(1, test_sync_service()->add_observer_count());
+
+  EXPECT_EQ(0, test_sync_service()->add_type_debug_info_observer_count());
+  handler()->HandleRegisterForPerTypeCounters(&empty_list);
+  EXPECT_EQ(1, test_sync_service()->add_type_debug_info_observer_count());
+
+  EXPECT_EQ(0, test_sync_service()->remove_observer_count());
+  EXPECT_EQ(0, test_sync_service()->remove_type_debug_info_observer_count());
+  ResetHandler();
+  EXPECT_EQ(1, test_sync_service()->remove_observer_count());
+  EXPECT_EQ(1, test_sync_service()->remove_type_debug_info_observer_count());
+
+  // Add calls should never have increased since the initial subscription.
+  EXPECT_EQ(1, test_sync_service()->add_observer_count());
+  EXPECT_EQ(1, test_sync_service()->add_type_debug_info_observer_count());
+}
+
+TEST_F(SyncInternalsMessageHandlerTest, AddRemoveObserversDisallowJavascript) {
+  ListValue empty_list;
+
+  EXPECT_EQ(0, test_sync_service()->add_observer_count());
+  handler()->HandleRegisterForEvents(&empty_list);
+  EXPECT_EQ(1, test_sync_service()->add_observer_count());
+
+  EXPECT_EQ(0, test_sync_service()->add_type_debug_info_observer_count());
+  handler()->HandleRegisterForPerTypeCounters(&empty_list);
+  EXPECT_EQ(1, test_sync_service()->add_type_debug_info_observer_count());
+
+  EXPECT_EQ(0, test_sync_service()->remove_observer_count());
+  EXPECT_EQ(0, test_sync_service()->remove_type_debug_info_observer_count());
+  handler()->DisallowJavascript();
+  EXPECT_EQ(1, test_sync_service()->remove_observer_count());
+  EXPECT_EQ(1, test_sync_service()->remove_type_debug_info_observer_count());
+
+  // Deregistration should not repeat, no counts should increase.
+  ResetHandler();
+  EXPECT_EQ(1, test_sync_service()->add_observer_count());
+  EXPECT_EQ(1, test_sync_service()->add_type_debug_info_observer_count());
+  EXPECT_EQ(1, test_sync_service()->remove_observer_count());
+  EXPECT_EQ(1, test_sync_service()->remove_type_debug_info_observer_count());
+}
+
+TEST_F(SyncInternalsMessageHandlerTest, AddRemoveObserversSyncDisabled) {
+  // Simulate completely disabling sync by flag or other mechanism.
+  ResetSyncService();
 
-TEST_F(SyncInternalsMessageHandlerTest, SendAboutInfoWithService) {
+  ListValue empty_list;
+  handler()->HandleRegisterForEvents(&empty_list);
+  handler()->HandleRegisterForPerTypeCounters(&empty_list);
+  handler()->DisallowJavascript();
+  // Cannot verify observer methods on sync services were not called, because
+  // there is no sync service. Rather, we're just making sure the handler hasn't
+  // performed any invalid operations when the sync service is missing.
+}
+
+TEST_F(SyncInternalsMessageHandlerTest,
+       RepeatedHandleRegisterForPerTypeCounters) {
+  ListValue empty_list;
+  handler()->HandleRegisterForPerTypeCounters(&empty_list);
+  EXPECT_EQ(1, test_sync_service()->add_type_debug_info_observer_count());
+  EXPECT_EQ(0, test_sync_service()->remove_type_debug_info_observer_count());
+
+  handler()->HandleRegisterForPerTypeCounters(&empty_list);
+  EXPECT_EQ(2, test_sync_service()->add_type_debug_info_observer_count());
+  EXPECT_EQ(1, test_sync_service()->remove_type_debug_info_observer_count());
+
+  handler()->HandleRegisterForPerTypeCounters(&empty_list);
+  EXPECT_EQ(3, test_sync_service()->add_type_debug_info_observer_count());
+  EXPECT_EQ(2, test_sync_service()->remove_type_debug_info_observer_count());
+
+  ResetHandler();
+  EXPECT_EQ(3, test_sync_service()->add_type_debug_info_observer_count());
+  EXPECT_EQ(3, test_sync_service()->remove_type_debug_info_observer_count());
+}
+
+TEST_F(SyncInternalsMessageHandlerTest, HandleGetAllNodes) {
+  ListValue args;
+  args.AppendInteger(0);
+  handler()->HandleGetAllNodes(&args);
+  test_sync_service()->get_all_nodes_callback().Run(
+      base::MakeUnique<ListValue>());
+  EXPECT_EQ(1, CallCountWithName(syncer::sync_ui_util::kGetAllNodesCallback));
+
+  handler()->HandleGetAllNodes(&args);
+  // This  breaks the weak ref the callback is hanging onto. Which results in
+  // the call count not incrementing.
+  handler()->DisallowJavascript();
+  test_sync_service()->get_all_nodes_callback().Run(
+      base::MakeUnique<ListValue>());
+  EXPECT_EQ(1, CallCountWithName(syncer::sync_ui_util::kGetAllNodesCallback));
+
+  handler()->HandleGetAllNodes(&args);
+  test_sync_service()->get_all_nodes_callback().Run(
+      base::MakeUnique<ListValue>());
+  EXPECT_EQ(2, CallCountWithName(syncer::sync_ui_util::kGetAllNodesCallback));
+}
+
+TEST_F(SyncInternalsMessageHandlerTest, SendAboutInfo) {
+  handler()->AllowJavascriptForTesting();
   handler()->OnStateChanged(nullptr);
-  EXPECT_EQ(1, fake_extractor()->call_count());
-  EXPECT_NE(nullptr, fake_extractor()->last_service());
-  EXPECT_NE(nullptr, fake_extractor()->last_signin());
+  EXPECT_EQ(1, about_sync_data_delegate_call_count());
+  EXPECT_NE(nullptr, last_delegate_sync_service());
   ValidateAboutInfoCall();
 }
 
-TEST_F(SyncInternalsMessageHandlerTest, SendAboutInfoWithoutService) {
-  base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kDisableSync);
+TEST_F(SyncInternalsMessageHandlerTest, SendAboutInfoSyncDisabled) {
+  // Simulate completely disabling sync by flag or other mechanism.
+  ResetSyncService();
+
+  handler()->AllowJavascriptForTesting();
   handler()->OnStateChanged(nullptr);
-  EXPECT_EQ(1, fake_extractor()->call_count());
-  EXPECT_EQ(nullptr, fake_extractor()->last_service());
-  EXPECT_EQ(nullptr, fake_extractor()->last_signin());
+  EXPECT_EQ(1, about_sync_data_delegate_call_count());
+  EXPECT_EQ(nullptr, last_delegate_sync_service());
   ValidateAboutInfoCall();
 }
+
+}  // namespace