[Extension Bindings] Test event listeners throwing exceptions

[Extension Bindings] Test event listeners throwing exceptions

Listeners can throw exceptions (as with any third-party code). Ensure
that we have tests for this. When a listener throws an exception, it
should be logged to the console, but not bubbled up to external try-
catches (so that JS code that may have triggered the event doesn't
receive it). This is the same as we do for web events.

BUG=653596
Review-Url: https://codereview.chromium.org/2613093002
Cr-Commit-Position: refs/heads/master@{#442759}
Committed: https://chromium.googlesource.com/chromium/src/+/44aec8013cedf62952664272b787bbbfd51d4375

[Devlin, 2017-01-05 19:20:29]

rdevlin.cronin@chromium.org changed reviewers:
+ jbroman@chromium.org, lazyboy@chromium.org

[Devlin, 2017-01-05 19:20:30]

Just adding a test

[jbroman, 2017-01-05 19:52:51]

Thanks for the test.

https://codereview.chromium.org/2613093002/diff/1/extensions/renderer/api_eve...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2613093002/diff/1/extensions/renderer/api_eve...
extensions/renderer/api_event_handler_unittest.cc:452: auto
run_js_and_expect_error = [](v8::Local<v8::Function> function,
Isn't it the RunJSFunction that's supposed to be catching exceptions (which this
test one doesn't, but I think the production one does)? Without that, it looks
to me like the exception does escape from FireEventInContext, which I wouldn't
have expected.

[Devlin, 2017-01-05 21:01:46]

https://codereview.chromium.org/2613093002/diff/1/extensions/renderer/api_eve...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2613093002/diff/1/extensions/renderer/api_eve...
extensions/renderer/api_event_handler_unittest.cc:452: auto
run_js_and_expect_error = [](v8::Local<v8::Function> function,
On 2017/01/05 19:52:51, jbroman wrote:
> Isn't it the RunJSFunction that's supposed to be catching exceptions (which
this
> test one doesn't, but I think the production one does)? Without that, it looks
> to me like the exception does escape from FireEventInContext, which I wouldn't
> have expected.

No, RunJSFunction doesn't always catch exceptions (and doesn't in the prod
version).  The test version usually does, but that's really just to catch
unintentional exceptions (since we have RunFunction and
RunFunctionAndExpectError).  The complication is that we can't pass either one
of those in here, since it only sometimes expects an error.  Does that make
sense?

I've also added a basic event test (including dispatching to listeners and
throwing exceptions) to the NativeExtensionBindingsSystem, which uses the prod
run js function.

[jbroman, 2017-01-05 22:22:14]

https://codereview.chromium.org/2613093002/diff/1/extensions/renderer/api_eve...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2613093002/diff/1/extensions/renderer/api_eve...
extensions/renderer/api_event_handler_unittest.cc:452: auto
run_js_and_expect_error = [](v8::Local<v8::Function> function,
On 2017/01/05 at 21:01:46, Devlin (sheriff - maybe slow) wrote:
> On 2017/01/05 19:52:51, jbroman wrote:
> > Isn't it the RunJSFunction that's supposed to be catching exceptions (which
this
> > test one doesn't, but I think the production one does)? Without that, it
looks
> > to me like the exception does escape from FireEventInContext, which I
wouldn't
> > have expected.
> 
> No, RunJSFunction doesn't always catch exceptions (and doesn't in the prod
version).  The test version usually does, but that's really just to catch
unintentional exceptions (since we have RunFunction and
RunFunctionAndExpectError).  The complication is that we can't pass either one
of those in here, since it only sometimes expects an error.  Does that make
sense?
> 
> I've also added a basic event test (including dispatching to listeners and
throwing exceptions) to the NativeExtensionBindingsSystem, which uses the prod
run js function.

Should we be catching exceptions in the event dispatch logic, then? Blink does
catch exceptions during event listeners.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

I _think_ here they'll end up propagating to script, which seems weird.

[Devlin, 2017-01-10 21:23:00]

Description was changed from

==========
[Extension Bindings] Test event listeners throwing exceptions

Listeners can throw exceptions (as with any third-party code). Ensure
that we have tests for this.

BUG=653596
==========

to

==========
[Extension Bindings] Test event listeners throwing exceptions

Listeners can throw exceptions (as with any third-party code). Ensure
that we have tests for this. When a listener throws an exception, it
should be logged to the console, but not bubbled up to external try-
catches (so that JS code that may have triggered the event doesn't
receive it). This is the same as we do for web events.

BUG=653596
==========

[Devlin, 2017-01-10 21:28:07]

https://codereview.chromium.org/2613093002/diff/1/extensions/renderer/api_eve...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2613093002/diff/1/extensions/renderer/api_eve...
extensions/renderer/api_event_handler_unittest.cc:452: auto
run_js_and_expect_error = [](v8::Local<v8::Function> function,
On 2017/01/05 22:22:14, jbroman wrote:
> On 2017/01/05 at 21:01:46, Devlin (sheriff - maybe slow) wrote:
> > On 2017/01/05 19:52:51, jbroman wrote:
> > > Isn't it the RunJSFunction that's supposed to be catching exceptions
(which
> this
> > > test one doesn't, but I think the production one does)? Without that, it
> looks
> > > to me like the exception does escape from FireEventInContext, which I
> wouldn't
> > > have expected.
> > 
> > No, RunJSFunction doesn't always catch exceptions (and doesn't in the prod
> version).  The test version usually does, but that's really just to catch
> unintentional exceptions (since we have RunFunction and
> RunFunctionAndExpectError).  The complication is that we can't pass either one
> of those in here, since it only sometimes expects an error.  Does that make
> sense?
> > 
> > I've also added a basic event test (including dispatching to listeners and
> throwing exceptions) to the NativeExtensionBindingsSystem, which uses the prod
> run js function.
> 
> Should we be catching exceptions in the event dispatch logic, then? Blink does
> catch exceptions during event listeners.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> 
> I _think_ here they'll end up propagating to script, which seems weird.

Fixed (thanks for the help!).  Note that we still need this helper function,
since the test utils (RunFunction et al) expect no exception ever thrown, but I
think the test demonstrates the correct behavior.

[jbroman, 2017-01-10 23:17:22]

lgtm

https://codereview.chromium.org/2613093002/diff/60001/extensions/renderer/api...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2613093002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:518:
static_cast<bool*>(v8::Local<v8::External>::Cast(data)->Value());
super-nit: using Local::As instead of Local::Cast makes this fit on one line.

bool* did_throw = static_cast<bool*>(data.As<v8::External>()->Value());

https://codereview.chromium.org/2613093002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:525:
v8::External::New(isolate(), &did_throw));
nit: Hrm...I guess this is probably fine, but it'd be a little unfortunate if
this had already gone out of scope before something (maybe in the teardown)
triggered a message.

One way would be a scoped object that removed itself as a listener when it went
out of scope.


class ScopedMessageListener {
 public:
  ScopedMessageListener(v8::Isolate* isolate) : isolate_(isolate) {
    isolate->AddMessageListener(&ScopedMessageListener::OnMessage,
v8::External::New(isolate, this));
  }
  ~ScopedMessageListener() {
isolate_->RemoveMessageListeners(&ScopedMessageListener::OnMessage); }
  static void OnMessage(v8::Local<v8::Message> message, v8::Local<v8::Value>
data) {
    ASSERT_FALSE(data.IsEmpty());
    ASSERT_TRUE(data->IsExternal());
    auto* self =
static_cast<ScopedMessageListener*>(data.As<v8::External>()->Value());
    self->message_ = gin::V8ToString(message->Get());
  }
  const std::string& message() { return message_; }
 private:
  v8::Isolate* isolate_;
  std::string message_;
};

ScopedMessageListener listener(isolate);
// do stuff
EXPECT_EQ("Uncaught Error: Event handler error", listener.message());


Since this is just a one-off, ScopedClosureRunner might be fine (lacking one for
lambdas):


isolate()->AddMessageListener(message_listener,
                              v8::External::New(isolate(), &did_throw));
base::ScopedClosureRunner scoped_remove(base::Bind([](v8::Isolate* isolate,
v8::MessageCallback callback) {
  isolate->RemoveMessageListeners(callback);
}, isolate, callback));


Or we could live with the mild risk of unsafety here. Since it's just a unit
test, I don't feel strongly.

[Devlin, 2017-01-10 23:25:45]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[Devlin, 2017-01-10 23:26:01]

lazyboy@, wanna take a look?

https://codereview.chromium.org/2613093002/diff/60001/extensions/renderer/api...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2613093002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:518:
static_cast<bool*>(v8::Local<v8::External>::Cast(data)->Value());
On 2017/01/10 23:17:22, jbroman wrote:
> super-nit: using Local::As instead of Local::Cast makes this fit on one line.
> 
> bool* did_throw = static_cast<bool*>(data.As<v8::External>()->Value());

heh fair enough.  Done.

https://codereview.chromium.org/2613093002/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:525:
v8::External::New(isolate(), &did_throw));
On 2017/01/10 23:17:22, jbroman wrote:
> nit: Hrm...I guess this is probably fine, but it'd be a little unfortunate if
> this had already gone out of scope before something (maybe in the teardown)
> triggered a message.
> 
> One way would be a scoped object that removed itself as a listener when it
went
> out of scope.
> 
> 
> class ScopedMessageListener {
>  public:
>   ScopedMessageListener(v8::Isolate* isolate) : isolate_(isolate) {
>     isolate->AddMessageListener(&ScopedMessageListener::OnMessage,
> v8::External::New(isolate, this));
>   }
>   ~ScopedMessageListener() {
> isolate_->RemoveMessageListeners(&ScopedMessageListener::OnMessage); }
>   static void OnMessage(v8::Local<v8::Message> message, v8::Local<v8::Value>
> data) {
>     ASSERT_FALSE(data.IsEmpty());
>     ASSERT_TRUE(data->IsExternal());
>     auto* self =
> static_cast<ScopedMessageListener*>(data.As<v8::External>()->Value());
>     self->message_ = gin::V8ToString(message->Get());
>   }
>   const std::string& message() { return message_; }
>  private:
>   v8::Isolate* isolate_;
>   std::string message_;
> };
> 
> ScopedMessageListener listener(isolate);
> // do stuff
> EXPECT_EQ("Uncaught Error: Event handler error", listener.message());
> 
> 
> Since this is just a one-off, ScopedClosureRunner might be fine (lacking one
for
> lambdas):
> 
> 
> isolate()->AddMessageListener(message_listener,
>                               v8::External::New(isolate(), &did_throw));
> base::ScopedClosureRunner scoped_remove(base::Bind([](v8::Isolate* isolate,
> v8::MessageCallback callback) {
>   isolate->RemoveMessageListeners(callback);
> }, isolate, callback));
> 
> 
> Or we could live with the mild risk of unsafety here. Since it's just a unit
> test, I don't feel strongly.

Using the ScopedClosureRunner sgtm; done.

[commit-bot: I haz the power, 2017-01-10 23:26:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2017-01-10 23:42:03]

lgtm

[commit-bot: I haz the power, 2017-01-11 00:26:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 00:26:32]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-01-11 01:28:31]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2017-01-11 01:28:32]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2613093002/#ps80001
(title: "Remove listener")

