[Extensions Bindings] Add an ExceptionHandler class

[Extensions Bindings] Add an ExceptionHandler class

There are a number of times we need to handle exceptions from untrusted
code running in the bindings system; two of the main ones include when
running a callback to an asynchronous extension function and when
dispatching events to listeners. In both of these cases, we want to log
the exception (so the developer knows something went wrong), but we
don't want to allow the exception to interrupt the flow of running JS.

Introduce an ExceptionHandler class to handle these cases and log the
caught exceptions to either the console (default) or a custom-set
handler (used in testing). Wire it up to the APIRequestHandler (with
more uses coming in later patch sets).

Add unit tests for both the ExceptionHandler and the APIRequestHandler's
use of it.

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

[Devlin, 2017-06-28 18:48:09]

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

[commit-bot: I haz the power, 2017-06-28 18:48:41]

Dry run: 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-06-28 18:52:36]

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

[commit-bot: I haz the power, 2017-06-28 18:52:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2017-06-28 20:21:42]

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

[commit-bot: I haz the power, 2017-06-28 20:22:06]

Dry run: 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-06-28 21:55:43]

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

[commit-bot: I haz the power, 2017-06-28 21:55:46]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-28 22:25:06]

Description was changed from

==========
[exception ahndler]
BUG=
==========

to

==========
[Extensions Bindings] Add an ExceptionHandler class

There are a number of times we need to handle exceptions from untrusted
code running in the bindings system; two of the main ones include when
running a callback to an asynchronous extension function and when
dispatching events to listeners. In both of these cases, we want to log
the exception (so the developer knows something went wrong), but we
don't want to allow the exception to interrupt the flow of running JS.

Introduce an ExceptionHandler class to handle these cases and log the
caught exceptions to either the console (default) or a custom-set
handler (used in testing). Wire it up to the APIRequestHandler (with
more uses coming in later patch sets).

Add unit tests for both the ExceptionHandler and the APIRequestHandler's
use of it.

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

[Devlin, 2017-06-29 01:36:00]

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

[Devlin, 2017-06-29 01:36:01]

Hey folks, mind taking a look?

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
File extensions/renderer/bindings/exception_handler.h (right):

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.h:37: void
SetHandlerForContext(v8::Local<v8::Context> context,
Note: This isn't actually used in this CL (though it's tested), but it will be
used in a followup.  Including the usage here made the CL too large, but
omitting it completely seemed unnecessary.

[jbroman, 2017-06-29 17:45:48]

High-level question: v8::TryCatch::SetVerbose seems to handle the "we want to
log caught exceptions" case, so is the additional value this is providing the
ability to handle them in some other way? If so, what other way do we
anticipate?

[Devlin, 2017-06-29 18:26:13]

On 2017/06/29 17:45:48, jbroman wrote:
> High-level question: v8::TryCatch::SetVerbose seems to handle the "we want to
> log caught exceptions" case, so is the additional value this is providing the
> ability to handle them in some other way? If so, what other way do we
> anticipate?

