[Sync] Migrate SyncInternalsMessageHandler off CallJavascriptFunctionUnsafe.

chrome/browser/ui/webui/sync_internals_message_handler.cc

Index: chrome/browser/ui/webui/sync_internals_message_handler.cc
diff --git a/chrome/browser/ui/webui/sync_internals_message_handler.cc b/chrome/browser/ui/webui/sync_internals_message_handler.cc
index 4e29f99aa8637096092f329813a2d08459577e35..c74f9f1a97b9e8d1bb4260d4035cd08ed2a83611 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,36 @@
 #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);
-
-  ProfileSyncService* service = GetProfileSyncService();
-  if (service && service->HasObserver(this)) {
-    service->RemoveObserver(this);
-    service->RemoveProtocolEventObserver(this);
-  }
+  UnregisterModelNotifications();
+}
 
-  if (service && is_registered_for_counters_) {
-    service->RemoveTypeDebugInfoObserver(this);
-  }
+void SyncInternalsMessageHandler::OnJavascriptDisallowed() {
+  UnregisterModelNotifications();

[Dan Beam, 2017/05/22 23:31:43]

can we just call weak_ptr_factory_.InvalidateWeakPtrs() here to sever the
connection to any async, long-living callbacks?  this doesn't really change much
about existing code and just ensures what when the page is refreshed or a
renderer dies, there will be no callbacks called until javascript is allowed
again.

if the JS (as it's initialized) always calls the same messages to get
information, disconnecting the weak pointers for existing calls and then
re-doing the work when the JS is created seems like the simplest way.

[skym, 2017/05/24 00:20:16]

Woah. It seems I can call WeakPtrFactory::Invalidate(), and then start handing
out new weak pointers and the new ones work from that same factory object. I
didn't think it was going to be so easy. Done.

 }
 
 void SyncInternalsMessageHandler::RegisterMessages() {
@@ -96,13 +88,14 @@ void SyncInternalsMessageHandler::RegisterMessages() {
 }
 
 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,10 +106,11 @@ void SyncInternalsMessageHandler::HandleRegisterForEvents(
 }
 
 void SyncInternalsMessageHandler::HandleRegisterForPerTypeCounters(
-    const base::ListValue* args) {
+    const ListValue* args) {
   DCHECK(args->empty());
+  AllowJavascript();
 
-  if (ProfileSyncService* service = GetProfileSyncService()) {
+  if (SyncService* service = sync_service_provider_.Run()) {

[Dan Beam, 2017/05/22 23:31:43]

i think it's fairly uncommon to do assignment in a conditional (outside of
Blink)

can we split this into 2 lines?  i know the old code did this but i never ever
see this in webui code

SyncService* service = sync_service_provider_.Run();
if (!service)
  return;

see also: L155

[skym, 2017/05/24 00:20:16]

Moved this invocation out of the if check, added an early return. I didn't
change line 155, not sure if you were asking me to or just referencing where we
don't put it into the if check.

     if (!is_registered_for_counters_) {
       service->AddTypeDebugInfoObserver(this);
       is_registered_for_counters_ = true;
@@ -129,35 +123,36 @@ void SyncInternalsMessageHandler::HandleRegisterForPerTypeCounters(
 }
 
 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());
-  ModelTypeSet protocol_types = syncer::ProtocolTypes();
-  for (ModelTypeSet::Iterator it = protocol_types.First();
-       it.Good(); it.Inc()) {
+  AllowJavascript();
+
+  DictionaryValue event_details;
+  auto type_list = base::MakeUnique<ListValue>();
+  for (ModelTypeSet::Iterator it = syncer::ProtocolTypes().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) {
     service->GetAllNodes(
         base::Bind(&SyncInternalsMessageHandler::OnReceivedAllNodes,
@@ -167,23 +162,24 @@ void SyncInternalsMessageHandler::HandleGetAllNodes(
 
 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) {
+  // This is a callback that's invoked asyncrhonously. However, it's possible
+  // that our UI is no longer allowing javascript, and we need to manually check
+  // before proceeding.
+  if (IsJavascriptAllowed()) {
+    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 +203,12 @@ void SyncInternalsMessageHandler::OnStatusCountersUpdated(
 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 +216,45 @@ void SyncInternalsMessageHandler::HandleJsEvent(
     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);
-}
-
-// 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());
+  std::unique_ptr<DictionaryValue> value = about_sync_data_delegate_.Run(
+      sync_service_provider_.Run(), chrome::GetChannel());
+  DispatchEvent(syncer::sync_ui_util::kOnAboutInfoUpdated, *value);
+}
+
+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() {
+  // Cannot use ScopedObserver to do all the tracking because most don't follow
+  // AddObserver/RemoveObserver method naming style.
+  SyncService* service = sync_service_provider_.Run();

[Dan Beam, 2017/05/22 23:31:43]

can we get this later (inside both ifs) or early out if it's null?

[skym, 2017/05/24 00:20:16]

Done, and removed the DCHECKs that don't make sense anymore.

+  if (is_registered_) {
+    DCHECK(js_controller_);
+    DCHECK(service);
+
+    service->RemoveObserver(this);
+    service->RemoveProtocolEventObserver(this);
+    js_controller_->RemoveJsEventHandler(this);
+
+    js_controller_ = nullptr;
+    is_registered_ = false;
+  }
+
+  if (is_registered_for_counters_) {
+    DCHECK(service);
+    service->RemoveTypeDebugInfoObserver(this);
+    is_registered_for_counters_ = false;
+  }
 }