[commit-bot: I haz the power, 2017-01-11 01:29:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 01:34:04]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484098111718380,
"parent_rev": "714ce74842af647ab192124910b08a07ee7d606b", "commit_rev":
"44aec8013cedf62952664272b787bbbfd51d4375"}

[commit-bot: I haz the power, 2017-01-11 01:34:31]

Description was changed from

==========
[Extension Bindings] Test event listeners throwing exceptions

Listeners can throw exceptions (as with any third-party code). Ensure
that we have tests for this. When a listener throws an exception, it
should be logged to the console, but not bubbled up to external try-
catches (so that JS code that may have triggered the event doesn't
receive it). This is the same as we do for web events.

BUG=653596
==========

to

==========
[Extension Bindings] Test event listeners throwing exceptions

Listeners can throw exceptions (as with any third-party code). Ensure
that we have tests for this. When a listener throws an exception, it
should be logged to the console, but not bubbled up to external try-
catches (so that JS code that may have triggered the event doesn't
receive it). This is the same as we do for web events.

BUG=653596

Review-Url: https://codereview.chromium.org/2613093002
Cr-Commit-Position: refs/heads/master@{#442759}
Committed:
https://chromium.googlesource.com/chromium/src/+/44aec8013cedf62952664272b787...
==========

[commit-bot: I haz the power, 2017-01-11 01:34:33]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/44aec8013cedf62952664272b787...