There's a couple reasons:
- We can provide more context by prepending a message.  E.g., we can say "Error
in handler for chrome.foo.bar: Uncaught error blah".  Sometimes this is
self-evident (such as when you construct the callback as an anonymous function
in the API call), other times it's not (such as when you pass a callback to
multiple events or api calls).
- We can potentially include the stack trace from before the API call.  We do
this right now, though it's a bit buggy.  But it's actually really nice to be
able to see the stack of the call you made to the API in addition to the error
itself.  We don't have that wired up yet, but it's possible.
- Our tests set a custom exception handler. :(

In the general case, I agree that this is very similar to SetVerbose(), and it's
a discussion worth having whether or not it's worth preserving all this
behavior.  But I'd like to start with parity, and make changes from there. 
WDYT?

[jbroman, 2017-06-30 18:35:31]

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
File extensions/renderer/bindings/exception_handler.cc (right):

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:21: const v8::TryCatch&
try_catch) {
Hmm. Minor question here: do we want to reset the TryCatch once we've "handled"
it, to reduce the chance of handling it more than once?

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:39: v8::TryCatch
try_catch(isolate);
Is it desirable to have exceptions here be verbose, or not?

Also, I thought the signature of the RunJSFunction already involved catching the
exceptions (and possibly also running asynchronously). Is that so, or am I
misremembering?

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
File extensions/renderer/bindings/exception_handler.h (right):

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.h:50:
std::vector<std::pair<v8::Global<v8::Context>, v8::Global<v8::Function>>>;
This seems to roughly re-implement PerContextData (stores per-context data,
dumps it on context invalidation). Is there an advantage to this approach?

[Devlin, 2017-07-06 16:49:17]

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

[Devlin, 2017-07-06 16:49:23]

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
File extensions/renderer/bindings/exception_handler.cc (right):

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:21: const v8::TryCatch&
try_catch) {
On 2017/06/30 18:35:31, jbroman wrote:
> Hmm. Minor question here: do we want to reset the TryCatch once we've
"handled"
> it, to reduce the chance of handling it more than once?

Good idea; done.

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:39: v8::TryCatch
try_catch(isolate);
On 2017/06/30 18:35:31, jbroman wrote:
> Is it desirable to have exceptions here be verbose, or not?
Done.

> Also, I thought the signature of the RunJSFunction already involved catching
the
> exceptions (and possibly also running asynchronously). Is that so, or am I
> misremembering?
AFAIK, it doesn't (in production).  RunJSFunction is CallJsFunction() from
native_extension_bindings_system.cc
(https://chromium.googlesource.com/chromium/src/+/5bf60281e3ba01737805ba8b505a...),
which calls ScriptContext::SafeCallFunction().  I don't think there's any
TryCatch-ing there.

The asynchronous aspect is a good and interesting point.  I suppose that if JS
were disabled when we caught an exception to handle it (can that happen?), then
this try catch would go out of scope before the script was executed, and we'd
then throw an error.  A few thoughts here:
- This is only relevant with custom handling, which should only be relevant in
tests.
- Is it possible to have JS disabled immediately after throwing an error?
- What's the worst that could happen?  This doesn't strike me as a security
concern, even if it's a bit weird.

Overall, I'm inclined to say this is okay, but if this is worse than I
understand, lemme know. :)

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
File extensions/renderer/bindings/exception_handler.h (right):

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.h:50:
std::vector<std::pair<v8::Global<v8::Context>, v8::Global<v8::Function>>>;
On 2017/06/30 18:35:31, jbroman wrote:
> This seems to roughly re-implement PerContextData (stores per-context data,
> dumps it on context invalidation). Is there an advantage to this approach?

Brevity :)  It was actually quite a bit shorter than the PerContextData
approach, and we didn't need the advantage of being able to access the
PerContextData by only a context (whereas in other cases we've had, e.g., static
methods that need to access context data, these methods always go directly
through the ExceptionHandler).  But I can understand the desire to use the tools
we have.

I've added a patch that uses PerContextData.  Let me know which you prefer; I'm
fine with either.

[commit-bot: I haz the power, 2017-07-06 16:49:24]

Dry run: 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-07-06 18:17:58]

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

[commit-bot: I haz the power, 2017-07-06 18:17:59]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-07-06 21:09:19]

lgtm

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
File extensions/renderer/bindings/exception_handler.cc (right):

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:39: v8::TryCatch
try_catch(isolate);
On 2017/07/06 at 16:49:22, Devlin wrote:
> On 2017/06/30 18:35:31, jbroman wrote:
> > Is it desirable to have exceptions here be verbose, or not?
> Done.
> 
> > Also, I thought the signature of the RunJSFunction already involved catching
the
> > exceptions (and possibly also running asynchronously). Is that so, or am I
> > misremembering?
> AFAIK, it doesn't (in production).  RunJSFunction is CallJsFunction() from
native_extension_bindings_system.cc
(https://chromium.googlesource.com/chromium/src/+/5bf60281e3ba01737805ba8b505a...),
which calls ScriptContext::SafeCallFunction().  I don't think there's any
TryCatch-ing there.

Ah, you're quite correct; I misremembered.

> The asynchronous aspect is a good and interesting point.  I suppose that if JS
were disabled when we caught an exception to handle it (can that happen?), then
this try catch would go out of scope before the script was executed, and we'd
then throw an error.  A few thoughts here:
> - This is only relevant with custom handling, which should only be relevant in
tests.
> - Is it possible to have JS disabled immediately after throwing an error?
> - What's the worst that could happen?  This doesn't strike me as a security
concern, even if it's a bit weird.
> 
> Overall, I'm inclined to say this is okay, but if this is worse than I
understand, lemme know. :)

