[Extensions Bindings] Add chrome.runtime.lastError support

[Extensions Bindings] Add chrome.runtime.lastError support

If an API function returns an error, we surface it through
chrome.runtime.lastError. Add support for this with native bindings.

Note: we also currently set the last error on the (deprecated)
chrome.extension.lastError. This CL does not address that.

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

[Devlin, 2017-01-27 18:07:35]

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

[commit-bot: I haz the power, 2017-01-27 18:07:58]

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-01-27 18:20:08]

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

[commit-bot: I haz the power, 2017-01-27 18:20:09]

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

[Devlin, 2017-01-27 21:26:38]

Description was changed from

==========
lasterrorstart

BUG=
==========

to

==========
[Extensions Bindings] Add chrome.runtime.lastError support

If an API function returns an error, we surface it through
chrome.runtime.lastError. Add support for this with native bindings.

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

[Devlin, 2017-01-27 21:27:18]

Description was changed from

==========
[Extensions Bindings] Add chrome.runtime.lastError support

If an API function returns an error, we surface it through
chrome.runtime.lastError. Add support for this with native bindings.

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

to

==========
[Extensions Bindings] Add chrome.runtime.lastError support

If an API function returns an error, we surface it through
chrome.runtime.lastError. Add support for this with native bindings.

Note: we also currently set the last error on the (deprecated)
chrome.extension.lastError. This CL does not address that.

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

[Devlin, 2017-01-27 21:33:59]

Patchset #1 (id:1) has been deleted

[Devlin, 2017-01-27 21:34:06]

Patchset #1 (id:20001) has been deleted

[Devlin, 2017-01-27 21:34:11]

Patchset #1 (id:40001) has been deleted

[Devlin, 2017-01-27 21:34:16]

Patchset #1 (id:60001) has been deleted

[Devlin, 2017-01-27 21:41:48]

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

[Devlin, 2017-01-27 21:41:48]

Take a look when you have a chance.  jbroman@, see questions in unittest.cc :)

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
File extensions/renderer/api_last_error_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
extensions/renderer/api_last_error_unittest.cc:103: EXPECT_TRUE(did_throw);
Strangely, this test fails.  I've verified that we throw the exception in
api_last_error.cc as expected, but it doesn't reach the message listener.  Any
idea why?  Should we be throwing in a different way than
Isolate::ThrowException()?

Also interesting, this test fails in the tear down where we verify that the
contexts get properly cleaned up, and it seems the culprit is the call to
ThrowException() (if I comment it out, the context is cleaned up properly).  Any
ideas?

[jbroman, 2017-01-30 22:49:52]

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
File extensions/renderer/api_last_error_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
extensions/renderer/api_last_error_unittest.cc:103: EXPECT_TRUE(did_throw);
On 2017/01/27 at 21:41:48, Devlin wrote:
> Strangely, this test fails.  I've verified that we throw the exception in
api_last_error.cc as expected, but it doesn't reach the message listener.  Any
idea why?  Should we be throwing in a different way than
Isolate::ThrowException()?
> 
> Also interesting, this test fails in the tear down where we verify that the
contexts get properly cleaned up, and it seems the culprit is the call to
ThrowException() (if I comment it out, the context is cleaned up properly).  Any
ideas?

This is a subtlety about how throwing exceptions works. The public method
actually only makes it the "scheduled" exception, which is thrown when the V8
runtime is entered. As a result, the exception is getting thrown in a subsequent
test, when the isolate next checks for a scheduled exception to promote.

I think in this case, it's probably simplest to check for the error by using a
TryCatch and checking HasCaught (you can also get the message from the TryCatch,
if you want to examine it). Nothing strange is going on here that you should
need to use a global message listener (unlike the other case, where you were
checking what happened if an exception was unhandled in an event listener).

[jbroman, 2017-01-30 23:08:34]

