[Extensions Bindings] Allow event listener unregistration in handling

extensions/renderer/api_event_handler_unittest.cc

Index: extensions/renderer/api_event_handler_unittest.cc
diff --git a/extensions/renderer/api_event_handler_unittest.cc b/extensions/renderer/api_event_handler_unittest.cc
index ce1993779b74cf469e82d8be4e1bce1cd76ee3c5..65635b5baf760fa8686eebab894547ea14f26116 100644
--- a/extensions/renderer/api_event_handler_unittest.cc
+++ b/extensions/renderer/api_event_handler_unittest.cc
@@ -447,4 +447,63 @@ TEST_F(APIEventHandlerTest, TestDispatchFromJs) {
                 context->Global(), context, "eventArgs"));
 }
 
+// Test listeners that remove themselves in their handling of the event.
+TEST_F(APIEventHandlerTest, RemovingListenersWhileHandlingEvent) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = ContextLocal();
+
+  APIEventHandler handler(base::Bind(&RunFunctionOnGlobalAndIgnoreResult));
+  const char kEventName[] = "alpha";
+  v8::Local<v8::Object> event =
+      handler.CreateEventInstance(kEventName, context);
+  ASSERT_FALSE(event.IsEmpty());
+
+  {
+    // Cache the event object on the global in order to allow for easy removal.
+    v8::Local<v8::Function> set_event_on_global =
+        FunctionFromString(
+            context,
+           "(function(event) { this.testEvent = event; })");
+    v8::Local<v8::Value> args[] = {event};
+    RunFunctionOnGlobal(set_event_on_global, context, arraysize(args), args);
+    EXPECT_EQ(event,
+              GetPropertyFromObject(context->Global(), context, "testEvent"));
+  }
+
+  // A listener function that removes itself as a listener.
+  const char kListenerFunction[] =
+      "(function listener%d() {\n"

[jbroman, 2017/01/02 20:30:32]

nit: You could also do this to avoid the neat to printf, just using an anonymous
function to separate out the variable names. Since this is a test, I don't feel
strongly.

(function() {
  return function listener() {
    this.testEvent.removeListener(listener);
  }
})();

[Devlin, 2017/01/04 18:14:54]

I like it, done.

+      "  this.testEvent.removeListener(listener%d);\n"
+      "});";
+
+  // Create and add a bunch of listeners.
+  std::vector<v8::Local<v8::Function>> listeners;
+  const int kNumListeners = 20;
+  listeners.reserve(kNumListeners);
+  for (int i = 0; i < kNumListeners; ++i) {
+    listeners.push_back(FunctionFromString(
+        context,
+        base::StringPrintf(kListenerFunction, i, i)));
+  }
+
+  const char kAddListenerFunction[] =
+      "(function(event, listener) { event.addListener(listener); })";
+  v8::Local<v8::Function> add_listener_function =
+      FunctionFromString(context, kAddListenerFunction);
+
+  for (const auto& listener : listeners) {
+    v8::Local<v8::Value> argv[] = {event, listener};
+    RunFunctionOnGlobal(add_listener_function, context, arraysize(argv), argv);
+  }
+
+  // Fire the event. All listeners should be removed (and we shouldn't crash).
+  EXPECT_EQ(20u, handler.GetNumEventListenersForTesting(kEventName, context));
+  handler.FireEventInContext(kEventName, context, base::ListValue());
+  EXPECT_EQ(0u, handler.GetNumEventListenersForTesting(kEventName, context));
+
+  // TODO(devlin): Another possible test: register listener a and listener b,
+  // where a removes b and b removes a. Theoretically, only one should be
+  // notified. Investigate what we currently do in JS-style bindings.
+}
+
 }  // namespace extensions