[Sync] Migrate SyncInternalsMessageHandler off CallJavascriptFunctionUnsafe.
This CL is a result of comments in
https://codereview.chromium.org/2872023002/ that became too large to be
included in the original code review. These changes should make
SyncInternalsMessageHandler behave more safely in regards to when
javascript methods are invoked, as well as increasing test coverage.
* Replaced CallJavascriptFunctionUnsafe with CallJavascriptFunction.
This mostly worked by adding AllowJavascript to all callbacks from
javascript, and unregistering from model callbacks/observations in
OnJavascriptDisallowed(). However, this didn't work for
OnReceivedAllNodes, which is called asycronously. Instead we need to
manually verify IsJavascriptAllowed() is still true.
* Moved SigninManagerBase accessor from ProfileSyncService to
SyncService[Base]. This allowed us to remove the SyncManagerBase param
from the ConstructAboutInformation function. In general we want to move
things out of ProfileSyncService that don't have a reason to be there.
* Reworked how SyncInternalsMessageHandler accesses a SyncService to
facilitate unit tests.
* Replaced extractor with callback to be more similar to SyncService
injection pattern.
* Added test coverage for registration/unregistration.
BUG=719044
Review-Url: https://codereview.chromium.org/2898723003
Cr-Commit-Position: refs/heads/master@{#474755}
diff --git a/chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.cc b/chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.cc
index 29594961..c76b85d 100644
--- a/chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.cc
+++ b/chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.cc
@@ -4,6 +4,9 @@
#include "chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.h"
+#include <memory>
+#include <string>
+
#include "base/json/json_string_value_serializer.h"
#include "base/strings/string_util.h"
#include "base/sys_info.h"
@@ -169,8 +172,8 @@
browser_sync::ProfileSyncService* service =
ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile);
std::unique_ptr<base::DictionaryValue> sync_logs(
- syncer::sync_ui_util::ConstructAboutInformation(
- service, service->signin(), chrome::GetChannel()));
+ syncer::sync_ui_util::ConstructAboutInformation(service,
+ chrome::GetChannel()));
// Remove identity section.
base::ListValue* details = NULL;
diff --git a/chrome/browser/sync/profile_sync_service_android.cc b/chrome/browser/sync/profile_sync_service_android.cc
index 48eb13a..61be6202 100644
--- a/chrome/browser/sync/profile_sync_service_android.cc
+++ b/chrome/browser/sync/profile_sync_service_android.cc
@@ -467,8 +467,8 @@
DCHECK_CURRENTLY_ON(BrowserThread::UI);
std::unique_ptr<base::DictionaryValue> about_info =
- syncer::sync_ui_util::ConstructAboutInformation(
- sync_service_, sync_service_->signin(), chrome::GetChannel());
+ syncer::sync_ui_util::ConstructAboutInformation(sync_service_,
+ chrome::GetChannel());
std::string about_info_json;
base::JSONWriter::Write(*about_info, &about_info_json);
diff --git a/chrome/browser/sync/test/integration/profile_sync_service_harness.cc b/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
index b186e7f..411beeb 100644
--- a/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
+++ b/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
@@ -543,8 +543,8 @@
std::string ProfileSyncServiceHarness::GetServiceStatus() {
std::unique_ptr<base::DictionaryValue> value(
- syncer::sync_ui_util::ConstructAboutInformation(
- service(), service()->signin(), chrome::GetChannel()));
+ syncer::sync_ui_util::ConstructAboutInformation(service(),
+ chrome::GetChannel()));
std::string service_status;
base::JSONWriter::WriteWithOptions(
*value, base::JSONWriter::OPTIONS_PRETTY_PRINT, &service_status);
diff --git a/chrome/browser/ui/webui/sync_internals_message_handler.cc b/chrome/browser/ui/webui/sync_internals_message_handler.cc
index 4e29f99aa..3a68db4 100644
--- a/chrome/browser/ui/webui/sync_internals_message_handler.cc
+++ b/chrome/browser/ui/webui/sync_internals_message_handler.cc
@@ -4,6 +4,8 @@
#include "chrome/browser/ui/webui/sync_internals_message_handler.h"
+#include <stdint.h>
+
#include <utility>
#include <vector>
@@ -13,7 +15,6 @@
#include "chrome/browser/sync/profile_sync_service_factory.h"
#include "chrome/common/channel_info.h"
#include "components/browser_sync/profile_sync_service.h"
-#include "components/signin/core/browser/signin_manager_base.h"
#include "components/sync/base/weak_handle.h"
#include "components/sync/driver/about_sync_util.h"
#include "components/sync/driver/sync_service.h"
@@ -25,45 +26,41 @@
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/web_ui.h"
+using base::DictionaryValue;
+using base::ListValue;
+using base::Value;
using browser_sync::ProfileSyncService;
using syncer::JsEventDetails;
using syncer::ModelTypeSet;
+using syncer::SyncService;
using syncer::WeakHandle;
-namespace {
-class UtilAboutSyncDataExtractor : public AboutSyncDataExtractor {
- public:
- std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
- syncer::SyncService* service,
- SigninManagerBase* signin) override {
- return syncer::sync_ui_util::ConstructAboutInformation(
- service, signin, chrome::GetChannel());
- }
-};
-} // namespace
-
SyncInternalsMessageHandler::SyncInternalsMessageHandler()
: SyncInternalsMessageHandler(
- base::MakeUnique<UtilAboutSyncDataExtractor>()) {}
+ base::BindRepeating(
+ &SyncInternalsMessageHandler::BindForSyncServiceProvider,
+ base::Unretained(this)),
+ base::BindRepeating(
+ &syncer::sync_ui_util::ConstructAboutInformation)) {}
SyncInternalsMessageHandler::SyncInternalsMessageHandler(
- std::unique_ptr<AboutSyncDataExtractor> about_sync_data_extractor)
- : about_sync_data_extractor_(std::move(about_sync_data_extractor)),
+ SyncServiceProvider sync_service_provider,
+ AboutSyncDataDelegate about_sync_data_delegate)
+ : sync_service_provider_(std::move(sync_service_provider)),
+ about_sync_data_delegate_(std::move(about_sync_data_delegate)),
weak_ptr_factory_(this) {}
SyncInternalsMessageHandler::~SyncInternalsMessageHandler() {
- if (js_controller_)
- js_controller_->RemoveJsEventHandler(this);
+ UnregisterModelNotifications();
+}
- ProfileSyncService* service = GetProfileSyncService();
- if (service && service->HasObserver(this)) {
- service->RemoveObserver(this);
- service->RemoveProtocolEventObserver(this);
- }
-
- if (service && is_registered_for_counters_) {
- service->RemoveTypeDebugInfoObserver(this);
- }
+void SyncInternalsMessageHandler::OnJavascriptDisallowed() {
+ // Invaliding weak ptrs works well here because the only weak ptr we vend is
+ // to the sync side to give us information that should be used to populate the
+ // javascript side. If javascript is disallowed, we don't care about updating
+ // the UI with data, so dropping those callbacks is fine.
+ weak_ptr_factory_.InvalidateWeakPtrs();
+ UnregisterModelNotifications();
}
void SyncInternalsMessageHandler::RegisterMessages() {
@@ -96,13 +93,14 @@
}
void SyncInternalsMessageHandler::HandleRegisterForEvents(
- const base::ListValue* args) {
+ const ListValue* args) {
DCHECK(args->empty());
+ AllowJavascript();
// is_registered_ flag protects us from double-registering. This could
// happen on a page refresh, where the JavaScript gets re-run but the
// message handler remains unchanged.
- ProfileSyncService* service = GetProfileSyncService();
+ SyncService* service = sync_service_provider_.Run();
if (service && !is_registered_) {
service->AddObserver(this);
service->AddProtocolEventObserver(this);
@@ -113,52 +111,61 @@
}
void SyncInternalsMessageHandler::HandleRegisterForPerTypeCounters(
- const base::ListValue* args) {
+ const ListValue* args) {
DCHECK(args->empty());
+ AllowJavascript();
- if (ProfileSyncService* service = GetProfileSyncService()) {
- if (!is_registered_for_counters_) {
- service->AddTypeDebugInfoObserver(this);
- is_registered_for_counters_ = true;
- } else {
- // Re-register to ensure counters get re-emitted.
- service->RemoveTypeDebugInfoObserver(this);
- service->AddTypeDebugInfoObserver(this);
- }
+ SyncService* service = sync_service_provider_.Run();
+ if (!service)
+ return;
+
+ if (!is_registered_for_counters_) {
+ service->AddTypeDebugInfoObserver(this);
+ is_registered_for_counters_ = true;
+ } else {
+ // Re-register to ensure counters get re-emitted.
+ service->RemoveTypeDebugInfoObserver(this);
+ service->AddTypeDebugInfoObserver(this);
}
}
void SyncInternalsMessageHandler::HandleRequestUpdatedAboutInfo(
- const base::ListValue* args) {
+ const ListValue* args) {
DCHECK(args->empty());
+ AllowJavascript();
SendAboutInfo();
}
void SyncInternalsMessageHandler::HandleRequestListOfTypes(
- const base::ListValue* args) {
+ const ListValue* args) {
DCHECK(args->empty());
- base::DictionaryValue event_details;
- std::unique_ptr<base::ListValue> type_list(new base::ListValue());
+ AllowJavascript();
+
+ DictionaryValue event_details;
+ auto type_list = base::MakeUnique<ListValue>();
ModelTypeSet protocol_types = syncer::ProtocolTypes();
- for (ModelTypeSet::Iterator it = protocol_types.First();
- it.Good(); it.Inc()) {
+ for (ModelTypeSet::Iterator it = protocol_types.First(); it.Good();
+ it.Inc()) {
type_list->AppendString(ModelTypeToString(it.Get()));
}
event_details.Set(syncer::sync_ui_util::kTypes, std::move(type_list));
- web_ui()->CallJavascriptFunctionUnsafe(
- syncer::sync_ui_util::kDispatchEvent,
- base::Value(syncer::sync_ui_util::kOnReceivedListOfTypes), event_details);
+ DispatchEvent(syncer::sync_ui_util::kOnReceivedListOfTypes, event_details);
}
-void SyncInternalsMessageHandler::HandleGetAllNodes(
- const base::ListValue* args) {
+void SyncInternalsMessageHandler::HandleGetAllNodes(const ListValue* args) {
DCHECK_EQ(1U, args->GetSize());
+ AllowJavascript();
+
int request_id = 0;
bool success = ExtractIntegerValue(args, &request_id);
DCHECK(success);
- ProfileSyncService* service = GetProfileSyncService();
+ SyncService* service = sync_service_provider_.Run();
if (service) {
+ // This opens up the possibility of non-javascript code calling us
+ // asynchronously, and potentially at times we're not allowed to call into
+ // the javascript side. We guard against this by invalidating this weak ptr
+ // should javascript become disallowed.
service->GetAllNodes(
base::Bind(&SyncInternalsMessageHandler::OnReceivedAllNodes,
weak_ptr_factory_.GetWeakPtr(), request_id));
@@ -167,23 +174,19 @@
void SyncInternalsMessageHandler::OnReceivedAllNodes(
int request_id,
- std::unique_ptr<base::ListValue> nodes) {
- base::Value id(request_id);
- web_ui()->CallJavascriptFunctionUnsafe(
- syncer::sync_ui_util::kGetAllNodesCallback, id, *nodes);
+ std::unique_ptr<ListValue> nodes) {
+ CallJavascriptFunction(syncer::sync_ui_util::kGetAllNodesCallback,
+ Value(request_id), *nodes);
}
-void SyncInternalsMessageHandler::OnStateChanged(syncer::SyncService* sync) {
+void SyncInternalsMessageHandler::OnStateChanged(SyncService* sync) {
SendAboutInfo();
}
void SyncInternalsMessageHandler::OnProtocolEvent(
const syncer::ProtocolEvent& event) {
- std::unique_ptr<base::DictionaryValue> value(
- syncer::ProtocolEvent::ToValue(event));
- web_ui()->CallJavascriptFunctionUnsafe(
- syncer::sync_ui_util::kDispatchEvent,
- base::Value(syncer::sync_ui_util::kOnProtocolEvent), *value);
+ std::unique_ptr<DictionaryValue> value(syncer::ProtocolEvent::ToValue(event));
+ DispatchEvent(syncer::sync_ui_util::kOnProtocolEvent, *value);
}
void SyncInternalsMessageHandler::OnCommitCountersUpdated(
@@ -207,14 +210,12 @@
void SyncInternalsMessageHandler::EmitCounterUpdate(
syncer::ModelType type,
const std::string& counter_type,
- std::unique_ptr<base::DictionaryValue> value) {
- std::unique_ptr<base::DictionaryValue> details(new base::DictionaryValue());
+ std::unique_ptr<DictionaryValue> value) {
+ auto details = base::MakeUnique<DictionaryValue>();
details->SetString(syncer::sync_ui_util::kModelType, ModelTypeToString(type));
details->SetString(syncer::sync_ui_util::kCounterType, counter_type);
details->Set(syncer::sync_ui_util::kCounters, std::move(value));
- web_ui()->CallJavascriptFunctionUnsafe(
- syncer::sync_ui_util::kDispatchEvent,
- base::Value(syncer::sync_ui_util::kOnCountersUpdated), *details);
+ DispatchEvent(syncer::sync_ui_util::kOnCountersUpdated, *details);
}
void SyncInternalsMessageHandler::HandleJsEvent(
@@ -222,25 +223,44 @@
const JsEventDetails& details) {
DVLOG(1) << "Handling event: " << name
<< " with details " << details.ToString();
- web_ui()->CallJavascriptFunctionUnsafe(syncer::sync_ui_util::kDispatchEvent,
- base::Value(name), details.Get());
+ DispatchEvent(name, details.Get());
}
void SyncInternalsMessageHandler::SendAboutInfo() {
- ProfileSyncService* sync_service = GetProfileSyncService();
- SigninManagerBase* signin = sync_service ? sync_service->signin() : nullptr;
- std::unique_ptr<base::DictionaryValue> value =
- about_sync_data_extractor_->ConstructAboutInformation(sync_service,
- signin);
- web_ui()->CallJavascriptFunctionUnsafe(
- syncer::sync_ui_util::kDispatchEvent,
- base::Value(syncer::sync_ui_util::kOnAboutInfoUpdated), *value);
+ std::unique_ptr<DictionaryValue> value = about_sync_data_delegate_.Run(
+ sync_service_provider_.Run(), chrome::GetChannel());
+ DispatchEvent(syncer::sync_ui_util::kOnAboutInfoUpdated, *value);
}
-// Gets the ProfileSyncService of the underlying original profile.
-// May return NULL (e.g., if sync is disabled on the command line).
-ProfileSyncService* SyncInternalsMessageHandler::GetProfileSyncService() {
- Profile* profile = Profile::FromWebUI(web_ui());
+SyncService* SyncInternalsMessageHandler::BindForSyncServiceProvider() {
return ProfileSyncServiceFactory::GetForProfile(
- profile->GetOriginalProfile());
+ Profile::FromWebUI(web_ui())->GetOriginalProfile());
+}
+
+void SyncInternalsMessageHandler::DispatchEvent(const std::string& name,
+ const Value& details_value) {
+ CallJavascriptFunction(syncer::sync_ui_util::kDispatchEvent, Value(name),
+ details_value);
+}
+
+void SyncInternalsMessageHandler::UnregisterModelNotifications() {
+ SyncService* service = sync_service_provider_.Run();
+ if (!service)
+ return;
+
+ // Cannot use ScopedObserver to do all the tracking because most don't follow
+ // AddObserver/RemoveObserver method naming style.
+ if (is_registered_) {
+ DCHECK(js_controller_);
+ service->RemoveObserver(this);
+ service->RemoveProtocolEventObserver(this);
+ js_controller_->RemoveJsEventHandler(this);
+ js_controller_ = nullptr;
+ is_registered_ = false;
+ }
+
+ if (is_registered_for_counters_) {
+ service->RemoveTypeDebugInfoObserver(this);
+ is_registered_for_counters_ = false;
+ }
}
diff --git a/chrome/browser/ui/webui/sync_internals_message_handler.h b/chrome/browser/ui/webui/sync_internals_message_handler.h
index 3fe7cca..a399117 100644
--- a/chrome/browser/ui/webui/sync_internals_message_handler.h
+++ b/chrome/browser/ui/webui/sync_internals_message_handler.h
@@ -18,10 +18,9 @@
#include "components/sync/engine/events/protocol_event_observer.h"
#include "components/sync/js/js_controller.h"
#include "components/sync/js/js_event_handler.h"
+#include "components/version_info/channel.h"
#include "content/public/browser/web_ui_message_handler.h"
-class SigninManagerBase;
-
namespace browser_sync {
class ProfileSyncService;
} // namespace browser_sync
@@ -30,17 +29,6 @@
class SyncService;
} // namespace syncer
-// Interface to abstract away the creation of the about-sync value dictionary.
-class AboutSyncDataExtractor {
- public:
- // Given state about sync, extracts various interesting fields and populates
- // a tree of base::Value objects.
- virtual std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
- syncer::SyncService* service,
- SigninManagerBase* signin) = 0;
- virtual ~AboutSyncDataExtractor() {}
-};
-
// The implementation for the chrome://sync-internals page.
class SyncInternalsMessageHandler : public content::WebUIMessageHandler,
public syncer::JsEventHandler,
@@ -51,6 +39,8 @@
SyncInternalsMessageHandler();
~SyncInternalsMessageHandler() override;
+ // content::WebUIMessageHandler implementation.
+ void OnJavascriptDisallowed() override;
void RegisterMessages() override;
// Sets up observers to receive events and forward them to the UI.
@@ -79,10 +69,10 @@
// syncer::SyncServiceObserver implementation.
void OnStateChanged(syncer::SyncService* sync) override;
- // ProtocolEventObserver implementation.
+ // syncer::ProtocolEventObserver implementation.
void OnProtocolEvent(const syncer::ProtocolEvent& e) override;
- // TypeDebugInfoObserver implementation.
+ // syncer::TypeDebugInfoObserver implementation.
void OnCommitCountersUpdated(syncer::ModelType type,
const syncer::CommitCounters& counters) override;
void OnUpdateCountersUpdated(syncer::ModelType type,
@@ -100,16 +90,33 @@
std::unique_ptr<base::DictionaryValue> value);
protected:
- // Constructor used for unit testing to override the about sync info.
- SyncInternalsMessageHandler(
- std::unique_ptr<AboutSyncDataExtractor> about_sync_data_extractor);
+ using SyncServiceProvider = base::RepeatingCallback<syncer::SyncService*()>;
+
+ using AboutSyncDataDelegate =
+ base::RepeatingCallback<std::unique_ptr<base::DictionaryValue>(
+ syncer::SyncService* service,
+ version_info::Channel channel)>;
+
+ // Constructor used for unit testing to override dependencies.
+ SyncInternalsMessageHandler(SyncServiceProvider sync_service_provider,
+ AboutSyncDataDelegate about_sync_data_delegate);
private:
// Fetches updated aboutInfo and sends it to the page in the form of an
// onAboutInfoUpdated event.
void SendAboutInfo();
- browser_sync::ProfileSyncService* GetProfileSyncService();
+ // Gets the ProfileSyncService of the underlying original profile. May return
+ // nullptr (e.g., if sync is disabled on the command line). Shouldn't be
+ // called directly, but instead through |sync_service_provider_|.
+ syncer::SyncService* BindForSyncServiceProvider();
+
+ // Sends a dispatch event to the UI. Javascript must be enabled.
+ void DispatchEvent(const std::string& name, const base::Value& details_value);
+
+ // Unregisters for notifications from all notifications coming from the sync
+ // machinery. Leaves notifications hooked into the UI alone.
+ void UnregisterModelNotifications();
base::WeakPtr<syncer::JsController> js_controller_;
@@ -120,8 +127,11 @@
// ProfileSyncService.
bool is_registered_for_counters_ = false;
+ // An abstraction of who provides the SyncService.
+ SyncServiceProvider sync_service_provider_;
+
// An abstraction of who creates the about sync info value map.
- std::unique_ptr<AboutSyncDataExtractor> about_sync_data_extractor_;
+ AboutSyncDataDelegate about_sync_data_delegate_;
base::WeakPtrFactory<SyncInternalsMessageHandler> weak_ptr_factory_;
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 466b6e2..9824aec 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_;
}
- int call_count() const { return call_count_; }
- syncer::SyncService* last_service() const { return last_service_; }
- SigninManagerBase* last_signin() const { return last_signin_; }
+ void RemoveObserver(SyncServiceObserver* observer) override {
+ ++remove_observer_count_;
+ }
+
+ void AddTypeDebugInfoObserver(TypeDebugInfoObserver* observer) override {
+ ++add_type_debug_info_observer_count_;
+ }
+
+ 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 @@
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 @@
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 @@
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;
};
+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();
+
+ 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, about_sync_data_delegate_call_count());
+ EXPECT_NE(nullptr, last_delegate_sync_service());
+ ValidateAboutInfoCall();
+}
+
+TEST_F(SyncInternalsMessageHandlerTest, SendAboutInfoSyncDisabled) {
+ // Simulate completely disabling sync by flag or other mechanism.
+ ResetSyncService();
+
+ handler()->AllowJavascriptForTesting();
+ handler()->OnStateChanged(nullptr);
+ EXPECT_EQ(1, about_sync_data_delegate_call_count());
+ EXPECT_EQ(nullptr, last_delegate_sync_service());
+ ValidateAboutInfoCall();
+}
+
} // namespace
-
-TEST_F(SyncInternalsMessageHandlerTest, SendAboutInfoWithService) {
- handler()->OnStateChanged(nullptr);
- EXPECT_EQ(1, fake_extractor()->call_count());
- EXPECT_NE(nullptr, fake_extractor()->last_service());
- EXPECT_NE(nullptr, fake_extractor()->last_signin());
- ValidateAboutInfoCall();
-}
-
-TEST_F(SyncInternalsMessageHandlerTest, SendAboutInfoWithoutService) {
- base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kDisableSync);
- handler()->OnStateChanged(nullptr);
- EXPECT_EQ(1, fake_extractor()->call_count());
- EXPECT_EQ(nullptr, fake_extractor()->last_service());
- EXPECT_EQ(nullptr, fake_extractor()->last_signin());
- ValidateAboutInfoCall();
-}
diff --git a/components/browser_sync/profile_sync_service.cc b/components/browser_sync/profile_sync_service.cc
index df12c63..cf5f2af 100644
--- a/components/browser_sync/profile_sync_service.cc
+++ b/components/browser_sync/profile_sync_service.cc
@@ -2194,13 +2194,6 @@
return sync_prefs_.IsSyncRequested() || sync_prefs_.IsLocalSyncEnabled();
}
-SigninManagerBase* ProfileSyncService::signin() const {
- DCHECK(thread_checker_.CalledOnValidThread());
- if (!signin_)
- return nullptr;
- return signin_->GetOriginal();
-}
-
void ProfileSyncService::RequestStart() {
DCHECK(thread_checker_.CalledOnValidThread());
if (!IsSyncAllowed()) {
diff --git a/components/browser_sync/profile_sync_service.h b/components/browser_sync/profile_sync_service.h
index b4e29d8..a713ca3c 100644
--- a/components/browser_sync/profile_sync_service.h
+++ b/components/browser_sync/profile_sync_service.h
@@ -496,8 +496,6 @@
// Returns true if the syncer is waiting for new datatypes to be encrypted.
virtual bool encryption_pending() const;
- SigninManagerBase* signin() const;
-
syncer::SyncErrorController* sync_error_controller() {
return sync_error_controller_.get();
}
diff --git a/components/sync/driver/about_sync_util.cc b/components/sync/driver/about_sync_util.cc
index b1863dc..ad7ce99 100644
--- a/components/sync/driver/about_sync_util.cc
+++ b/components/sync/driver/about_sync_util.cc
@@ -278,7 +278,6 @@
// classes defined above.
std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
SyncService* service,
- SigninManagerBase* signin,
version_info::Channel channel) {
std::unique_ptr<base::DictionaryValue> about_info(
new base::DictionaryValue());
@@ -434,8 +433,8 @@
sync_id.SetValue(full_status.sync_id);
if (is_status_valid && !full_status.invalidator_client_id.empty())
invalidator_id.SetValue(full_status.invalidator_client_id);
- if (signin)
- username.SetValue(signin->GetAuthenticatedAccountInfo().email);
+ if (service->signin())
+ username.SetValue(service->signin()->GetAuthenticatedAccountInfo().email);
const SyncService::SyncTokenStatus& token_status =
service->GetSyncTokenStatus();
diff --git a/components/sync/driver/about_sync_util.h b/components/sync/driver/about_sync_util.h
index df016d8..f77088e 100644
--- a/components/sync/driver/about_sync_util.h
+++ b/components/sync/driver/about_sync_util.h
@@ -9,8 +9,6 @@
#include "components/version_info/version_info.h"
-class SigninManagerBase;
-
namespace base {
class DictionaryValue;
}
@@ -68,7 +66,6 @@
// Note that |service| may be null.
std::unique_ptr<base::DictionaryValue> ConstructAboutInformation(
SyncService* service,
- SigninManagerBase* signin,
version_info::Channel channel);
} // namespace sync_ui_util
diff --git a/components/sync/driver/about_sync_util_unittest.cc b/components/sync/driver/about_sync_util_unittest.cc
index 7a6effb..1ad4d4e4 100644
--- a/components/sync/driver/about_sync_util_unittest.cc
+++ b/components/sync/driver/about_sync_util_unittest.cc
@@ -38,8 +38,8 @@
TEST(SyncUIUtilTestAbout, ConstructAboutInformationWithUnrecoverableErrorTest) {
SyncServiceMock service;
- std::unique_ptr<base::DictionaryValue> strings(ConstructAboutInformation(
- &service, nullptr, version_info::Channel::UNKNOWN));
+ std::unique_ptr<base::DictionaryValue> strings(
+ ConstructAboutInformation(&service, version_info::Channel::UNKNOWN));
EXPECT_TRUE(strings->HasKey("unrecoverable_error_detected"));
}
diff --git a/components/sync/driver/fake_sync_service.cc b/components/sync/driver/fake_sync_service.cc
index 71abee3..e0d00fd 100644
--- a/components/sync/driver/fake_sync_service.cc
+++ b/components/sync/driver/fake_sync_service.cc
@@ -214,4 +214,8 @@
void FakeSyncService::GetAllNodes(
const base::Callback<void(std::unique_ptr<base::ListValue>)>& callback) {}
+SigninManagerBase* FakeSyncService::signin() const {
+ return nullptr;
+}
+
} // namespace syncer
diff --git a/components/sync/driver/fake_sync_service.h b/components/sync/driver/fake_sync_service.h
index 543a382..086953e 100644
--- a/components/sync/driver/fake_sync_service.h
+++ b/components/sync/driver/fake_sync_service.h
@@ -83,6 +83,7 @@
base::WeakPtr<JsController> GetJsController() override;
void GetAllNodes(const base::Callback<void(std::unique_ptr<base::ListValue>)>&
callback) override;
+ SigninManagerBase* signin() const override;
// DataTypeEncryptionHandler:
bool IsPassphraseRequired() const override;
diff --git a/components/sync/driver/sync_service.h b/components/sync/driver/sync_service.h
index 84691ad79..1b323b2f 100644
--- a/components/sync/driver/sync_service.h
+++ b/components/sync/driver/sync_service.h
@@ -20,6 +20,7 @@
#include "google_apis/gaia/google_service_auth_error.h"
class GoogleServiceAuthError;
+class SigninManagerBase;
namespace sync_sessions {
class OpenTabsUIDelegate;
@@ -347,6 +348,10 @@
const base::Callback<void(std::unique_ptr<base::ListValue>)>&
callback) = 0;
+ // Non-owning pointer to sign in logic that can be used to fetch information
+ // about the currently signed in user.
+ virtual SigninManagerBase* signin() const = 0;
+
protected:
SyncService() {}
diff --git a/components/sync/driver/sync_service_base.cc b/components/sync/driver/sync_service_base.cc
index 39e252c8..a98b3cf 100644
--- a/components/sync/driver/sync_service_base.cc
+++ b/components/sync/driver/sync_service_base.cc
@@ -78,6 +78,16 @@
observers_.RemoveObserver(observer);
}
+bool SyncServiceBase::HasObserver(const SyncServiceObserver* observer) const {
+ DCHECK(thread_checker_.CalledOnValidThread());
+ return observers_.HasObserver(observer);
+}
+
+SigninManagerBase* SyncServiceBase::signin() const {
+ DCHECK(thread_checker_.CalledOnValidThread());
+ return signin_ ? signin_->GetOriginal() : nullptr;
+}
+
// static
base::FilePath SyncServiceBase::FormatSyncDataPath(
const base::FilePath& base_directory) {
@@ -91,11 +101,6 @@
.Append(base::FilePath(kLevelDBFolderName));
}
-bool SyncServiceBase::HasObserver(const SyncServiceObserver* observer) const {
- DCHECK(thread_checker_.CalledOnValidThread());
- return observers_.HasObserver(observer);
-}
-
void SyncServiceBase::NotifyObservers() {
for (auto& observer : observers_) {
observer.OnStateChanged(this);
diff --git a/components/sync/driver/sync_service_base.h b/components/sync/driver/sync_service_base.h
index fb902b0..a012196 100644
--- a/components/sync/driver/sync_service_base.h
+++ b/components/sync/driver/sync_service_base.h
@@ -47,6 +47,7 @@
void AddObserver(SyncServiceObserver* observer) override;
void RemoveObserver(SyncServiceObserver* observer) override;
bool HasObserver(const SyncServiceObserver* observer) const override;
+ SigninManagerBase* signin() const override;
// Given base path (path to profile) formats path to "Sync Data" folder where
// sync engine stores directory database.
diff --git a/components/sync/engine/events/protocol_event.cc b/components/sync/engine/events/protocol_event.cc
index 3f7f813..629b2bb 100644
--- a/components/sync/engine/events/protocol_event.cc
+++ b/components/sync/engine/events/protocol_event.cc
@@ -4,6 +4,10 @@
#include "components/sync/engine/events/protocol_event.h"
+#include <utility>
+
+#include "base/memory/ptr_util.h"
+
namespace syncer {
ProtocolEvent::ProtocolEvent() {}
@@ -12,13 +16,11 @@
std::unique_ptr<base::DictionaryValue> ProtocolEvent::ToValue(
const ProtocolEvent& event) {
- std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
-
+ auto dict = base::MakeUnique<base::DictionaryValue>();
dict->SetDouble("time", event.GetTimestamp().ToJsTime());
dict->SetString("type", event.GetType());
dict->SetString("details", event.GetDetails());
- dict->Set("proto", event.GetProtoMessage().release());
-
+ dict->Set("proto", event.GetProtoMessage());
return dict;
}
diff --git a/ios/chrome/browser/ui/webui/sync_internals/sync_internals_message_handler.cc b/ios/chrome/browser/ui/webui/sync_internals/sync_internals_message_handler.cc
index 7b889fc..164e4c11 100644
--- a/ios/chrome/browser/ui/webui/sync_internals/sync_internals_message_handler.cc
+++ b/ios/chrome/browser/ui/webui/sync_internals/sync_internals_message_handler.cc
@@ -9,7 +9,6 @@
#include "base/logging.h"
#include "base/values.h"
#include "components/browser_sync/profile_sync_service.h"
-#include "components/signin/core/browser/signin_manager.h"
#include "components/sync/base/weak_handle.h"
#include "components/sync/driver/about_sync_util.h"
#include "components/sync/driver/sync_service.h"
@@ -19,7 +18,6 @@
#include "components/sync/engine/events/protocol_event.h"
#include "components/sync/js/js_event_details.h"
#include "ios/chrome/browser/browser_state/chrome_browser_state.h"
-#include "ios/chrome/browser/signin/signin_manager_factory.h"
#include "ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.h"
#include "ios/chrome/common/channel_info.h"
#include "ios/web/public/web_thread.h"
@@ -209,14 +207,10 @@
}
void SyncInternalsMessageHandler::SendAboutInfo() {
- ios::ChromeBrowserState* browser_state =
- ios::ChromeBrowserState::FromWebUIIOS(web_ui());
- SigninManager* signin_manager =
- ios::SigninManagerFactory::GetForBrowserState(browser_state);
syncer::SyncService* sync_service = GetSyncService();
std::unique_ptr<base::DictionaryValue> value =
- syncer::sync_ui_util::ConstructAboutInformation(
- sync_service, signin_manager, GetChannel());
+ syncer::sync_ui_util::ConstructAboutInformation(sync_service,
+ GetChannel());
web_ui()->CallJavascriptFunction(
syncer::sync_ui_util::kDispatchEvent,
base::Value(syncer::sync_ui_util::kOnAboutInfoUpdated), *value);