Will have a closer look, but high level: it's unfortunate that this is a
property that appears and disappears; usually this sort of thing would be a
property that had the value 'null' or 'undefined' when there's no other value.
(Then this could be an accessor, and we wouldn't need the logic to add and
delete the property.) I'm assuming that we want to save this behavior (even
though it will mean repeatedly changing the shape of this object)?

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

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:120: parent->Delete(context, key);
Warning: v8::Object::Delete can throw an exception. I'm not sure whether one can
actually happen here (a lot them can only happen in strict mode), but we should
be looking at the return value of Delete.

[Devlin, 2017-01-31 16:20:39]

(Just responding to the high-level comment; will address others soon)
On 2017/01/30 23:08:34, jbroman wrote:
> Will have a closer look, but high level: it's unfortunate that this is a
> property that appears and disappears; usually this sort of thing would be a
> property that had the value 'null' or 'undefined' when there's no other value.
> (Then this could be an accessor, and we wouldn't need the logic to add and
> delete the property.) I'm assuming that we want to save this behavior (even
> though it will mean repeatedly changing the shape of this object)?

Yeah, it is a little unfortunate, but it's how our current system works, and, as
you guessed, we probably don't want to change that behavior at this point.  Most
extensions will probably check last error with something like

if (chrome.runtime.lastError)
(which is how we usually have it in examples), but I wouldn't be surprised if
some did
if ('lastError' in chrome.runtime)
or something similar to that, and those cases would fail with lastError simply
set to null/undefined.

Going forward, this would be an API worth changing (though maybe we just move to
promises), but for now, one of the main goals is full backwards compatibility.

On the upside, this only happens when the extension makes an API call that fails
- which should be relatively infrequently, and, in many cases, won't be during
the critical path (since most API errors are the result of an async method
implemented in the browser process).  So I'm hoping that, while less efficient,
it shouldn't make a huge difference (and if an extension is repeatedly causing
errors, there's already something wrong with it ;)).

[Devlin, 2017-01-31 17:15:00]

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

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:120: parent->Delete(context, key);
On 2017/01/30 23:08:34, jbroman wrote:
> Warning: v8::Object::Delete can throw an exception. I'm not sure whether one
can
> actually happen here (a lot them can only happen in strict mode), but we
should
> be looking at the return value of Delete.

What should we do with the result?  Similar to line 92, it may throw, but if it
does, is there any different behavior we want?  Or can we assume that this
should never throw, and CHECK the result?

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
File extensions/renderer/api_last_error_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
extensions/renderer/api_last_error_unittest.cc:103: EXPECT_TRUE(did_throw);
On 2017/01/30 22:49:52, jbroman wrote:
> On 2017/01/27 at 21:41:48, Devlin wrote:
> > Strangely, this test fails.  I've verified that we throw the exception in
> api_last_error.cc as expected, but it doesn't reach the message listener.  Any
> idea why?  Should we be throwing in a different way than
> Isolate::ThrowException()?
> > 
> > Also interesting, this test fails in the tear down where we verify that the
> contexts get properly cleaned up, and it seems the culprit is the call to
> ThrowException() (if I comment it out, the context is cleaned up properly). 
Any
> ideas?
> 
> This is a subtlety about how throwing exceptions works. The public method
> actually only makes it the "scheduled" exception, which is thrown when the V8
> runtime is entered. As a result, the exception is getting thrown in a
subsequent
> test, when the isolate next checks for a scheduled exception to promote.
> 
> I think in this case, it's probably simplest to check for the error by using a
> TryCatch and checking HasCaught (you can also get the message from the
TryCatch,
> if you want to examine it). Nothing strange is going on here that you should
> need to use a global message listener (unlike the other case, where you were
> checking what happened if an exception was unhandled in an event listener).

Using the TryCatch here worked, thanks!  It's still crashing when trying to
dispose the Isolate, though... any ideas on that front?

