[Extensions Bindings] Add Events support

extensions/renderer/api_event_handler.cc

Index: extensions/renderer/api_event_handler.cc
diff --git a/extensions/renderer/api_event_handler.cc b/extensions/renderer/api_event_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..eb606654b7ad5a0fd82edcc17870c8423dd42b6f
--- /dev/null
+++ b/extensions/renderer/api_event_handler.cc
@@ -0,0 +1,222 @@
+// Copyright 2016 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 "extensions/renderer/api_event_handler.h"
+
+#include <algorithm>
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "base/values.h"
+#include "content/public/child/v8_value_converter.h"
+#include "gin/arguments.h"
+#include "gin/per_context_data.h"
+
+namespace extensions {
+
+namespace {
+
+const char kExtensionAPIEventPerContextKey[] = "extension_api_events";
+const char kEventListenersKey[] = "event_listeners";
+
+void ForwardToGinHandler(const v8::FunctionCallbackInfo<v8::Value>& info) {
+  gin::Arguments args(info);
+  v8::Local<v8::External> external;
+  CHECK(args.GetData(&external));
+  auto callback =
+      static_cast<APIEventHandler::HandlerCallback*>(external->Value());
+  callback->Run(info.This(), &args);
+}
+
+}  // namespace
+
+APIEventHandler::APIEventPerContextData::APIEventPerContextData() = default;
+APIEventHandler::APIEventPerContextData::~APIEventPerContextData() = default;
+
+APIEventHandler::APIEventHandler(const binding::RunJSFunction& call_js)
+    : call_js_(call_js), weak_factory_(this) {}
+APIEventHandler::~APIEventHandler() {}
+
+v8::Local<v8::Object> APIEventHandler::CreateEventInstance(

[jbroman, 2016/11/01 18:59:24]

High-level comment here: while we needed to get closer to the V8 API to deal
with some of the quirks of the extensions IDL (and the fact that we read the
JSON dynamically at runtime), the event system does not seem similarly
constrained. Have you looked at whether things turn out simpler if you just use
gin::Wrappable for this (leaving fewer of the subtle cases to write yourself)?
Something like (modulo choice of names and header/source-file split):

// should only be used from one context (a separate instance is expected
per-context)
class EventEmitter final : public gin::Wrappable<EventEmitter> {
 public:
  static gin::WrapperInfo kWrapperInfo = {gin::kEmbedderNativeGin};

  static gin::Handle<EventEmitter> Create(v8::Isolate* isolate,
v8::Local<v8::Context> context, const std::string& event_name) {
    return gin::CreateHandle(isolate, new EventEmitter(context, event_name));
  }

  gin::ObjectTemplateBuilder GetObjectTemplateBuilder(v8::Isolate* isolate)
final {
    return Wrappable<EventEmitter>::GetObjectTemplateBuilder(isolate)
        .SetMethod("addListener", &EventEmitter::AddListener)
        .SetMethod("removeListener", &EventEmitter::RemoveListener)
        .SetMethod("hasListener", &EventEmitter::HasListener)
        .SetMethod("hasListeners", &EventEmitter::HasListeners);
  }

  EventEmitter(v8::Local<v8::Context> context, const std::string& event_name) {
    // talk to APIEventPerContextData, initialize members, etc.
  }

  void AddListener(v8::Local<v8::Function> listener) {
    if (!HasListener(listener))
      listeners_.push_back(listener);
  }
  void RemoveListener(v8::Local<v8::Function> listener) { /* ... */ }
  bool HasListener(v8::Local<v8::Function> listener) { /* ... */ }
  bool HasListeners() { return !listeners_.empty(); }

  void FireEvent(...) { ... }

 private:
  std::vector<v8::Global<v8::Function>> listeners_;
};

And then you can just extract the v8 local from the gin::Handle when you want to
attach it to things.

While gin doesn't necessarily make _optimal_ choices for this use case, it comes
close and has been vetted by jochen@ for correctness. It will also give you
things like caching of the object template (and constituent function templates),
validating the underlying wrapped object, and so on "for free".

If you do switch to gin::Wrappable, it will nullify most (all?) of my other
comments here.

[jbroman, 2016/11/01 19:14:33]

One note here: a little more caution than I've implied here is necessary to
avoid cycles between listeners and the emitter wrapper causing a leak. Probably
resolvable just by clearing out the listener list when the context is dying (or
they otherwise could not possibly be needed anymore), though there are more
sophisticated approaches (like making the globals weak and keeping them alive
through a private property).

[Devlin, 2016/11/02 00:48:29]

So, there's a few gotchas here and there with how we would have to cache and
lookup the event emitters for each event-context pair, etc, that make me a
little dubious of the full EventEmitter approach.  That said, I've managed to
take (what I think is) the main motivation of it (using gin::Wrappable,
ObjectTemplateBuilder, etc) and combine it with some of the nice bits of this
one (easy to look up event listeners for a context, dispatch events, single
object to route to).  I think it's pretty clean.  But I might have just made
Frankenstein's monster to someone's eyes if they're more knowledgable about
v8/gin. :)  Lemme know if this is on the right track!

