[Extensions Bindings] Have the bindings system own the IPCMessageSender

Move ownership of the ICPMessageSender to the bindings system. This
internalizes a bit more of the logic into the ExtensionBindingsSystem,
and moves a bit more out of the overly-busy Dispatcher classes.

Bug: 653596
Change-Id: I321afadb64c8351ca95cb0953a0148c71ad7f005
Reviewed-on: https://chromium-review.googlesource.com/570932
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487291}

extensions/renderer/dispatcher.cc

@@ -244,25 +244,23 @@ Dispatcher::Dispatcher(DispatcherDelegate* delegate)
       content_watcher_(new ContentWatcher()),
       source_map_(&ResourceBundle::GetSharedInstance()),
       v8_schema_registry_(new V8SchemaRegistry),
-      ipc_message_sender_(IPCMessageSender::CreateMainThreadIPCMessageSender()),
       user_script_set_manager_observer_(this),
       activity_logging_enabled_(false) {
   const base::CommandLine& command_line =
       *(base::CommandLine::ForCurrentProcess());
 
+  std::unique_ptr<IPCMessageSender> ipc_message_sender =
+      IPCMessageSender::CreateMainThreadIPCMessageSender();
   if (FeatureSwitch::native_crx_bindings()->IsEnabled()) {
     // This Unretained is safe because the IPCMessageSender is guaranteed to
     // outlive the bindings system.
     auto system = base::MakeUnique<NativeExtensionBindingsSystem>(
-        base::Bind(&IPCMessageSender::SendRequestIPC,
-                   base::Unretained(ipc_message_sender_.get())),
-        base::Bind(&SendEventListenersIPC));
+        std::move(ipc_message_sender), base::Bind(&SendEventListenersIPC));
     delegate_->InitializeBindingsSystem(this, system->api_system());
     bindings_system_ = std::move(system);
   } else {
     bindings_system_ = base::MakeUnique<JsExtensionBindingsSystem>(
-        &source_map_,
-        base::MakeUnique<RequestSender>(ipc_message_sender_.get()));
+        &source_map_, std::move(ipc_message_sender));
   }
 
   set_idle_notifications_ =