[jbroman, 2017-02-06 18:31:27]

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

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:120: parent->Delete(context, key);
On 2017/01/31 at 17:15:00, Devlin wrote:
> On 2017/01/30 23:08:34, jbroman wrote:
> > Warning: v8::Object::Delete can throw an exception. I'm not sure whether one
can
> > actually happen here (a lot them can only happen in strict mode), but we
should
> > be looking at the return value of Delete.
> 
> What should we do with the result?  Similar to line 92, it may throw, but if
it does, is there any different behavior we want?  Or can we assume that this
should never throw, and CHECK the result?

I'm not sure. We could:
- let it be/rethrow (but we should probably signal to the caller that it should
abort, rather than continuing as though nothing has happened)
- CHECK (though since we believe this not to be impossible, that's arguably
outside what CHECK is for)
- catch and suppress it

At the moment, though the exception seems to just sit on the isolate, and I'm
not confident it will necessarily be thrown to script at a place where we expect
it.

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
File extensions/renderer/api_last_error_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
extensions/renderer/api_last_error_unittest.cc:103: EXPECT_TRUE(did_throw);
On 2017/01/31 at 17:15:00, Devlin wrote:
> On 2017/01/30 22:49:52, jbroman wrote:
> > On 2017/01/27 at 21:41:48, Devlin wrote:
> > > Strangely, this test fails.  I've verified that we throw the exception in
> > api_last_error.cc as expected, but it doesn't reach the message listener. 
Any
> > idea why?  Should we be throwing in a different way than
> > Isolate::ThrowException()?
> > > 
> > > Also interesting, this test fails in the tear down where we verify that
the
> > contexts get properly cleaned up, and it seems the culprit is the call to
> > ThrowException() (if I comment it out, the context is cleaned up properly). 
Any
> > ideas?
> > 
> > This is a subtlety about how throwing exceptions works. The public method
> > actually only makes it the "scheduled" exception, which is thrown when the
V8
> > runtime is entered. As a result, the exception is getting thrown in a
subsequent
> > test, when the isolate next checks for a scheduled exception to promote.
> > 
> > I think in this case, it's probably simplest to check for the error by using
a
> > TryCatch and checking HasCaught (you can also get the message from the
TryCatch,
> > if you want to examine it). Nothing strange is going on here that you should
> > need to use a global message listener (unlike the other case, where you were
> > checking what happened if an exception was unhandled in an event listener).
> 
> Using the TryCatch here worked, thanks!  It's still crashing when trying to
dispose the Isolate, though... any ideas on that front?

That's a red herring (due to a bug in APIBindingTest, that I'll send you a CL
for shortly). The actual problem is that the check that the context can be
collected failed -- which suggests that there's a memory leak around somewhere.

https://codereview.chromium.org/2657613005/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_last_error_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/100001/extensions/renderer/ap...
extensions/renderer/api_last_error_unittest.cc:75: v8::TryCatch
try_catch(isolate());
nit: whichever you prefer, but you could reuse the same TryCatch and just Reset
it between the two; then you wouldn't need the block nesting

[Devlin, 2017-02-06 20:28:22]

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

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
extensions/renderer/api_last_error.cc:120: parent->Delete(context, key);
On 2017/02/06 18:31:26, jbroman wrote:
> On 2017/01/31 at 17:15:00, Devlin wrote:
> > On 2017/01/30 23:08:34, jbroman wrote:
> > > Warning: v8::Object::Delete can throw an exception. I'm not sure whether
one
> can
> > > actually happen here (a lot them can only happen in strict mode), but we
> should
> > > be looking at the return value of Delete.
> > 
> > What should we do with the result?  Similar to line 92, it may throw, but if
> it does, is there any different behavior we want?  Or can we assume that this
> should never throw, and CHECK the result?
> 
> I'm not sure. We could:
> - let it be/rethrow (but we should probably signal to the caller that it
should
> abort, rather than continuing as though nothing has happened)
> - CHECK (though since we believe this not to be impossible, that's arguably
> outside what CHECK is for)
> - catch and suppress it
> 
> At the moment, though the exception seems to just sit on the isolate, and I'm
> not confident it will necessarily be thrown to script at a place where we
expect
> it.