+    const std::string& event_name,
+    v8::Local<v8::Context> context) {
+  gin::PerContextData* per_context_data = gin::PerContextData::From(context);
+  DCHECK(per_context_data);
+  APIEventPerContextData* data = static_cast<APIEventPerContextData*>(
+      per_context_data->GetUserData(kExtensionAPIEventPerContextKey));
+  if (!data) {
+    auto api_data = base::MakeUnique<APIEventPerContextData>();
+    data = api_data.get();
+    per_context_data->SetUserData(kExtensionAPIEventPerContextKey,
+                                  api_data.release());
+  }
+
+  DCHECK(data->event_data.find(event_name) == data->event_data.end());
+  auto listeners = base::MakeUnique<EventListeners>();
+
+  if (event_template_.IsEmpty())
+    InitializeTemplate(context->GetIsolate(), data);
+
+  v8::Local<v8::ObjectTemplate> local_template =
+      event_template_.Get(context->GetIsolate());
+  v8::MaybeLocal<v8::Object> maybe_object =
+      local_template->NewInstance(context);
+  v8::Local<v8::Object> object;
+  // TODO(devlin): Could this fail?
+  CHECK(maybe_object.ToLocal(&object));

[jbroman, 2016/11/01 18:59:24]

I believe this won't fail, because we know that since there is no provided
constructor function, it should use the built-in object constructor function,
which should neither throw nor configure properties in such a way that adding
addListener etc. should be able to fail. It used to be the case that problems
would happen if Object.prototype.constructor were overloaded, but I think that's
been changed since?

nit: You can probably just put this on one line (if you extract
context->GetIsolate() into a local variable, which seems helpful anyhow
considering how many times it occurs here):

CHECK(event_template_.Get(isolate)->NewInstance(context).ToLocal(&object));

[Devlin, 2016/11/02 00:48:29]

Moot if we go with the latest patch set.

+
+  // We cache the EventListeners pointer on the event object for easy lookups
+  // when adding, removing, or querying listeners from JS.
+  v8::Maybe<bool> success = object->SetPrivate(
+      context, v8::Private::ForApi(context->GetIsolate(),
+                                   gin::StringToSymbol(context->GetIsolate(),
+                                                       kEventListenersKey)),
+      v8::External::New(context->GetIsolate(), listeners.get()));
+  DCHECK(success.IsJust());
+  DCHECK(success.FromJust());
+
+  data->event_data.insert(std::make_pair(event_name, std::move(listeners)));
+
+  return object;
+}
+
+void APIEventHandler::InitializeTemplate(v8::Isolate* isolate,
+                                         APIEventPerContextData* data) {
+  v8::Local<v8::ObjectTemplate> event_template =
+      v8::ObjectTemplate::New(isolate);
+
+  {
+    struct Method {
+      const char* name;
+      void (APIEventHandler::*handler)(v8::Local<v8::Object> object,
+                                       gin::Arguments* args);
+    } methods[] = {
+        {"addListener", &APIEventHandler::AddListener},
+        {"removeListener", &APIEventHandler::RemoveListener},
+        {"hasListener", &APIEventHandler::HasListener},
+        {"hasListeners", &APIEventHandler::HasListeners},
+    };
+    for (const auto& method : methods) {
+      auto handler_callback = base::MakeUnique<HandlerCallback>(
+          base::Bind(method.handler, weak_factory_.GetWeakPtr()));
+      // TODO(devlin): FunctionTemplate docs say "There can only be one function
+      // created from a FunctionTemplate in a context." I'm assuming that means
+      // we can't change the template, rather than we can only use it to
+      // instantiate a single function object?

[jbroman, 2016/11/01 18:59:24]

It means that if you call v8::FunctionTemplate::GetFunction with the same
context, you get back the same v8::Function (with reference equality) each time.

For instance, chrome.idle.onStateChanged.addListener ===
chrome.runtime.onMessage.addListener.

The function objects are equal, but you can distinguish the call by examining
the holder of the function callback info.

You should only have one instance of this FunctionTemplate (and similar for
ObjectTemplate) for the entire isolate. Given that, event_template_ might as
well be held with a v8::Eternal, like gin::PerIsolateData does.

[Devlin, 2016/11/02 00:48:29]

Ditto

+      v8::Local<v8::FunctionTemplate> function = v8::FunctionTemplate::New(
+          isolate, &ForwardToGinHandler,
+          v8::External::New(isolate, handler_callback.get()),
+          v8::Local<v8::Signature>(), 0, v8::ConstructorBehavior::kThrow);

[jbroman, 2016/11/01 18:59:24]

If you use a signature, you can have V8 validate that the holder is always an
instance of this ObjectTemplate, which limits the kind of weird tricks that
script can play. You'd do this by starting with a FunctionTemplate (that would
be the "constructor" of these objects, even though it wouldn't be exposed to
script), and then adding these to its PrototypeTemplate (or InstanceTemplate if
you'd really rather they be functions defined on the instance).

[Devlin, 2016/11/02 00:48:29]

Ditto

+      data->callbacks.push_back(std::move(handler_callback));
+      event_template->Set(gin::StringToSymbol(isolate, method.name), function);
+    }
+  }
+
+  event_template_ = v8::Global<v8::ObjectTemplate>(isolate, event_template);
+}
+
+void APIEventHandler::FireEventInContext(const std::string& event_name,
+                                         v8::Local<v8::Context> context,
+                                         const base::ListValue& args) {
+  gin::PerContextData* per_context_data = gin::PerContextData::From(context);
+  DCHECK(per_context_data);
+  APIEventPerContextData* data = static_cast<APIEventPerContextData*>(
+      per_context_data->GetUserData(kExtensionAPIEventPerContextKey));
+  if (!data)
+    return;
+
+  auto iter = data->event_data.find(event_name);
+  if (iter == data->event_data.end())
+    return;
+
+  EventListeners* listeners = iter->second.get();
+
+  std::vector<v8::Local<v8::Value>> v8_args;
+  v8_args.reserve(args.GetSize());
+  std::unique_ptr<content::V8ValueConverter> converter(
+      content::V8ValueConverter::create());
+  for (const auto& arg : args)
+    v8_args.push_back(converter->ToV8Value(arg.get(), context));
+
+  for (auto& listener : *listeners) {
+    call_js_.Run(listener.Get(context->GetIsolate()), context, v8_args.size(),
+                 v8_args.data());
+  }
+}
+
+const APIEventHandler::EventListeners&
+APIEventHandler::GetEventListenersForTesting(const std::string& event_name,
+                                             v8::Local<v8::Context> context) {
+  gin::PerContextData* per_context_data = gin::PerContextData::From(context);
+  DCHECK(per_context_data);
+  APIEventPerContextData* data = static_cast<APIEventPerContextData*>(
+      per_context_data->GetUserData(kExtensionAPIEventPerContextKey));
+  DCHECK(data);
+
+  auto iter = data->event_data.find(event_name);
+  DCHECK(iter != data->event_data.end());
+  return *iter->second;
+}
+
+APIEventHandler::EventListeners* APIEventHandler::GetListenersForObject(
+    v8::Local<v8::Object> object,
+    v8::Local<v8::Context> context) {
+  v8::MaybeLocal<v8::Value> maybe_value = object->GetPrivate(
+      context, v8::Private::ForApi(context->GetIsolate(),
+                                   gin::StringToSymbol(context->GetIsolate(),
+                                                       kEventListenersKey)));
+  v8::Local<v8::Value> value;
+  CHECK(maybe_value.ToLocal(&value));
+  CHECK(value->IsExternal());

[jbroman, 2016/11/01 18:59:24]

You can't rely on this working, at the moment. You're using the |this| value
provided by script, but it won't necessarily actually be an event-emitter
object.

For instance, script could do this:
chrome.runtime.onMessage.addEventListener.call(someOtherObject);

In which case these checks will fail. You should either use a signature (and
correspondingly, a FunctionTemplate for the hidden constructor of this type of
object) to have V8 check this before dispatching the call (this is what Blink
does) and guarantee that info.Holder() -- but not info.This() -- is actually a
wrapper object created by this class, or you should be prepared for these to
fail, and handle that case.

(Probably a fun category of cases to check with a unit test, incidentally.)

[Devlin, 2016/11/02 00:48:29]

Also moot.

+  v8::Local<v8::External> external = v8::Local<v8::External>::Cast(value);
+  return static_cast<EventListeners*>(external->Value());
+}
+
+void APIEventHandler::AddListener(v8::Local<v8::Object> caller,
+                                  gin::Arguments* args) {
+  v8::Local<v8::Function> listener;
+  if (args->Length() != 1 || !args->GetNext(&listener))
+    return;
+  EventListeners* listeners =
+      GetListenersForObject(caller, args->isolate()->GetCurrentContext());

[jbroman, 2016/11/01 18:59:24]

Didn't catch this before, but getting the context from the holder is what Blink
does, rather than using GetCurrentContext.

v8::Local<v8::Object> holder;
if (!args->GetHolder(&holder))
  return;
v8::Local<v8::Context> context = holder->CreationContext();
EventListeners* listeners = GetListenersForObject(holder, context);

[Devlin, 2016/11/02 00:48:29]

And moot :)

