[Extensions Bindings] Restructure event dispatching during JS suspension

JS can be suspended when the renderer receives the IPC to dispatch an
event. In this case, the JS shouldn't immediately run, but rather
respect the JS state. Currently, the EventEmitter does this by using
JSRunner::RunJSFunction() (which does respect JS suspension) with each
of the listeners for the event.

This mostly works, but has a couple of problems. For one, it makes it
difficult to catch exceptions thrown from a JS listener (since the
listener isn't notified when we queue it up through the JSRunner). It
also doesn't account for the possibility of listeners being removed
between a event comes in, and when it is actually disptached. Finally,
it makes creating a single result for the dispatch difficult, which is
problematic for messaging bindings.

Rework the way event dispatching works to instead use a v8::Function
wrapper around the EventEmitter to trigger the event being dispatched
to listeners. Once that function is triggered, it's safe to use
synchronous JS execution, since JS is already running at that point.
This addresses the concerns with the current approach.

Also add a Suspension utility to the TestJSRunner to simulate JS being
suspended in testing, and add unit tests for dispatching events while
JS is suspended.

Bug: 653596

Change-Id: I7899e1946e8fe750fa2a1f53d62a04e5bd075664
Reviewed-on: https://chromium-review.googlesource.com/792033
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521592}

extensions/renderer/bindings/api_event_handler_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/callback_helpers.h"
 #include "base/memory/ptr_util.h"
 #include "base/optional.h"
+#include "base/run_loop.h"
 #include "base/test/mock_callback.h"
 #include "base/values.h"
 #include "extensions/common/event_filtering_info.h"
@@ -1046,4 +1047,170 @@ TEST_F(APIEventHandlerTest, TestEventsWithoutLazyListeners) {
   DisposeContext(context);
 }
 