As discussed offline, we'll catch the error (verbosely), so that callers don't
have to worry about it, but we'll still surface data to the extension if
anything funky happens.

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
File extensions/renderer/api_last_error_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
extensions/renderer/api_last_error_unittest.cc:103: EXPECT_TRUE(did_throw);
On 2017/02/06 18:31:26, jbroman wrote:
> On 2017/01/31 at 17:15:00, Devlin wrote:
> > On 2017/01/30 22:49:52, jbroman wrote:
> > > On 2017/01/27 at 21:41:48, Devlin wrote:
> > > > Strangely, this test fails.  I've verified that we throw the exception
in
> > > api_last_error.cc as expected, but it doesn't reach the message listener. 
> Any
> > > idea why?  Should we be throwing in a different way than
> > > Isolate::ThrowException()?
> > > > 
> > > > Also interesting, this test fails in the tear down where we verify that
> the
> > > contexts get properly cleaned up, and it seems the culprit is the call to
> > > ThrowException() (if I comment it out, the context is cleaned up
properly). 
> Any
> > > ideas?
> > > 
> > > This is a subtlety about how throwing exceptions works. The public method
> > > actually only makes it the "scheduled" exception, which is thrown when the
> V8
> > > runtime is entered. As a result, the exception is getting thrown in a
> subsequent
> > > test, when the isolate next checks for a scheduled exception to promote.
> > > 
> > > I think in this case, it's probably simplest to check for the error by
using
> a
> > > TryCatch and checking HasCaught (you can also get the message from the
> TryCatch,
> > > if you want to examine it). Nothing strange is going on here that you
should
> > > need to use a global message listener (unlike the other case, where you
were
> > > checking what happened if an exception was unhandled in an event
listener).
> > 
> > Using the TryCatch here worked, thanks!  It's still crashing when trying to
> dispose the Isolate, though... any ideas on that front?
> 
> That's a red herring (due to a bug in APIBindingTest, that I'll send you a CL
> for shortly). The actual problem is that the check that the context can be
> collected failed -- which suggests that there's a memory leak around
somewhere.

As discussed offline, the real question here is what the leak is... it appears
to only happen when we have the ThrowException() call in ClearError() - without
that, the context is properly destroyed.  But nothing appears to be hanging on
to the error directly, so it seems like something in v8's machinery.

https://codereview.chromium.org/2657613005/diff/100001/extensions/renderer/ap...
File extensions/renderer/api_last_error_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/100001/extensions/renderer/ap...
extensions/renderer/api_last_error_unittest.cc:75: v8::TryCatch
try_catch(isolate());
On 2017/02/06 18:31:26, jbroman wrote:
> nit: whichever you prefer, but you could reuse the same TryCatch and just
Reset
> it between the two; then you wouldn't need the block nesting

I actually slightly prefer this - it makes it easier to separate out the test
cases visually.  If you have a strong preference the other way, I can combine
them, but otherwise, I'd say we can leave it.