+  if (std::find(listeners->begin(), listeners->end(), listener) !=
+      listeners->end())
+    return;
+  listeners->push_back(v8::Global<v8::Function>(args->isolate(), listener));
+}
+
+void APIEventHandler::RemoveListener(v8::Local<v8::Object> caller,
+                                     gin::Arguments* args) {
+  v8::Local<v8::Function> listener;
+  if (args->Length() != 1 || !args->GetNext(&listener))
+    return;
+  EventListeners* listeners =
+      GetListenersForObject(caller, args->isolate()->GetCurrentContext());
+  auto iter = std::find(listeners->begin(), listeners->end(), listener);
+  if (iter != listeners->end())
+    listeners->erase(iter);
+}
+
+void APIEventHandler::HasListener(v8::Local<v8::Object> caller,
+                                  gin::Arguments* args) {
+  v8::Local<v8::Function> listener;
+  if (args->Length() != 1 || !args->GetNext(&listener))
+    return;
+  EventListeners* listeners =
+      GetListenersForObject(caller, args->isolate()->GetCurrentContext());
+  auto iter = std::find(listeners->begin(), listeners->end(), listener);
+  args->Return(iter != listeners->end());
+}
+
+void APIEventHandler::HasListeners(v8::Local<v8::Object> caller,
+                                   gin::Arguments* args) {
+  if (args->Length() != 0)
+    return;
+  EventListeners* listeners =
+      GetListenersForObject(caller, args->isolate()->GetCurrentContext());
+  args->Return(!listeners->empty());
+}
+
+}  // namespace extensions