Yeah, I think that's fine. I am generally suspect that we do something
reasonable when RequestExecuteV8Function goes async and then throws an
exception, but I suppose that's separate from this.

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
File extensions/renderer/bindings/exception_handler.h (right):

https://codereview.chromium.org/2961103002/diff/40001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.h:50:
std::vector<std::pair<v8::Global<v8::Context>, v8::Global<v8::Function>>>;
On 2017/07/06 at 16:49:22, Devlin wrote:
> On 2017/06/30 18:35:31, jbroman wrote:
> > This seems to roughly re-implement PerContextData (stores per-context data,
> > dumps it on context invalidation). Is there an advantage to this approach?
> 
> Brevity :)  It was actually quite a bit shorter than the PerContextData
approach, and we didn't need the advantage of being able to access the
PerContextData by only a context (whereas in other cases we've had, e.g., static
methods that need to access context data, these methods always go directly
through the ExceptionHandler).  But I can understand the desire to use the tools
we have.
> 
> I've added a patch that uses PerContextData.  Let me know which you prefer;
I'm fine with either.

I think I mildly prefer the PerContextData version because we don't have to
explicitly remember to handle invalidation etc.

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
File extensions/renderer/bindings/exception_handler.cc (right):

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:22:
ExceptionHandlerPerContextData() {}
Is the constructor necessary? I thought it was implied.

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:74: v8::TryCatch
try_catch(isolate);
nit: Give this a separate name, to avoid shadowing the |try_catch| formal
parameter?

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:87:
v8::Global<v8::Function>(context->GetIsolate(), handler);
slightly more concise to say "data->custom_handler.Reset(context->GetIsolate(),
handler);" rather than move-assign, but I don't mind either way.

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:87:
v8::Global<v8::Function>(context->GetIsolate(), handler);
It's kind of implied by the lack of a null check here, but this is only legal to
call on a v8::Context before it is invalidated, right? Is this useful to note at
the declaration?

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:92:
ExceptionHandlerPerContextData* data = GetContextData(context, true);
true -> false

[lazyboy, 2017-07-10 23:12:08]

lgtm

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
File extensions/renderer/bindings/api_request_handler.h (right):

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/api_request_handler.h:126: ExceptionHandler* const
exception_handler_;
Comment about ownership

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
File extensions/renderer/bindings/api_request_handler_unittest.cc (right):

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/api_request_handler_unittest.cc:520: // The outer
TryCatch should not be caught. This is important to not disrupt
Add a bit more context about the try catch you're referring to here, may be call
it |outer_try_catch| or similar. It's not immediately noticeable what the inner
try catch is... so maybe something along the line of "The inner try catch inside
|exception_handler| should be caught, but not the outer try catch".

[Devlin, 2017-07-11 15:33:56]

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

[commit-bot: I haz the power, 2017-07-11 15:34:12]

Dry run: 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-07-11 17:21:04]

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

[commit-bot: I haz the power, 2017-07-11 17:21:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-07-11 17:33:36]

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
File extensions/renderer/bindings/api_request_handler.h (right):

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/api_request_handler.h:126: ExceptionHandler* const
exception_handler_;
On 2017/07/10 23:12:08, lazyboy wrote:
> Comment about ownership

Done.

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
File extensions/renderer/bindings/api_request_handler_unittest.cc (right):

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/api_request_handler_unittest.cc:520: // The outer
TryCatch should not be caught. This is important to not disrupt
On 2017/07/10 23:12:08, lazyboy wrote:
> Add a bit more context about the try catch you're referring to here, may be
call
> it |outer_try_catch| or similar. It's not immediately noticeable what the
inner
> try catch is... so maybe something along the line of "The inner try catch
inside
> |exception_handler| should be caught, but not the outer try catch".