[jbroman, 2017-02-07 18:19:42]

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
File extensions/renderer/api_last_error_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
extensions/renderer/api_last_error_unittest.cc:103: EXPECT_TRUE(did_throw);
On 2017/02/06 at 20:28:22, Devlin wrote:
> On 2017/02/06 18:31:26, jbroman wrote:
> > On 2017/01/31 at 17:15:00, Devlin wrote:
> > > On 2017/01/30 22:49:52, jbroman wrote:
> > > > On 2017/01/27 at 21:41:48, Devlin wrote:
> > > > > Strangely, this test fails.  I've verified that we throw the exception
in
> > > > api_last_error.cc as expected, but it doesn't reach the message
listener. 
> > Any
> > > > idea why?  Should we be throwing in a different way than
> > > > Isolate::ThrowException()?
> > > > > 
> > > > > Also interesting, this test fails in the tear down where we verify
that
> > the
> > > > contexts get properly cleaned up, and it seems the culprit is the call
to
> > > > ThrowException() (if I comment it out, the context is cleaned up
properly). 
> > Any
> > > > ideas?
> > > > 
> > > > This is a subtlety about how throwing exceptions works. The public
method
> > > > actually only makes it the "scheduled" exception, which is thrown when
the
> > V8
> > > > runtime is entered. As a result, the exception is getting thrown in a
> > subsequent
> > > > test, when the isolate next checks for a scheduled exception to promote.
> > > > 
> > > > I think in this case, it's probably simplest to check for the error by
using
> > a
> > > > TryCatch and checking HasCaught (you can also get the message from the
> > TryCatch,
> > > > if you want to examine it). Nothing strange is going on here that you
should
> > > > need to use a global message listener (unlike the other case, where you
were
> > > > checking what happened if an exception was unhandled in an event
listener).
> > > 
> > > Using the TryCatch here worked, thanks!  It's still crashing when trying
to
> > dispose the Isolate, though... any ideas on that front?
> > 
> > That's a red herring (due to a bug in APIBindingTest, that I'll send you a
CL
> > for shortly). The actual problem is that the check that the context can be
> > collected failed -- which suggests that there's a memory leak around
somewhere.
> 
> As discussed offline, the real question here is what the leak is... it appears
to only happen when we have the ThrowException() call in ClearError() - without
that, the context is properly destroyed.  But nothing appears to be hanging on
to the error directly, so it seems like something in v8's machinery.

Filed a v8 bug (https://bugs.chromium.org/p/v8/issues/detail?id=5941) and sent
out a CL with a possible fix. Agreed that this is an issue on the v8 side.

[jbroman, 2017-02-08 16:36:19]

On 2017/02/07 at 18:19:42, jbroman wrote:
>
https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
> File extensions/renderer/api_last_error_unittest.cc (right):
> 
>
https://codereview.chromium.org/2657613005/diff/80001/extensions/renderer/api...
> extensions/renderer/api_last_error_unittest.cc:103: EXPECT_TRUE(did_throw);
> On 2017/02/06 at 20:28:22, Devlin wrote:
> > On 2017/02/06 18:31:26, jbroman wrote:
> > > On 2017/01/31 at 17:15:00, Devlin wrote:
> > > > On 2017/01/30 22:49:52, jbroman wrote:
> > > > > On 2017/01/27 at 21:41:48, Devlin wrote:
> > > > > > Strangely, this test fails.  I've verified that we throw the
exception in
> > > > > api_last_error.cc as expected, but it doesn't reach the message
listener. 
> > > Any
> > > > > idea why?  Should we be throwing in a different way than
> > > > > Isolate::ThrowException()?
> > > > > > 
> > > > > > Also interesting, this test fails in the tear down where we verify
that
> > > the
> > > > > contexts get properly cleaned up, and it seems the culprit is the call
to
> > > > > ThrowException() (if I comment it out, the context is cleaned up
properly). 
> > > Any
> > > > > ideas?
> > > > > 
> > > > > This is a subtlety about how throwing exceptions works. The public
method
> > > > > actually only makes it the "scheduled" exception, which is thrown when
the
> > > V8
> > > > > runtime is entered. As a result, the exception is getting thrown in a
> > > subsequent
> > > > > test, when the isolate next checks for a scheduled exception to
promote.
> > > > > 
> > > > > I think in this case, it's probably simplest to check for the error by
using
> > > a
> > > > > TryCatch and checking HasCaught (you can also get the message from the
> > > TryCatch,
> > > > > if you want to examine it). Nothing strange is going on here that you
should
> > > > > need to use a global message listener (unlike the other case, where
you were
> > > > > checking what happened if an exception was unhandled in an event
listener).
> > > > 
> > > > Using the TryCatch here worked, thanks!  It's still crashing when trying
to
> > > dispose the Isolate, though... any ideas on that front?
> > > 
> > > That's a red herring (due to a bug in APIBindingTest, that I'll send you a
CL
> > > for shortly). The actual problem is that the check that the context can be
> > > collected failed -- which suggests that there's a memory leak around
somewhere.
> > 
> > As discussed offline, the real question here is what the leak is... it
appears to only happen when we have the ThrowException() call in ClearError() -
without that, the context is properly destroyed.  But nothing appears to be
hanging on to the error directly, so it seems like something in v8's machinery.
> 
> Filed a v8 bug (https://bugs.chromium.org/p/v8/issues/detail?id=5941) and sent
out a CL with a possible fix. Agreed that this is an issue on the v8 side.

Landed the v8 side change. You'll have to wait for v8 to roll, but then you can
try again. :)

