[Extenisons Bindings] Don't throw unchecked errors; add console errors

[Extenisons Bindings] Don't throw unchecked errors; add console errors

Throwing an unchecked runtime.lastError results in an uncaught
exception, which can prevent future JS from properly running in
somewhat unpredictable ways. These errors should be logged as console
errors, rather than being thrown as exceptions.

Add tests to a) check errors being logged and b) check that chaining
API calls and callbacks works even when there are uncheked last errors.

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

[Devlin, 2017-04-13 22:35:33]

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

[commit-bot: I haz the power, 2017-04-13 22:36:14]

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-04-13 23:27:37]

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

[commit-bot: I haz the power, 2017-04-13 23:27:38]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-04-14 00:43:16]

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

[Devlin, 2017-04-14 00:43:17]

Hey folks, mind taking a look?

jbroman@, this was the root cause of the weirdness we were talking about the
other day.

https://codereview.chromium.org/2819683002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/native_bindings_apitest.cc (right):

https://codereview.chromium.org/2819683002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/native_bindings_apitest.cc:223: chrome.tabs.query({},
function(tabs) {
Interestingly, this level of nesting is necessary to cause the weirdness with
exceptions.  I would have expected that after the call to execute script, one
additional level would be sufficient (the exception is thrown after the callback
to executeScript finishes), but that alone isn't enough.  From experimentation
in some of the browser tests that failed because of this, I also sometimes saw a
limited number of js calls executing before it broke - but I'm not sure what the
deterministic way of telling how many calls are allowed would be.

I'm probably content to leave this as-is, but let me know if y'all have any
ideas.

[jbroman, 2017-04-19 17:45:05]

lgtm

https://codereview.chromium.org/2819683002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/native_bindings_apitest.cc (right):

https://codereview.chromium.org/2819683002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/native_bindings_apitest.cc:234: const Extension*
extension = nullptr;
nit: Since there's nothing below the below block, this could probably just be:

  ExtensionTestMessageListener listener("callback", false);
  ASSERT_TRUE(LoadExtension(test_dir.UnpackedPath());
  EXPECT_TRUE(listener.WaitUntilSatisfied());

But I don't feel strongly at all.

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/api...
File extensions/renderer/api_last_error.cc (right):

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:137: add_console_error_.Run(
Heh, I'd missed that the original code used console.error rather than throwing
an exception all along. Sorry.

The other way, I suppose, would be to wrap it in a verbose TryCatch and let
whatever handler is registered on the isolate handle that. I think I like the
current approach better. (I'm not entirely sure whether you get a stack trace
this way, though.)

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/con...
File extensions/renderer/console.cc (right):

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/con...
extensions/renderer/console.cc:76: << "\": no render frame found";
nit: no script context found?

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/scr...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/scr...
extensions/renderer/script_context.cc:439: void
ScriptContext::AddMessageToConsole(content::ConsoleMessageLevel level,
Why have both this and extensions::console::AddMessage (even if one calls the
other)? Why not either rewrite all calls to the latter to this one, or use
extensions::console::AddMessage in NativeExtensionBindingsSystem?

[lazyboy, 2017-04-19 18:45:40]

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/api...
File extensions/renderer/api_last_error.cc (right):

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:137: add_console_error_.Run(
I might be missing something, but
add_console_error_.is_null() == true for non-test code, right? Shouldn't we be
checking for that before calling Run()?
I've also noticed the same is true for get_parent_.

[Devlin, 2017-04-19 19:43:37]

Patchset #3 (id:40001) has been deleted

[Devlin, 2017-04-19 19:45:33]

https://codereview.chromium.org/2819683002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/native_bindings_apitest.cc (right):

https://codereview.chromium.org/2819683002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/native_bindings_apitest.cc:234: const Extension*
extension = nullptr;
On 2017/04/19 17:45:04, jbroman wrote:
> nit: Since there's nothing below the below block, this could probably just be:
> 
>   ExtensionTestMessageListener listener("callback", false);
>   ASSERT_TRUE(LoadExtension(test_dir.UnpackedPath());
>   EXPECT_TRUE(listener.WaitUntilSatisfied());
> 
> But I don't feel strongly at all.

Whoops, copy-paste leftovers.  Cleaned.

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/api...
File extensions/renderer/api_last_error.cc (right):

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:137: add_console_error_.Run(
On 2017/04/19 17:45:04, jbroman wrote:
> Heh, I'd missed that the original code used console.error rather than throwing
> an exception all along. Sorry.

Clearly, I did too. :)

> The other way, I suppose, would be to wrap it in a verbose TryCatch and let
> whatever handler is registered on the isolate handle that. I think I like the
> current approach better. (I'm not entirely sure whether you get a stack trace
> this way, though.)

The way the current code handles stack traces is by recording a stack trace at
the start of the request and currying it along, and then passing it to
lastError.reportIfUnchecked.  I've been torn on whether it's worth that currying
for the benefit it provides (which is also that if there's an error in a
callback, we can use that same trace).  In either case, I think it's best
handled in a follow-up, since it's a more far-reaching change.

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:137: add_console_error_.Run(
On 2017/04/19 18:45:40, lazyboy wrote:
> I might be missing something, but
> add_console_error_.is_null() == true for non-test code, right? Shouldn't we be
> checking for that before calling Run()?
> I've also noticed the same is true for get_parent_.

I don't think it should ever be null for non-test code (the only place we
instantiate it is through NativeExtensionBindingsSystem, passed to
APIBindingsSystem).  So I think unconditionally calling it (and requiring tests
that trigger it to provide something) is reasonable.

Am I missing something?

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/con...
File extensions/renderer/console.cc (right):

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/con...
extensions/renderer/console.cc:76: << "\": no render frame found";
On 2017/04/19 17:45:04, jbroman wrote:
> nit: no script context found?

Done.

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/scr...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/scr...
extensions/renderer/script_context.cc:439: void
ScriptContext::AddMessageToConsole(content::ConsoleMessageLevel level,
On 2017/04/19 17:45:05, jbroman wrote:
> Why have both this and extensions::console::AddMessage (even if one calls the
> other)? Why not either rewrite all calls to the latter to this one, or use
> extensions::console::AddMessage in NativeExtensionBindingsSystem?

Good question.  Removed.

[Devlin, 2017-04-19 19:45:39]

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

[commit-bot: I haz the power, 2017-04-19 19:46:27]

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

[lazyboy, 2017-04-19 20:28:07]

lgtm

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/api...
File extensions/renderer/api_last_error.cc (right):

https://codereview.chromium.org/2819683002/diff/20001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:137: add_console_error_.Run(
On 2017/04/19 19:45:33, Devlin wrote:
> On 2017/04/19 18:45:40, lazyboy wrote:
> > I might be missing something, but
> > add_console_error_.is_null() == true for non-test code, right? Shouldn't we
be
> > checking for that before calling Run()?
> > I've also noticed the same is true for get_parent_.
> 
> I don't think it should ever be null for non-test code (the only place we
> instantiate it is through NativeExtensionBindingsSystem, passed to
> APIBindingsSystem).  So I think unconditionally calling it (and requiring
tests
> that trigger it to provide something) is reasonable.
> 
> Am I missing something?

Nope. Looking again, I think I misread api_binding_unittest.cc.

[commit-bot: I haz the power, 2017-04-19 20:32:45]

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

[commit-bot: I haz the power, 2017-04-19 20:32:46]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-04-19 20:41:54]

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

[Devlin, 2017-04-19 20:41:54]

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/2819683002/#ps60001
(title: "jbroman's")

[commit-bot: I haz the power, 2017-04-19 20:42:34]

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-04-19 20:49:33]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492634514038210,
"parent_rev": "69e2570d4b5e50cd708e4c467e567d810c4b2134", "commit_rev":
"baa379d2816ac03540dc343fa20d6f0ba9791475"}

[commit-bot: I haz the power, 2017-04-19 20:49:42]

Description was changed from

==========
[Extenisons Bindings] Don't throw unchecked errors; add console errors

Throwing an unchecked runtime.lastError results in an uncaught
exception, which can prevent future JS from properly running in
somewhat unpredictable ways. These errors should be logged as console
errors, rather than being thrown as exceptions.

Add tests to a) check errors being logged and b) check that chaining
API calls and callbacks works even when there are uncheked last errors.

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

to

==========
[Extenisons Bindings] Don't throw unchecked errors; add console errors

Throwing an unchecked runtime.lastError results in an uncaught
exception, which can prevent future JS from properly running in
somewhat unpredictable ways. These errors should be logged as console
errors, rather than being thrown as exceptions.

Add tests to a) check errors being logged and b) check that chaining
API calls and callbacks works even when there are uncheked last errors.

BUG=653596

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

[commit-bot: I haz the power, 2017-04-19 20:49:43]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/baa379d2816ac03540dc343fa20d...