Make MessagingBindings use ScriptContextSet::ForEach. This is a long-standing

TODO and should address the dead isolate issue in bug 382179.

BUG=382179
R=rockot@chromium.org
NOTRY=true

Review URL: https://codereview.chromium.org/327953002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@276529 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 188 insertions, 190 deletions

diff --git a/extensions/renderer/dispatcher.cc b/extensions/renderer/dispatcher.cc
index 7197159..6098ee9 100644
--- a/extensions/renderer/dispatcher.cc
+++ b/extensions/renderer/dispatcher.cc
@@ -391,7 +391,6 @@ void Dispatcher::DispatchEvent(const std::string& extension_id,
   // Needed for Windows compilation, since kEventBindings is declared extern.
   const char* local_event_bindings = kEventBindings;
   script_context_set_.ForEach(extension_id,
-                              NULL,  // all render views
                               base::Bind(&CallModuleMethod,
                                          local_event_bindings,
                                          kEventDispatchFunction,
@@ -576,7 +575,7 @@ void Dispatcher::OnDeliverMessage(int target_port_id, const Message& message) {
         new RequestSender::ScopedTabID(request_sender(), it->second));
   }
 
-  MessagingBindings::DeliverMessage(script_context_set_.GetAll(),
+  MessagingBindings::DeliverMessage(script_context_set_,
                                     target_port_id,
                                     message,
                                     NULL);  // All render views.
@@ -594,20 +593,18 @@ void Dispatcher::OnDispatchOnConnect(
   source_tab.GetInteger("id", &sender_tab_id);
   port_to_tab_id_map_[target_port_id] = sender_tab_id;
 
-  MessagingBindings::DispatchOnConnect(script_context_set_.GetAll(),
+  MessagingBindings::DispatchOnConnect(script_context_set_,
                                        target_port_id,
                                        channel_name,
                                        source_tab,
-                                       info.source_id,
-                                       info.target_id,
-                                       info.source_url,
+                                       info,
                                        tls_channel_id,
                                        NULL);  // All render views.
 }
 
 void Dispatcher::OnDispatchOnDisconnect(int port_id,
                                         const std::string& error_message) {
-  MessagingBindings::DispatchOnDisconnect(script_context_set_.GetAll(),
+  MessagingBindings::DispatchOnDisconnect(script_context_set_,
                                           port_id,
                                           error_message,
                                           NULL);  // All render views.
@@ -842,7 +839,6 @@ void Dispatcher::EnableCustomElementWhiteList() {
 
 void Dispatcher::UpdateBindings(const std::string& extension_id) {
   script_context_set().ForEach(extension_id,
-                               NULL,  // all render views
                                base::Bind(&Dispatcher::UpdateBindingsForContext,
                                           base::Unretained(this)));
 }
diff --git a/extensions/renderer/extension_helper.cc b/extensions/renderer/extension_helper.cc
index a3d4362..4162dda 100644
--- a/extensions/renderer/extension_helper.cc
+++ b/extensions/renderer/extension_helper.cc
@@ -267,34 +267,26 @@ void ExtensionHelper::OnExtensionDispatchOnConnect(
     const base::DictionaryValue& source_tab,
     const ExtensionMsg_ExternalConnectionInfo& info,
     const std::string& tls_channel_id) {
-  MessagingBindings::DispatchOnConnect(
-      dispatcher_->script_context_set().GetAll(),
-      target_port_id,
-      channel_name,
-      source_tab,
-      info.source_id,
-      info.target_id,
-      info.source_url,
-      tls_channel_id,
-      render_view());
+  MessagingBindings::DispatchOnConnect(dispatcher_->script_context_set(),
+                                       target_port_id,
+                                       channel_name,
+                                       source_tab,
+                                       info,
+                                       tls_channel_id,
+                                       render_view());
 }
 
 void ExtensionHelper::OnExtensionDeliverMessage(int target_id,
                                                 const Message& message) {
-  MessagingBindings::DeliverMessage(dispatcher_->script_context_set().GetAll(),
-                                    target_id,
-                                    message,
-                                    render_view());
+  MessagingBindings::DeliverMessage(
+      dispatcher_->script_context_set(), target_id, message, render_view());
 }
 
 void ExtensionHelper::OnExtensionDispatchOnDisconnect(
     int port_id,
     const std::string& error_message) {
   MessagingBindings::DispatchOnDisconnect(
-      dispatcher_->script_context_set().GetAll(),
-      port_id,
-      error_message,
-      render_view());
+      dispatcher_->script_context_set(), port_id, error_message, render_view());
 }
 
 void ExtensionHelper::OnExecuteCode(
diff --git a/extensions/renderer/messaging_bindings.cc b/extensions/renderer/messaging_bindings.cc
index d442681..fea9a19 100644
--- a/extensions/renderer/messaging_bindings.cc
+++ b/extensions/renderer/messaging_bindings.cc
@@ -237,6 +237,129 @@ class ExtensionImpl : public ObjectBackedNativeHandler {
   Dispatcher* dispatcher_;
 };
 
+void DispatchOnConnectToScriptContext(
+    int target_port_id,
+    const std::string& channel_name,
+    const base::DictionaryValue* source_tab,
+    const ExtensionMsg_ExternalConnectionInfo& info,
+    const std::string& tls_channel_id,
+    bool* port_created,
+    ScriptContext* script_context) {
+  v8::Isolate* isolate = script_context->isolate();
+  v8::HandleScope handle_scope(isolate);
+
+  scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
+
+  const std::string& source_url_spec = info.source_url.spec();
+  std::string target_extension_id = script_context->GetExtensionID();
+  const Extension* extension = script_context->extension();
+
+  v8::Handle<v8::Value> tab = v8::Null(isolate);
+  v8::Handle<v8::Value> tls_channel_id_value = v8::Undefined(isolate);
+
+  if (extension) {
+    if (!source_tab->empty() && !extension->is_platform_app())
+      tab = converter->ToV8Value(source_tab, script_context->v8_context());
+
+    ExternallyConnectableInfo* externally_connectable =
+        ExternallyConnectableInfo::Get(extension);
+    if (externally_connectable &&
+        externally_connectable->accepts_tls_channel_id) {
+      tls_channel_id_value = v8::String::NewFromUtf8(isolate,
+                                                     tls_channel_id.c_str(),
+                                                     v8::String::kNormalString,
+                                                     tls_channel_id.size());
+    }
+  }
+
+  v8::Handle<v8::Value> arguments[] = {
+      // portId
+      v8::Integer::New(isolate, target_port_id),
+      // channelName
+      v8::String::NewFromUtf8(isolate,
+                              channel_name.c_str(),
+                              v8::String::kNormalString,
+                              channel_name.size()),
+      // sourceTab
+      tab,
+      // sourceExtensionId
+      v8::String::NewFromUtf8(isolate,
+                              info.source_id.c_str(),
+                              v8::String::kNormalString,
+                              info.source_id.size()),
+      // targetExtensionId
+      v8::String::NewFromUtf8(isolate,
+                              target_extension_id.c_str(),
+                              v8::String::kNormalString,
+                              target_extension_id.size()),
+      // sourceUrl
+      v8::String::NewFromUtf8(isolate,
+                              source_url_spec.c_str(),
+                              v8::String::kNormalString,
+                              source_url_spec.size()),
+      // tlsChannelId
+      tls_channel_id_value,
+  };
+
+  v8::Handle<v8::Value> retval =
+      script_context->module_system()->CallModuleMethod(
+          "messaging", "dispatchOnConnect", arraysize(arguments), arguments);
+
+  if (!retval.IsEmpty()) {
+    CHECK(retval->IsBoolean());
+    *port_created |= retval->BooleanValue();
+  } else {
+    LOG(ERROR) << "Empty return value from dispatchOnConnect.";
+  }
+}
+
+void DeliverMessageToScriptContext(const std::string& message_data,
+                                   int target_port_id,
+                                   ScriptContext* script_context) {
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope handle_scope(isolate);
+
+  // Check to see whether the context has this port before bothering to create
+  // the message.
+  v8::Handle<v8::Value> port_id_handle =
+      v8::Integer::New(isolate, target_port_id);
+  v8::Handle<v8::Value> has_port =
+      script_context->module_system()->CallModuleMethod(
+          "messaging", "hasPort", 1, &port_id_handle);
+
+  CHECK(!has_port.IsEmpty());
+  if (!has_port->BooleanValue())
+    return;
+
+  std::vector<v8::Handle<v8::Value> > arguments;
+  arguments.push_back(v8::String::NewFromUtf8(isolate,
+                                              message_data.c_str(),
+                                              v8::String::kNormalString,
+                                              message_data.size()));
+  arguments.push_back(port_id_handle);
+  script_context->module_system()->CallModuleMethod(
+      "messaging", "dispatchOnMessage", &arguments);
+}
+
+void DispatchOnDisconnectToScriptContext(int port_id,
+                                         const std::string& error_message,
+                                         ScriptContext* script_context) {
+  v8::Isolate* isolate = script_context->isolate();
+  v8::HandleScope handle_scope(isolate);
+
+  std::vector<v8::Handle<v8::Value> > arguments;
+  arguments.push_back(v8::Integer::New(isolate, port_id));
+  if (!error_message.empty()) {
+    arguments.push_back(
+        v8::String::NewFromUtf8(isolate, error_message.c_str()));
+  } else {
+    arguments.push_back(v8::Null(isolate));
+  }
+
+  script_context->module_system()->CallModuleMethod(
+      "messaging", "dispatchOnDisconnect", &arguments);
+}
+
 }  // namespace
 
 ObjectBackedNativeHandler* MessagingBindings::Get(Dispatcher* dispatcher,
@@ -246,95 +369,23 @@ ObjectBackedNativeHandler* MessagingBindings::Get(Dispatcher* dispatcher,
 
 // static
 void MessagingBindings::DispatchOnConnect(
-    const ScriptContextSet::ContextSet& contexts,
+    const ScriptContextSet& context_set,
     int target_port_id,
     const std::string& channel_name,
     const base::DictionaryValue& source_tab,
-    const std::string& source_extension_id,
-    const std::string& target_extension_id,
-    const GURL& source_url,
+    const ExtensionMsg_ExternalConnectionInfo& info,
     const std::string& tls_channel_id,
     content::RenderView* restrict_to_render_view) {
-  v8::Isolate* isolate = v8::Isolate::GetCurrent();
-  v8::HandleScope handle_scope(isolate);
-
-  scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
-
   bool port_created = false;
-  std::string source_url_spec = source_url.spec();
-
-  // TODO(kalman): pass in the full ScriptContextSet; call ForEach.
-  for (ScriptContextSet::ContextSet::const_iterator it = contexts.begin();
-       it != contexts.end();
-       ++it) {
-    if (restrict_to_render_view &&
-        restrict_to_render_view != (*it)->GetRenderView()) {
-      continue;
-    }
-
-    // TODO(kalman): remove when ContextSet::ForEach is available.
-    if ((*it)->v8_context().IsEmpty())
-      continue;
-
-    v8::Handle<v8::Value> tab = v8::Null(isolate);
-    v8::Handle<v8::Value> tls_channel_id_value = v8::Undefined(isolate);
-    const Extension* extension = (*it)->extension();
-    if (extension) {
-      if (!source_tab.empty() && !extension->is_platform_app())
-        tab = converter->ToV8Value(&source_tab, (*it)->v8_context());
-
-      ExternallyConnectableInfo* externally_connectable =
-          ExternallyConnectableInfo::Get(extension);
-      if (externally_connectable &&
-          externally_connectable->accepts_tls_channel_id) {
-        tls_channel_id_value =
-            v8::String::NewFromUtf8(isolate,
-                                    tls_channel_id.c_str(),
-                                    v8::String::kNormalString,
-                                    tls_channel_id.size());
-      }
-    }
-
-    v8::Handle<v8::Value> arguments[] = {
-        // portId
-        v8::Integer::New(isolate, target_port_id),
-        // channelName
-        v8::String::NewFromUtf8(isolate,
-                                channel_name.c_str(),
-                                v8::String::kNormalString,
-                                channel_name.size()),
-        // sourceTab
-        tab,
-        // sourceExtensionId
-        v8::String::NewFromUtf8(isolate,
-                                source_extension_id.c_str(),
-                                v8::String::kNormalString,
-                                source_extension_id.size()),
-        // targetExtensionId
-        v8::String::NewFromUtf8(isolate,
-                                target_extension_id.c_str(),
-                                v8::String::kNormalString,
-                                target_extension_id.size()),
-        // sourceUrl
-        v8::String::NewFromUtf8(isolate,
-                                source_url_spec.c_str(),
-                                v8::String::kNormalString,
-                                source_url_spec.size()),
-        // tlsChannelId
-        tls_channel_id_value,
-    };
-
-    v8::Handle<v8::Value> retval = (*it)->module_system()->CallModuleMethod(
-        "messaging", "dispatchOnConnect", arraysize(arguments), arguments);
-
-    if (retval.IsEmpty()) {
-      LOG(ERROR) << "Empty return value from dispatchOnConnect.";
-      continue;
-    }
-
-    CHECK(retval->IsBoolean());
-    port_created |= retval->BooleanValue();
-  }
+  context_set.ForEach(info.target_id,
+                      restrict_to_render_view,
+                      base::Bind(&DispatchOnConnectToScriptContext,
+                                 target_port_id,
+                                 channel_name,
+                                 &source_tab,
+                                 info,
+                                 tls_channel_id,
+                                 &port_created));
 
   // If we didn't create a port, notify the other end of the channel (treat it
   // as a disconnect).
@@ -346,7 +397,7 @@ void MessagingBindings::DispatchOnConnect(
 
 // static
 void MessagingBindings::DeliverMessage(
-    const ScriptContextSet::ContextSet& contexts,
+    const ScriptContextSet& context_set,
     int target_port_id,
     const Message& message,
     content::RenderView* restrict_to_render_view) {
@@ -357,77 +408,20 @@ void MessagingBindings::DeliverMessage(
     allow_window_focus.reset(new blink::WebScopedWindowFocusAllowedIndicator);
   }
 
-  v8::Isolate* isolate = v8::Isolate::GetCurrent();
-  v8::HandleScope handle_scope(isolate);
-
-  // TODO(kalman): pass in the full ScriptContextSet; call ForEach.
-  for (ScriptContextSet::ContextSet::const_iterator it = contexts.begin();
-       it != contexts.end();
-       ++it) {
-    if (restrict_to_render_view &&
-        restrict_to_render_view != (*it)->GetRenderView()) {
-      continue;
-    }
-
-    // TODO(kalman): remove when ContextSet::ForEach is available.
-    if ((*it)->v8_context().IsEmpty())
-      continue;
-
-    // Check to see whether the context has this port before bothering to create
-    // the message.
-    v8::Handle<v8::Value> port_id_handle =
-        v8::Integer::New(isolate, target_port_id);
-    v8::Handle<v8::Value> has_port = (*it)->module_system()->CallModuleMethod(
-        "messaging", "hasPort", 1, &port_id_handle);
-
-    CHECK(!has_port.IsEmpty());
-    if (!has_port->BooleanValue())
-      continue;
-
-    std::vector<v8::Handle<v8::Value> > arguments;
-    arguments.push_back(v8::String::NewFromUtf8(isolate,
-                                                message.data.c_str(),
-                                                v8::String::kNormalString,
-                                                message.data.size()));
-    arguments.push_back(port_id_handle);
-    (*it)->module_system()->CallModuleMethod(
-        "messaging", "dispatchOnMessage", &arguments);
-  }
+  context_set.ForEach(
+      restrict_to_render_view,
+      base::Bind(&DeliverMessageToScriptContext, message.data, target_port_id));
 }
 
 // static
 void MessagingBindings::DispatchOnDisconnect(
-    const ScriptContextSet::ContextSet& contexts,
+    const ScriptContextSet& context_set,
     int port_id,
     const std::string& error_message,
     content::RenderView* restrict_to_render_view) {
-  v8::Isolate* isolate = v8::Isolate::GetCurrent();
-  v8::HandleScope handle_scope(isolate);
-
-  // TODO(kalman): pass in the full ScriptContextSet; call ForEach.
-  for (ScriptContextSet::ContextSet::const_iterator it = contexts.begin();
-       it != contexts.end();
-       ++it) {
-    if (restrict_to_render_view &&
-        restrict_to_render_view != (*it)->GetRenderView()) {
-      continue;
-    }
-
-    // TODO(kalman): remove when ContextSet::ForEach is available.
-    if ((*it)->v8_context().IsEmpty())
-      continue;
-
-    std::vector<v8::Handle<v8::Value> > arguments;
-    arguments.push_back(v8::Integer::New(isolate, port_id));
-    if (!error_message.empty()) {
-      arguments.push_back(
-          v8::String::NewFromUtf8(isolate, error_message.c_str()));
-    } else {
-      arguments.push_back(v8::Null(isolate));
-    }
-    (*it)->module_system()->CallModuleMethod(
-        "messaging", "dispatchOnDisconnect", &arguments);
-  }
+  context_set.ForEach(
+      restrict_to_render_view,
+      base::Bind(&DispatchOnDisconnectToScriptContext, port_id, error_message));
 }
 
 }  // namespace extensions
diff --git a/extensions/renderer/messaging_bindings.h b/extensions/renderer/messaging_bindings.h
index 721b434..422dcae 100644
--- a/extensions/renderer/messaging_bindings.h
+++ b/extensions/renderer/messaging_bindings.h
@@ -9,6 +9,8 @@
 
 #include "extensions/renderer/script_context_set.h"
 
+struct ExtensionMsg_ExternalConnectionInfo;
+
 namespace base {
 class DictionaryValue;
 }
@@ -25,6 +27,7 @@ namespace extensions {
 class Dispatcher;
 struct Message;
 class ObjectBackedNativeHandler;
+class ScriptContextSet;
 
 // Manually implements JavaScript bindings for extension messaging.
 //
@@ -38,29 +41,27 @@ class MessagingBindings {
                                         ScriptContext* context);
 
   // Dispatches the onConnect content script messaging event to some contexts
-  // in |contexts|. If |restrict_to_render_view| is specified, only contexts in
-  // that render view will receive the message.
-  static void DispatchOnConnect(const ScriptContextSet::ContextSet& contexts,
+  // in |context_set|. If |restrict_to_render_view| is specified, only contexts
+  // in that render view will receive the message.
+  static void DispatchOnConnect(const ScriptContextSet& context_set,
                                 int target_port_id,
                                 const std::string& channel_name,
                                 const base::DictionaryValue& source_tab,
-                                const std::string& source_extension_id,
-                                const std::string& target_extension_id,
-                                const GURL& source_url,
+                                const ExtensionMsg_ExternalConnectionInfo& info,
                                 const std::string& tls_channel_id,
                                 content::RenderView* restrict_to_render_view);
 
   // Delivers a message sent using content script messaging to some of the
   // contexts in |bindings_context_set|. If |restrict_to_render_view| is
   // specified, only contexts in that render view will receive the message.
-  static void DeliverMessage(const ScriptContextSet::ContextSet& context_set,
+  static void DeliverMessage(const ScriptContextSet& context_set,
                              int target_port_id,
                              const Message& message,
                              content::RenderView* restrict_to_render_view);
 
   // Dispatches the onDisconnect event in response to the channel being closed.
   static void DispatchOnDisconnect(
-      const ScriptContextSet::ContextSet& context_set,
+      const ScriptContextSet& context_set,
       int port_id,
       const std::string& error_message,
       content::RenderView* restrict_to_render_view);
diff --git a/extensions/renderer/resources/messaging.js b/extensions/renderer/resources/messaging.js
index c7a45de..697002c 100644
--- a/extensions/renderer/resources/messaging.js
+++ b/extensions/renderer/resources/messaging.js
@@ -12,6 +12,7 @@
   var Event = require('event_bindings').Event;
   var lastError = require('lastError');
   var logActivity = requireNative('activityLogger');
+  var logging = requireNative('logging');
   var messagingNatives = requireNative('messaging_natives');
   var processNatives = requireNative('process');
   var unloadEvent = require('unload_event');
@@ -227,8 +228,10 @@
     // channels were opened to and from the same process, closing one would
     // close both.
     var extensionId = processNatives.GetExtensionId();
-    if (targetExtensionId != extensionId)
-      return false;  // not for us
+
+    // messaging_bindings.cc should ensure that this method only gets called for
+    // the right extension.
+    logging.CHECK(targetExtensionId == extensionId);
 
     if (ports[getOppositePortId(portId)])
       return false;  // this channel was opened by us, so ignore it
diff --git a/extensions/renderer/script_context.cc b/extensions/renderer/script_context.cc
index b4eeefd..60a558b 100644
--- a/extensions/renderer/script_context.cc
+++ b/extensions/renderer/script_context.cc
@@ -7,6 +7,7 @@
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/values.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/renderer/render_view.h"
@@ -58,8 +59,8 @@ void ScriptContext::Invalidate() {
   v8_context_.reset();
 }
 
-std::string ScriptContext::GetExtensionID() const {
-  return extension_.get() ? extension_->id() : std::string();
+const std::string& ScriptContext::GetExtensionID() const {
+  return extension_.get() ? extension_->id() : base::EmptyString();
 }
 
 content::RenderView* ScriptContext::GetRenderView() const {
diff --git a/extensions/renderer/script_context.h b/extensions/renderer/script_context.h
index d5f1ae1..26002ec 100644
--- a/extensions/renderer/script_context.h
+++ b/extensions/renderer/script_context.h
@@ -45,7 +45,7 @@ class ScriptContext : public RequestSender::Source {
   bool is_valid() const { return !v8_context_.IsEmpty(); }
 
   v8::Handle<v8::Context> v8_context() const {
-    return v8_context_.NewHandle(v8::Isolate::GetCurrent());
+    return v8_context_.NewHandle(isolate());
   }
 
   const Extension* extension() const { return extension_.get(); }
@@ -66,7 +66,7 @@ class ScriptContext : public RequestSender::Source {
 
   // Returns the ID of the extension associated with this context, or empty
   // string if there is no such extension.
-  std::string GetExtensionID() const;
+  const std::string& GetExtensionID() const;
 
   // Returns the RenderView associated with this context. Can return NULL if the
   // context is in the process of being destroyed.
diff --git a/extensions/renderer/script_context_set.h b/extensions/renderer/script_context_set.h
index a6be82d..aa7b253 100644
--- a/extensions/renderer/script_context_set.h
+++ b/extensions/renderer/script_context_set.h
@@ -66,11 +66,22 @@ class ScriptContextSet {
   // Synchronously runs |callback| with each ScriptContext that belongs to
   // |extension_id| in |render_view|.
   //
-  // |extension_id| may be "" to match all extensions.
-  // |render_view| may be NULL to match all render views.
+  // An empty |extension_id| will match all extensions, and a NULL |render_view|
+  // will match all render views, but try to use the inline variants of these
+  // methods instead.
   void ForEach(const std::string& extension_id,
                content::RenderView* render_view,
                const base::Callback<void(ScriptContext*)>& callback) const;
+  // ForEach which matches all extensions.
+  void ForEach(content::RenderView* render_view,
+               const base::Callback<void(ScriptContext*)>& callback) const {
+    ForEach("", render_view, callback);
+  }
+  // ForEach which matches all render views.
+  void ForEach(const std::string& extension_id,
+               const base::Callback<void(ScriptContext*)>& callback) const {
+    ForEach(extension_id, NULL, callback);
+  }
 
   // Cleans up contexts belonging to an unloaded extension.
   //