[Devlin, 2017-02-09 18:23:00]

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

[commit-bot: I haz the power, 2017-02-09 18:23:38]

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-02-09 20:21:02]

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

[commit-bot: I haz the power, 2017-02-09 20:21:03]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-02-09 20:53:29]

Rebased with the v8 roll; all tests passing now.

[jbroman, 2017-02-13 19:41:28]

lgtm

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
File extensions/renderer/api_last_error.cc (right):

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error.cc:82: if
(!maybe_error.ToLocal(&local_error))
nit: Don't have to, but it's usual to do the ToLocal inline with the thing that
could fail, like:

v8::Local<v8::Value> local_error;
if (!parent->Get(context, key).ToLocal(&local_error))
  return;

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error.cc:125: v8::MaybeLocal<v8::Value> maybe_error
= parent->Get(context, key);
ditto here about ToLocal

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
File extensions/renderer/api_last_error.h (right):

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error.h:25: APILastError(const GetParent&
get_parent);
explicit

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error.h:26: APILastError(APILastError&& other);
It's a little weird to have a move constructor without a move assignment
operator, but I guess you never need to move-assign at present?

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
File extensions/renderer/api_last_error_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error_unittest.cc:55: EXPECT_EQ("undefined",
GetLastError(parent_object, context));
At least once, you might want to check that the property is actually absent, as
opposed to having the value undefined, since you're going to the effort of
preserving this behaviour.

Something like:
EXPECT_FALSE(parent_object.Has(context, gin::StringToV8(isolate(),
"lastError")).ToChecked()));

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/na...
File extensions/renderer/native_extension_bindings_system.cc (right):

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/na...
extensions/renderer/native_extension_bindings_system.cc:482:
v8::Local<v8::Object> result;
nit: Does this variable still need to exist? Looks like we could just have
"return value.As<v8::Object>();" and "return root_binding;" below.

[lazyboy, 2017-02-13 21:54:27]

lgtm

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
File extensions/renderer/api_last_error_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error_unittest.cc:122: // TODO(devlin): Or should
we? If someone sets chrome.runtime.lastError, it
This TODO feels more appropriate in line ~90 in api_last_error.cc

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/na...
File extensions/renderer/native_extension_bindings_system_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/na...
extensions/renderer/native_extension_bindings_system_unittest.cc:625: "     
this.lastErrorMessage = chrome.runtime.lastError.message\n"
nit:replace \n with ;\n

[Devlin, 2017-02-14 00:16:58]

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

[commit-bot: I haz the power, 2017-02-14 00:18:13]

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-02-14 01:35:22]

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

[commit-bot: I haz the power, 2017-02-14 01:35:23]

Dry run: Try jobs failed on following builders:
  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-02-14 01:44:26]

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

[commit-bot: I haz the power, 2017-02-14 01:45:08]

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-02-14 03:37:49]

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

[commit-bot: I haz the power, 2017-02-14 03:37:50]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-02-14 04:57:08]

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
File extensions/renderer/api_last_error.cc (right):

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error.cc:82: if
(!maybe_error.ToLocal(&local_error))
On 2017/02/13 19:41:27, jbroman wrote:
> nit: Don't have to, but it's usual to do the ToLocal inline with the thing
that
> could fail, like:
> 
> v8::Local<v8::Value> local_error;
> if (!parent->Get(context, key).ToLocal(&local_error))
>   return;