@@ -648,7 +646,6 @@ void Dispatcher::OnExtensionResponse(int request_id,
                                      const base::ListValue& response,
                                      const std::string& error) {
   bindings_system_->HandleResponse(request_id, success, response, error);
-  ipc_message_sender_->SendOnRequestResponseReceivedIPC(request_id);
 }
 
 void Dispatcher::DispatchEvent(const std::string& extension_id,

extensions/renderer/dispatcher.h

@@ -56,7 +56,6 @@ namespace extensions {
 class ContentWatcher;
 class DispatcherDelegate;
 class ExtensionBindingsSystem;
-class IPCMessageSender;
 class ScriptContext;
 class ScriptInjectionManager;
 struct EventFilteringInfo;
@@ -280,9 +279,6 @@ class Dispatcher : public content::RenderThreadObserver,
   // Cache for the v8 representation of extension API schemas.
   std::unique_ptr<V8SchemaRegistry> v8_schema_registry_;
 
-  // A helper to dispatch event and request IPCs.
-  std::unique_ptr<IPCMessageSender> ipc_message_sender_;
-
   // The bindings system associated with the main thread.
   std::unique_ptr<ExtensionBindingsSystem> bindings_system_;
 

extensions/renderer/extension_bindings_system.h

@@ -12,6 +12,7 @@ class ListValue;
 }
 
 namespace extensions {
+class IPCMessageSender;
 class RequestSender;
 class ScriptContext;
 struct EventFilteringInfo;
@@ -54,6 +55,9 @@ class ExtensionBindingsSystem {
                               const base::ListValue& response,
                               const std::string& error) = 0;
 
+  // Returns the associated IPC message sender.
+  virtual IPCMessageSender* GetIPCMessageSender() = 0;
+
   // Returns the associated RequestSender, if any.
   // TODO(devlin): Factor this out.
   virtual RequestSender* GetRequestSender() = 0;

extensions/renderer/js_extension_bindings_system.cc

@@ -5,6 +5,7 @@
 #include "extensions/renderer/js_extension_bindings_system.h"
 
 #include "base/command_line.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/string_split.h"
 #include "content/public/child/v8_value_converter.h"
 #include "content/public/common/content_switches.h"
@@ -18,7 +19,9 @@
 #include "extensions/common/manifest_handlers/externally_connectable.h"
 #include "extensions/renderer/binding_generating_native_handler.h"
 #include "extensions/renderer/event_bindings.h"
+#include "extensions/renderer/ipc_message_sender.h"
 #include "extensions/renderer/renderer_extension_registry.h"
+#include "extensions/renderer/request_sender.h"
 #include "extensions/renderer/resource_bundle_source_map.h"
 #include "extensions/renderer/script_context.h"
 #include "gin/converter.h"
@@ -134,8 +137,11 @@ void MaybeCreateEventBindings(ScriptContext* context) {
 
 JsExtensionBindingsSystem::JsExtensionBindingsSystem(
     ResourceBundleSourceMap* source_map,
-    std::unique_ptr<RequestSender> request_sender)
-    : source_map_(source_map), request_sender_(std::move(request_sender)) {}
+    std::unique_ptr<IPCMessageSender> ipc_message_sender)
+    : source_map_(source_map),
+      ipc_message_sender_(std::move(ipc_message_sender)),
+      request_sender_(
+          base::MakeUnique<RequestSender>(ipc_message_sender_.get())) {}
 
 JsExtensionBindingsSystem::~JsExtensionBindingsSystem() {}
 
@@ -224,12 +230,17 @@ void JsExtensionBindingsSystem::HandleResponse(int request_id,
                                                const base::ListValue& response,
                                                const std::string& error) {
   request_sender_->HandleResponse(request_id, success, response, error);
+  ipc_message_sender_->SendOnRequestResponseReceivedIPC(request_id);
 }
 
 RequestSender* JsExtensionBindingsSystem::GetRequestSender() {
   return request_sender_.get();
 }
 
+IPCMessageSender* JsExtensionBindingsSystem::GetIPCMessageSender() {
+  return ipc_message_sender_.get();
+}
+
 void JsExtensionBindingsSystem::DispatchEventInContext(
     const std::string& event_name,
     const base::ListValue* event_args,

extensions/renderer/js_extension_bindings_system.h

@@ -12,14 +12,14 @@
 #include "extensions/renderer/extension_bindings_system.h"
 
 namespace extensions {
-class RequestSender;
+class IPCMessageSender;
 class ResourceBundleSourceMap;
 
 // The bindings system using the traditional JS-injection style bindings.
 class JsExtensionBindingsSystem : public ExtensionBindingsSystem {
  public:
   JsExtensionBindingsSystem(ResourceBundleSourceMap* source_map,
-                            std::unique_ptr<RequestSender> request_sender);
+                            std::unique_ptr<IPCMessageSender> request_sender);
   ~JsExtensionBindingsSystem() override;
 
   // ExtensionBindingsSystem:
@@ -37,6 +37,7 @@ class JsExtensionBindingsSystem : public ExtensionBindingsSystem {
                       const base::ListValue& response,
                       const std::string& error) override;
   RequestSender* GetRequestSender() override;
+  IPCMessageSender* GetIPCMessageSender() override;
 
  private:
   void RegisterBinding(const std::string& api_name,
@@ -45,6 +46,8 @@ class JsExtensionBindingsSystem : public ExtensionBindingsSystem {
 
   ResourceBundleSourceMap* source_map_ = nullptr;
 
+  std::unique_ptr<IPCMessageSender> ipc_message_sender_;
+
   std::unique_ptr<RequestSender> request_sender_;
 
   DISALLOW_COPY_AND_ASSIGN(JsExtensionBindingsSystem);

extensions/renderer/native_extension_bindings_system.cc

@@ -21,6 +21,7 @@
 #include "extensions/renderer/console.h"
 #include "extensions/renderer/content_setting.h"
 #include "extensions/renderer/declarative_content_hooks_delegate.h"
+#include "extensions/renderer/ipc_message_sender.h"
 #include "extensions/renderer/module_system.h"
 #include "extensions/renderer/script_context.h"
 #include "extensions/renderer/script_context_set.h"
@@ -349,9 +350,9 @@ v8::Local<v8::Object> CreateFullBinding(
 }  // namespace
 
 NativeExtensionBindingsSystem::NativeExtensionBindingsSystem(
-    const SendRequestIPCMethod& send_request_ipc,
+    std::unique_ptr<IPCMessageSender> ipc_message_sender,
     const SendEventListenerIPCMethod& send_event_listener_ipc)
-    : send_request_ipc_(send_request_ipc),
+    : ipc_message_sender_(std::move(ipc_message_sender)),
       send_event_listener_ipc_(send_event_listener_ipc),
       api_system_(
           base::Bind(&CallJsFunction),
@@ -559,12 +560,17 @@ void NativeExtensionBindingsSystem::HandleResponse(
   api_system_.CompleteRequest(
       request_id, response,
       !success && error.empty() ? "Unknown error." : error);
+  ipc_message_sender_->SendOnRequestResponseReceivedIPC(request_id);
 }
 
 RequestSender* NativeExtensionBindingsSystem::GetRequestSender() {
   return nullptr;
 }
 
+IPCMessageSender* NativeExtensionBindingsSystem::GetIPCMessageSender() {
+  return ipc_message_sender_.get();
+}
+
 void NativeExtensionBindingsSystem::BindingAccessor(
     v8::Local<v8::Name> name,
     const v8::PropertyCallbackInfo<v8::Value>& info) {
@@ -733,7 +739,8 @@ void NativeExtensionBindingsSystem::SendRequest(
   params->worker_thread_id = -1;
   params->service_worker_version_id = kInvalidServiceWorkerVersionId;
 
-  send_request_ipc_.Run(script_context, std::move(params), request->thread);
+  ipc_message_sender_->SendRequestIPC(script_context, std::move(params),
+                                      request->thread);
 }
 
 void NativeExtensionBindingsSystem::OnEventListenerChanged(

extensions/renderer/native_extension_bindings_system.h

@@ -16,9 +16,8 @@
 #include "extensions/renderer/extension_bindings_system.h"
 #include "v8/include/v8.h"
 
-struct ExtensionHostMsg_Request_Params;
-
 namespace extensions {
+class IPCMessageSender;
 class ScriptContext;
 
 // The implementation of the Bindings System for extensions code with native
@@ -29,11 +28,6 @@ class ScriptContext;
 // Designed to be used in a single thread, but for all contexts on that thread.
 class NativeExtensionBindingsSystem : public ExtensionBindingsSystem {
  public:
-  // TODO(devlin): Instead, pass in an IPCMessageSender.
-  using SendRequestIPCMethod =
-      base::Callback<void(ScriptContext*,
-                          std::unique_ptr<ExtensionHostMsg_Request_Params>,
-                          binding::RequestThread)>;
   using SendEventListenerIPCMethod =
       base::Callback<void(binding::EventListenersChanged,
                           ScriptContext*,
@@ -42,7 +36,7 @@ class NativeExtensionBindingsSystem : public ExtensionBindingsSystem {
                           bool was_manual)>;
 
   NativeExtensionBindingsSystem(
-      const SendRequestIPCMethod& send_request_ipc,
+      std::unique_ptr<IPCMessageSender> ipc_message_sender,
       const SendEventListenerIPCMethod& send_event_listener_ipc);
   ~NativeExtensionBindingsSystem() override;
 
@@ -61,6 +55,7 @@ class NativeExtensionBindingsSystem : public ExtensionBindingsSystem {
                       const base::ListValue& response,
                       const std::string& error) override;
   RequestSender* GetRequestSender() override;
+  IPCMessageSender* GetIPCMessageSender() override;
 
   APIBindingsSystem* api_system() { return &api_system_; }
 
@@ -101,8 +96,7 @@ class NativeExtensionBindingsSystem : public ExtensionBindingsSystem {
   void GetJSBindingUtil(v8::Local<v8::Context> context,
                         v8::Local<v8::Value>* binding_util_out);
 
-  // Handler to send request IPCs. Abstracted out for testing purposes.
-  SendRequestIPCMethod send_request_ipc_;
+  std::unique_ptr<IPCMessageSender> ipc_message_sender_;
 
   // Handler to notify the browser of event registrations. Abstracted out for
   // testing purposes.

extensions/renderer/native_extension_bindings_system_unittest.cc

@@ -18,6 +18,7 @@
 #include "extensions/renderer/bindings/api_binding_test.h"
 #include "extensions/renderer/bindings/api_binding_test_util.h"
 #include "extensions/renderer/bindings/api_invocation_errors.h"
+#include "extensions/renderer/ipc_message_sender.h"
 #include "extensions/renderer/module_system.h"
 #include "extensions/renderer/safe_builtins.h"
 #include "extensions/renderer/script_context.h"
@@ -88,6 +89,29 @@ bool PropertyExists(v8::Local<v8::Context> context,
   return !value->IsUndefined();
 };
 
+class TestIPCMessageSender : public IPCMessageSender {
+ public:
+  TestIPCMessageSender() {}
+  ~TestIPCMessageSender() override {}
+
+  // IPCMessageSender:
+  void SendRequestIPC(ScriptContext* context,
+                      std::unique_ptr<ExtensionHostMsg_Request_Params> params,
+                      binding::RequestThread thread) override {
+    last_params_ = std::move(params);
+  }
+  void SendOnRequestResponseReceivedIPC(int request_id) override {}
+
+  const ExtensionHostMsg_Request_Params* last_params() const {
+    return last_params_.get();
+  }
+
+ private:
+  std::unique_ptr<ExtensionHostMsg_Request_Params> last_params_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestIPCMessageSender);
+};
+
 }  // namespace
 
 class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
@@ -105,9 +129,10 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
   void SetUp() override {
     render_thread_ = base::MakeUnique<content::MockRenderThread>();
     script_context_set_ = base::MakeUnique<ScriptContextSet>(&extension_ids_);
+    auto ipc_message_sender = base::MakeUnique<TestIPCMessageSender>();
+    ipc_message_sender_ = ipc_message_sender.get();
     bindings_system_ = base::MakeUnique<NativeExtensionBindingsSystem>(
-        base::Bind(&NativeExtensionBindingsSystemUnittest::MockSendRequestIPC,
-                   base::Unretained(this)),
+        std::move(ipc_message_sender),
         base::Bind(&NativeExtensionBindingsSystemUnittest::MockSendListenerIPC,
                    base::Unretained(this)));
     APIBindingTest::SetUp();
@@ -131,13 +156,6 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
     APIBindingTest::TearDown();
   }
 
-  void MockSendRequestIPC(
-      ScriptContext* context,
-      std::unique_ptr<ExtensionHostMsg_Request_Params> params,
-      binding::RequestThread thread) {
-    last_params_ = std::move(params);
-  }
-
   void MockSendListenerIPC(binding::EventListenersChanged changed,
                            ScriptContext* context,
                            const std::string& event_name,
@@ -187,8 +205,10 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
   NativeExtensionBindingsSystem* bindings_system() {
     return bindings_system_.get();
   }
-  bool has_last_params() const { return !!last_params_; }
-  const ExtensionHostMsg_Request_Params& last_params() { return *last_params_; }
+  bool has_last_params() const { return !!ipc_message_sender_->last_params(); }
+  const ExtensionHostMsg_Request_Params& last_params() {
+    return *ipc_message_sender_->last_params();
+  }
   StringSourceMap* source_map() { return &source_map_; }
   MockEventChangeHandler* event_change_handler() {
     return event_change_handler_.get();
@@ -200,6 +220,8 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
   std::unique_ptr<ScriptContextSet> script_context_set_;
   std::vector<ScriptContext*> raw_script_contexts_;
   std::unique_ptr<NativeExtensionBindingsSystem> bindings_system_;
+  // The TestIPCMessageSender; owned by the bindings system.
+  TestIPCMessageSender* ipc_message_sender_ = nullptr;
 
   std::unique_ptr<ExtensionHostMsg_Request_Params> last_params_;
   std::unique_ptr<MockEventChangeHandler> event_change_handler_;

extensions/renderer/service_worker_data.cc

@@ -5,19 +5,16 @@
 #include "extensions/renderer/service_worker_data.h"
 
 #include "extensions/renderer/extension_bindings_system.h"
-#include "extensions/renderer/ipc_message_sender.h"
 
 namespace extensions {
 
 ServiceWorkerData::ServiceWorkerData(
     int64_t service_worker_version_id,
     ScriptContext* context,
-    std::unique_ptr<ExtensionBindingsSystem> bindings_system,
-    std::unique_ptr<IPCMessageSender> ipc_message_sender)
+    std::unique_ptr<ExtensionBindingsSystem> bindings_system)
     : service_worker_version_id_(service_worker_version_id),
       context_(context),
       v8_schema_registry_(new V8SchemaRegistry),
-      ipc_message_sender_(std::move(ipc_message_sender)),
       bindings_system_(std::move(bindings_system)) {}
 
 ServiceWorkerData::~ServiceWorkerData() {}

extensions/renderer/service_worker_data.h

@@ -12,7 +12,6 @@
 
 namespace extensions {
 class ExtensionBindingsSystem;
-class IPCMessageSender;
 class ScriptContext;
 
 // Per ServiceWorker data in worker thread.
@@ -21,8 +20,7 @@ class ServiceWorkerData {
  public:
   ServiceWorkerData(int64_t service_worker_version_id,
                     ScriptContext* context,
-                    std::unique_ptr<ExtensionBindingsSystem> bindings_system,
-                    std::unique_ptr<IPCMessageSender> ipc_message_sender);
+                    std::unique_ptr<ExtensionBindingsSystem> bindings_system);
   ~ServiceWorkerData();
 
   V8SchemaRegistry* v8_schema_registry() { return v8_schema_registry_.get(); }
@@ -31,14 +29,12 @@ class ServiceWorkerData {
     return service_worker_version_id_;
   }
   ScriptContext* context() const { return context_; }
-  IPCMessageSender* ipc_message_sender() { return ipc_message_sender_.get(); }
 
  private:
   const int64_t service_worker_version_id_;
   ScriptContext* const context_;
 
   std::unique_ptr<V8SchemaRegistry> v8_schema_registry_;
-  std::unique_ptr<IPCMessageSender> ipc_message_sender_;
   std::unique_ptr<ExtensionBindingsSystem> bindings_system_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerData);

extensions/renderer/worker_thread_dispatcher.cc

@@ -135,7 +135,6 @@ void WorkerThreadDispatcher::OnResponseWorker(int worker_thread_id,
   ServiceWorkerData* data = g_data_tls.Pointer()->Get();
   data->bindings_system()->HandleResponse(request_id, succeeded, response,
                                           error);
-  data->ipc_message_sender()->SendOnRequestResponseReceivedIPC(request_id);
 }
 
 void WorkerThreadDispatcher::OnDispatchEvent(
@@ -165,17 +164,13 @@ void WorkerThreadDispatcher::AddWorkerData(
       // The Unretained below is safe since the IPC message sender outlives the
       // bindings system.
       bindings_system = base::MakeUnique<NativeExtensionBindingsSystem>(
-          base::Bind(&IPCMessageSender::SendRequestIPC,
-                     base::Unretained(ipc_message_sender.get())),
-          base::Bind(&SendEventListenersIPC));
+          std::move(ipc_message_sender), base::Bind(&SendEventListenersIPC));
     } else {
       bindings_system = base::MakeUnique<JsExtensionBindingsSystem>(
-          source_map,
-          base::MakeUnique<RequestSender>(ipc_message_sender.get()));
+          source_map, std::move(ipc_message_sender));
     }
     ServiceWorkerData* new_data = new ServiceWorkerData(
-        service_worker_version_id, context, std::move(bindings_system),
-        std::move(ipc_message_sender));
+        service_worker_version_id, context, std::move(bindings_system));
     g_data_tls.Pointer()->Set(new_data);
   }