[Sync] Migrate SyncInternalsMessageHandler off CallJavascriptFunctionUnsafe.

chrome/browser/ui/webui/sync_internals_message_handler.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 "chrome/browser/ui/webui/sync_internals_message_handler.h"
 
+#include <stdint.h>
+
 #include <utility>
 #include <vector>
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/profiles/profile.h"
 #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"
 #include "components/sync/engine/cycle/commit_counters.h"
 #include "components/sync/engine/cycle/status_counters.h"
 #include "components/sync/engine/cycle/update_counters.h"
 #include "components/sync/engine/events/protocol_event.h"
 #include "components/sync/js/js_event_details.h"
 #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() {
+  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() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   web_ui()->RegisterMessageCallback(
       syncer::sync_ui_util::kRegisterForEvents,
       base::Bind(&SyncInternalsMessageHandler::HandleRegisterForEvents,
                  base::Unretained(this)));
 
@@ skipping 12 matching lines @@
       base::Bind(&SyncInternalsMessageHandler::HandleRequestListOfTypes,
                  base::Unretained(this)));
 
   web_ui()->RegisterMessageCallback(
       syncer::sync_ui_util::kGetAllNodes,
       base::Bind(&SyncInternalsMessageHandler::HandleGetAllNodes,
                  base::Unretained(this)));
 }
 
 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);
     js_controller_ = service->GetJsController();
     js_controller_->AddJsEventHandler(this);
     is_registered_ = true;
   }
 }
 
 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;
     } 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());
-  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,
                    weak_ptr_factory_.GetWeakPtr(), request_id));
   }
 }
 
 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(
     syncer::ModelType type,
     const syncer::CommitCounters& counters) {
   EmitCounterUpdate(type, syncer::sync_ui_util::kCommit, counters.ToValue());
 }
 
 void SyncInternalsMessageHandler::OnUpdateCountersUpdated(
     syncer::ModelType type,
     const syncer::UpdateCounters& counters) {
   EmitCounterUpdate(type, syncer::sync_ui_util::kUpdate, counters.ToValue());
 }
 
 void SyncInternalsMessageHandler::OnStatusCountersUpdated(
     syncer::ModelType type,
     const syncer::StatusCounters& counters) {
   EmitCounterUpdate(type, syncer::sync_ui_util::kStatus, counters.ToValue());
 }
 
 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(
     const std::string& name,
     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() {
+  // 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;
+  }
+}