Whoops, been trying to do that more, but didn't here.  Done.

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error.cc:125: v8::MaybeLocal<v8::Value> maybe_error
= parent->Get(context, key);
On 2017/02/13 19:41:27, jbroman wrote:
> ditto here about ToLocal

Done.

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
File extensions/renderer/api_last_error.h (right):

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error.h:25: APILastError(const GetParent&
get_parent);
On 2017/02/13 19:41:27, jbroman wrote:
> explicit

Done.

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error.h:26: APILastError(APILastError&& other);
On 2017/02/13 19:41:27, jbroman wrote:
> It's a little weird to have a move constructor without a move assignment
> operator, but I guess you never need to move-assign at present?

Right.  Not sure if we have a preferred style here, so I default to "if it isn't
needed, don't add it."

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
File extensions/renderer/api_last_error_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error_unittest.cc:55: EXPECT_EQ("undefined",
GetLastError(parent_object, context));
On 2017/02/13 19:41:27, jbroman wrote:
> At least once, you might want to check that the property is actually absent,
as
> opposed to having the value undefined, since you're going to the effort of
> preserving this behaviour.
> 
> Something like:
> EXPECT_FALSE(parent_object.Has(context, gin::StringToV8(isolate(),
> "lastError")).ToChecked()));

Good call; done.

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/ap...
extensions/renderer/api_last_error_unittest.cc:122: // TODO(devlin): Or should
we? If someone sets chrome.runtime.lastError, it
On 2017/02/13 21:54:27, lazyboy wrote:
> This TODO feels more appropriate in line ~90 in api_last_error.cc

Done.

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/na...
File extensions/renderer/native_extension_bindings_system.cc (right):

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/na...
extensions/renderer/native_extension_bindings_system.cc:482:
v8::Local<v8::Object> result;
On 2017/02/13 19:41:27, jbroman wrote:
> nit: Does this variable still need to exist? Looks like we could just have
> "return value.As<v8::Object>();" and "return root_binding;" below.

Done.

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/na...
File extensions/renderer/native_extension_bindings_system_unittest.cc (right):

https://codereview.chromium.org/2657613005/diff/160001/extensions/renderer/na...
extensions/renderer/native_extension_bindings_system_unittest.cc:625: "     
this.lastErrorMessage = chrome.runtime.lastError.message\n"
On 2017/02/13 21:54:27, lazyboy wrote:
> nit:replace \n with ;\n

Done.

[Devlin, 2017-02-14 04:57:17]

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

[Devlin, 2017-02-14 04:57:17]

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/2657613005/#ps220001
(title: ".")

[commit-bot: I haz the power, 2017-02-14 04:58:00]

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-02-14 05:03:41]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487048237356470,
"parent_rev": "6f380e767298a49098c17a1bea12ff283fe25054", "commit_rev":
"931df34fb40d8130d986b1fc6df37a7f478cf9f9"}

[commit-bot: I haz the power, 2017-02-14 05:05:22]

Description was changed from

==========
[Extensions Bindings] Add chrome.runtime.lastError support

If an API function returns an error, we surface it through
chrome.runtime.lastError. Add support for this with native bindings.

Note: we also currently set the last error on the (deprecated)
chrome.extension.lastError. This CL does not address that.

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

to

==========
[Extensions Bindings] Add chrome.runtime.lastError support

If an API function returns an error, we surface it through
chrome.runtime.lastError. Add support for this with native bindings.

Note: we also currently set the last error on the (deprecated)
chrome.extension.lastError. This CL does not address that.

BUG=653596

Review-Url: https://codereview.chromium.org/2657613005
Cr-Commit-Position: refs/heads/master@{#450256}
Committed:
https://chromium.googlesource.com/chromium/src/+/931df34fb40d8130d986b1fc6df3...
==========

[commit-bot: I haz the power, 2017-02-14 05:05:24]

Committed patchset #8 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/931df34fb40d8130d986b1fc6df3...