+// Tests dispatching events while script is suspended.
+TEST_F(APIEventHandlerTest, TestDispatchingEventsWhileScriptSuspended) {
+  const char kEventName[] = "alpha";
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  v8::Local<v8::Object> event = handler()->CreateEventInstance(
+      kEventName, false, true, binding::kNoListenerMax, true, context);
+
+  const char kListenerFunction[] = "(function() { this.eventFired = true; });";
+  v8::Local<v8::Function> listener =
+      FunctionFromString(context, kListenerFunction);
+
+  {
+    v8::Local<v8::Function> add_listener_function =
+        FunctionFromString(context, kAddListenerFunction);
+    v8::Local<v8::Value> argv[] = {event, listener};
+    RunFunction(add_listener_function, context, arraysize(argv), argv);
+  }
+
+  {
+    // Suspend script and fire an event. The listener should *not* be notified
+    // while script is suspended.
+    TestJSRunner::Suspension script_suspension;
+    handler()->FireEventInContext(kEventName, context, base::ListValue(),
+                                  nullptr);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_EQ("undefined", GetStringPropertyFromObject(context->Global(),
+                                                       context, "eventFired"));
+  }
+
+  // After script resumes, the listener should be notified.
+  EXPECT_EQ("true", GetStringPropertyFromObject(context->Global(), context,
+                                                "eventFired"));
+}
+
+// Tests catching errors thrown by listeners while dispatching after script
+// suspension.
+TEST_F(APIEventHandlerTest,
+       TestListenersThrowingExceptionsAfterScriptSuspension) {
+  auto log_error =
+      [](std::vector<std::string>* errors_out, v8::Local<v8::Context> context,
+         const std::string& error) { errors_out->push_back(error); };
+
+  std::vector<std::string> logged_errors;
+  ExceptionHandler exception_handler(
+      base::BindRepeating(log_error, &logged_errors));
+  SetHandler(std::make_unique<APIEventHandler>(
+      base::BindRepeating(&DoNothingOnEventListenersChanged),
+      &exception_handler));
+
+  const char kEventName[] = "alpha";
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  v8::Local<v8::Object> event = handler()->CreateEventInstance(
+      kEventName, false, true, binding::kNoListenerMax, true, context);
+
+  const char kListenerFunction[] =
+      "(function() {\n"
+      "  this.eventFired = true;\n"
+      "  throw new Error('hahaha');\n"
+      "});";
+  v8::Local<v8::Function> listener =
+      FunctionFromString(context, kListenerFunction);
+
+  {
+    v8::Local<v8::Function> add_listener_function =
+        FunctionFromString(context, kAddListenerFunction);
+    v8::Local<v8::Value> argv[] = {event, listener};
+    RunFunction(add_listener_function, context, arraysize(argv), argv);
+  }
+
+  TestJSRunner::AllowErrors allow_errors;
+  {
+    // Suspend script and fire an event. The listener should not be notified,
+    // and no errors should be logged.
+    TestJSRunner::Suspension script_suspension;
+    handler()->FireEventInContext(kEventName, context, base::ListValue(),
+                                  nullptr);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_EQ("undefined", GetStringPropertyFromObject(context->Global(),
+                                                       context, "eventFired"));
+    EXPECT_TRUE(logged_errors.empty());
+  }
+
+  // Once script resumes, the listener should have been notifed, and we should
+  // have caught the error.
+  EXPECT_EQ("true", GetStringPropertyFromObject(context->Global(), context,
+                                                "eventFired"));
+  ASSERT_EQ(1u, logged_errors.size());
+  EXPECT_THAT(logged_errors[0],
+              testing::StartsWith("Error in event handler: Error: hahaha"));
+}
+
+// Tests dispatching events when listeners are removed between when an event
+// was fired (during script suspension) and when the script runs.
+TEST_F(APIEventHandlerTest,
+       TestDispatchingEventAfterListenersRemovedAfterScriptSuspension) {
+  const char kEventName[] = "alpha";
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  v8::Local<v8::Object> event = handler()->CreateEventInstance(
+      kEventName, false, true, binding::kNoListenerMax, true, context);
+
+  const char kListenerFunction1[] =
+      "(function() { this.eventFired1 = true; });";
+  const char kListenerFunction2[] =
+      "(function() { this.eventFired2 = true; });";
+  v8::Local<v8::Function> listener1 =
+      FunctionFromString(context, kListenerFunction1);
+  v8::Local<v8::Function> listener2 =
+      FunctionFromString(context, kListenerFunction2);
+
+  // Add two event listeners.
+  {
+    v8::Local<v8::Function> add_listener_function =
+        FunctionFromString(context, kAddListenerFunction);
+    {
+      v8::Local<v8::Value> argv[] = {event, listener1};
+      RunFunction(add_listener_function, context, arraysize(argv), argv);
+    }
+    {
+      v8::Local<v8::Value> argv[] = {event, listener2};
+      RunFunction(add_listener_function, context, arraysize(argv), argv);
+    }
+  }
+  EXPECT_EQ(2u, handler()->GetNumEventListenersForTesting(kEventName, context));
+
+  {
+    // Suspend script, and then queue up a call to remove the first listener.
+    TestJSRunner::Suspension script_suspension;
+    v8::Local<v8::Function> remove_listener_function =
+        FunctionFromString(context, kRemoveListenerFunction);
+    {
+      v8::Local<v8::Value> argv[] = {event, listener1};
+      // Note: Use JSRunner() so that script suspension is respected.
+      JSRunner::Get(context)->RunJSFunction(remove_listener_function, context,
+                                            arraysize(argv), argv);
+    }
+
+    // Since script has been suspended, there should still be two listeners, and
+    // neither should have been notified.
+    EXPECT_EQ(2u,
+              handler()->GetNumEventListenersForTesting(kEventName, context));
+    handler()->FireEventInContext(kEventName, context, base::ListValue(),
+                                  nullptr);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_EQ("undefined", GetStringPropertyFromObject(context->Global(),
+                                                       context, "eventFired1"));
+    EXPECT_EQ("undefined", GetStringPropertyFromObject(context->Global(),
+                                                       context, "eventFired2"));
+  }
+
+  // Once script resumes, the first listener should have been removed and the
+  // event should have been fired. Since the listener was removed before the
+  // event dispatch ran in JS, the first listener should *not* have been
+  // notified.
+  EXPECT_EQ(1u, handler()->GetNumEventListenersForTesting(kEventName, context));
+  EXPECT_EQ("undefined", GetStringPropertyFromObject(context->Global(), context,
+                                                     "eventFired1"));
+  EXPECT_EQ("true", GetStringPropertyFromObject(context->Global(), context,
+                                                "eventFired2"));
+}
+
 }  // namespace extensions