Done.

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
File extensions/renderer/bindings/exception_handler.cc (right):

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:22:
ExceptionHandlerPerContextData() {}
On 2017/07/06 21:09:18, jbroman wrote:
> Is the constructor necessary? I thought it was implied.

Removed.

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:74: v8::TryCatch
try_catch(isolate);
On 2017/07/06 21:09:18, jbroman wrote:
> nit: Give this a separate name, to avoid shadowing the |try_catch| formal
> parameter?

Done.

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:87:
v8::Global<v8::Function>(context->GetIsolate(), handler);
On 2017/07/06 21:09:18, jbroman wrote:
> It's kind of implied by the lack of a null check here, but this is only legal
to
> call on a v8::Context before it is invalidated, right? Is this useful to note
at
> the declaration?

Done.

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:87:
v8::Global<v8::Function>(context->GetIsolate(), handler);
On 2017/07/06 21:09:18, jbroman wrote:
> slightly more concise to say
"data->custom_handler.Reset(context->GetIsolate(),
> handler);" rather than move-assign, but I don't mind either way.

Done.

https://codereview.chromium.org/2961103002/diff/60001/extensions/renderer/bin...
extensions/renderer/bindings/exception_handler.cc:92:
ExceptionHandlerPerContextData* data = GetContextData(context, true);
On 2017/07/06 21:09:18, jbroman wrote:
> true -> false

Whoops, thanks.  Done.

[Devlin, 2017-07-11 17:39:56]

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

[Devlin, 2017-07-11 17:39:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
lazyboy@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2961103002/#ps80001
(title: "rebase + nits")

[commit-bot: I haz the power, 2017-07-11 17:40:15]

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-07-11 19:41:09]

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

[commit-bot: I haz the power, 2017-07-11 19:41:11]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-07-12 00:17:49]

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

[commit-bot: I haz the power, 2017-07-12 00:18:30]

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-07-12 02:30:46]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499818669654740,
"parent_rev": "e8fe6202113dc984079de2d8de7403eed9d7ab4e", "commit_rev":
"006b8a20add259f412ad68cf4cdf28bdcd02b0b6"}

[commit-bot: I haz the power, 2017-07-12 02:31:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499818669654740,
"parent_rev": "096516fbb8c326bca1666f98b07f1ca0ecc8c5aa", "commit_rev":
"e5e2cb9ce72dd22be2685f08cf881d6141cb21b0"}

[commit-bot: I haz the power, 2017-07-12 02:31:20]

Description was changed from

==========
[Extensions Bindings] Add an ExceptionHandler class

There are a number of times we need to handle exceptions from untrusted
code running in the bindings system; two of the main ones include when
running a callback to an asynchronous extension function and when
dispatching events to listeners. In both of these cases, we want to log
the exception (so the developer knows something went wrong), but we
don't want to allow the exception to interrupt the flow of running JS.

Introduce an ExceptionHandler class to handle these cases and log the
caught exceptions to either the console (default) or a custom-set
handler (used in testing). Wire it up to the APIRequestHandler (with
more uses coming in later patch sets).

Add unit tests for both the ExceptionHandler and the APIRequestHandler's
use of it.

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

to

==========
[Extensions Bindings] Add an ExceptionHandler class

There are a number of times we need to handle exceptions from untrusted
code running in the bindings system; two of the main ones include when
running a callback to an asynchronous extension function and when
dispatching events to listeners. In both of these cases, we want to log
the exception (so the developer knows something went wrong), but we
don't want to allow the exception to interrupt the flow of running JS.

Introduce an ExceptionHandler class to handle these cases and log the
caught exceptions to either the console (default) or a custom-set
handler (used in testing). Wire it up to the APIRequestHandler (with
more uses coming in later patch sets).

Add unit tests for both the ExceptionHandler and the APIRequestHandler's
use of it.

BUG=653596

Review-Url: https://codereview.chromium.org/2961103002
Cr-Commit-Position: refs/heads/master@{#485807}
Committed:
https://chromium.googlesource.com/chromium/src/+/e5e2cb9ce72dd22be2685f08cf88...
==========

[commit-bot: I haz the power, 2017-07-12 02:31:22]

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