extensions/renderer/bindings/event_emitter.h

@@ -5,8 +5,11 @@
 #ifndef EXTENSIONS_RENDERER_BINDINGS_EVENT_EMITTER_H_
 #define EXTENSIONS_RENDERER_BINDINGS_EVENT_EMITTER_H_
 
+#include <map>
 #include <vector>
 
+#include "extensions/common/event_filtering_info.h"
+#include "extensions/renderer/bindings/js_runner.h"
 #include "gin/wrappable.h"
 #include "v8/include/v8.h"
 
@@ -17,7 +20,6 @@ class Arguments;
 namespace extensions {
 class APIEventListeners;
 class ExceptionHandler;
-struct EventFilteringInfo;
 
 // A gin::Wrappable Event object. One is expected to be created per event, per
 // context. Note: this object *does not* clear any events, so it must be
@@ -55,14 +57,17 @@ class EventEmitter final : public gin::Wrappable<EventEmitter> {
   bool HasListeners();
   void Dispatch(gin::Arguments* arguments);
 
-  // Notifies the listeners of an event with the given |args|. If |run_sync| is
-  // true, runs JS synchronously and populates |out_values| with the results of
-  // the listeners.
-  void DispatchImpl(v8::Local<v8::Context> context,
-                    std::vector<v8::Local<v8::Value>>* args,
-                    const EventFilteringInfo* filter,
-                    bool run_sync,
-                    std::vector<v8::Local<v8::Value>>* out_values);
+  // Dispatches an event synchronously to listeners, returning the result.
+  v8::Local<v8::Value> DispatchSync(v8::Local<v8::Context> context,
+                                    std::vector<v8::Local<v8::Value>>* args,
+                                    const EventFilteringInfo* filter);
+
+  // Dispatches an event asynchronously to listeners.
+  void DispatchAsync(v8::Local<v8::Context> context,
+                     std::vector<v8::Local<v8::Value>>* args,
+                     const EventFilteringInfo* filter);
+  static void DispatchAsyncHelper(
+      const v8::FunctionCallbackInfo<v8::Value>& info);
 
   // Whether or not this object is still valid; false upon context release.
   // When invalid, no listeners can be added or removed.
@@ -74,7 +79,15 @@ class EventEmitter final : public gin::Wrappable<EventEmitter> {
   std::unique_ptr<APIEventListeners> listeners_;
 
   // The associated exception handler; guaranteed to outlive this object.
-  ExceptionHandler* const exception_handler_;
+  ExceptionHandler* const exception_handler_ = nullptr;
+
+  // The next id to use in the pending_filters_ map.
+  int next_filter_id_ = 0;
+  // A constant to indicate an invalid id.
+  static constexpr int kInvalidFilterId = -1;
+  // The map of EventFilteringInfos for events that are pending dispatch (since
+  // JS is suspended).
+  std::map<int, EventFilteringInfo> pending_filters_;
 
   DISALLOW_COPY_AND_ASSIGN(EventEmitter);
 };

extensions/renderer/bindings/test_js_runner.cc

@@ -12,9 +12,10 @@ namespace extensions {
 
 namespace {
 
-// NOTE(devlin): This isn't thread-safe. If we have multi-threaded unittests,
-// we'll need to expand this.
+// NOTE(devlin): These aren't thread-safe. If we have multi-threaded unittests,
+// we'll need to expand these.
 bool g_allow_errors = false;
+bool g_suspended = false;
 
 }  // namespace
 
@@ -40,6 +41,24 @@ TestJSRunner::AllowErrors::~AllowErrors() {
   g_allow_errors = false;
 }
 
+TestJSRunner::Suspension::Suspension() {
+  DCHECK(!g_suspended) << "Nested Suspension() blocks are not allowed.";
+  g_suspended = true;
+}
+
+TestJSRunner::Suspension::~Suspension() {
+  DCHECK(g_suspended);
+  g_suspended = false;
+  TestJSRunner* test_runner =
+      static_cast<TestJSRunner*>(JSRunner::GetInstanceForTesting());
+  DCHECK(test_runner);
+  test_runner->Flush();
+}
+
+TestJSRunner::PendingCall::PendingCall() {}
+TestJSRunner::PendingCall::~PendingCall() = default;
+TestJSRunner::PendingCall::PendingCall(PendingCall&& other) = default;
+
 TestJSRunner::TestJSRunner() {}
 TestJSRunner::TestJSRunner(const base::Closure& will_call_js)
     : will_call_js_(will_call_js) {}
@@ -49,6 +68,20 @@ void TestJSRunner::RunJSFunction(v8::Local<v8::Function> function,
                                  v8::Local<v8::Context> context,
                                  int argc,
                                  v8::Local<v8::Value> argv[]) {
+  if (g_suspended) {
+    // Script is suspended. Queue up the call and return.
+    v8::Isolate* isolate = context->GetIsolate();
+    PendingCall call;
+    call.isolate = isolate;
+    call.function.Reset(isolate, function);
+    call.context.Reset(isolate, context);
+    call.arguments.reserve(argc);
+    for (int i = 0; i < argc; ++i)
+      call.arguments.push_back(v8::Global<v8::Value>(isolate, argv[i]));
+    pending_calls_.push_back(std::move(call));
+    return;
+  }
+
   if (will_call_js_)
     will_call_js_.Run();
 
@@ -63,6 +96,8 @@ v8::MaybeLocal<v8::Value> TestJSRunner::RunJSFunctionSync(
     v8::Local<v8::Context> context,
     int argc,
     v8::Local<v8::Value> argv[]) {
+  // Note: deliberately circumvent g_suspension, since this should only be used
+  // in response to JS interaction.
   if (will_call_js_)
     will_call_js_.Run();
 
@@ -74,4 +109,21 @@ v8::MaybeLocal<v8::Value> TestJSRunner::RunJSFunctionSync(
   return RunFunctionOnGlobal(function, context, argc, argv);
 }
 
+void TestJSRunner::Flush() {
+  // Move pending_calls_ in case running any pending calls results in more calls
+  // into the JSRunner.
+  std::vector<PendingCall> calls = std::move(pending_calls_);
+  pending_calls_.clear();
+  for (auto& call : calls) {
+    v8::Isolate* isolate = call.isolate;
+    v8::Local<v8::Context> context = call.context.Get(isolate);
+    std::vector<v8::Local<v8::Value>> local_arguments;
+    local_arguments.reserve(call.arguments.size());
+    for (auto& arg : call.arguments)
+      local_arguments.push_back(arg.Get(isolate));
+    RunJSFunctionSync(call.function.Get(isolate), context,
+                      local_arguments.size(), local_arguments.data());
+  }
+}
+
 }  // namespace extensions

extensions/renderer/bindings/test_js_runner.h

@@ -55,6 +55,20 @@ class TestJSRunner : public JSRunner {
     DISALLOW_COPY_AND_ASSIGN(AllowErrors);
   };
 
+  // A scoped object that suspends script execution through the JSRunner. While
+  // in scope, function calls to JSRunner will be queued up, and finally run
+  // upon Suspension destruction.
+  // Note: *not* allowed to be nested; only one can be active at a time.
+  // Constructing multiple instances at the same time will DCHECK.
+  class Suspension {
+   public:
+    Suspension();
+    ~Suspension();
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Suspension);
+  };
+
   TestJSRunner();
   // Provides a callback to be called just before JS will be executed.
   explicit TestJSRunner(const base::Closure& will_call_js);
@@ -72,7 +86,24 @@ class TestJSRunner : public JSRunner {
       v8::Local<v8::Value> argv[]) override;
 
  private:
+  friend class Suspension;
+
+  struct PendingCall {
+    PendingCall();
+    ~PendingCall();
+    PendingCall(PendingCall&& other);
+
+    v8::Isolate* isolate;
+    v8::Global<v8::Function> function;
+    v8::Global<v8::Context> context;
+    std::vector<v8::Global<v8::Value>> arguments;
+  };
+
+  // Runs all pending calls.
+  void Flush();
+
   base::Closure will_call_js_;
+  std::vector<PendingCall> pending_calls_;
 
   DISALLOW_COPY_AND_ASSIGN(TestJSRunner);
 };