Validation of extension api callback and event parameters in DEBUG

Validation of extension api callbacks and event parameters in DEBUG

BUG=18711
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=24068

[rafaelw, 2009-08-19 21:22:50]

Matt, please review .cc & .h changes. The goal here is just to DCHECK if an
event or callback has return anything other than undefined.

Aaron, please review the rest.

[Matt Perry, 2009-08-19 21:42:17]

http://codereview.chromium.org/173034/diff/36/39
File chrome/renderer/extensions/bindings_utils.h (right):

http://codereview.chromium.org/173034/diff/36/39#newcode110
Line 110: // the function call. If an exception is thrown NULL will be returned.
What do you mean be NULL? v8::Null()? Also, I don't see where we actually handle
any v8 exceptions. I think there might be some code you have to do to cleanup
the JS state after one is thrown.

http://codereview.chromium.org/173034/diff/36/40
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/173034/diff/36/40#newcode299
Line 299: DCHECK(false);
use:
  NOTREACHED() << error;
that way the error will be printed to the console.

http://codereview.chromium.org/173034/diff/36/44
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/173034/diff/36/44#newcode86
Line 86: // if they have.
I don't see why we don't just always validate. Shouldn't the callback args be
valid regardless of whether the caller specified a callback?

[rafaelw, 2009-08-19 22:55:02]

http://codereview.chromium.org/173034/diff/36/39
File chrome/renderer/extensions/bindings_utils.h (right):

http://codereview.chromium.org/173034/diff/36/39#newcode110
Line 110: // the function call. If an exception is thrown NULL will be returned.
On 2009/08/19 21:42:17, Matt Perry wrote:
> What do you mean be NULL? v8::Null()? Also, I don't see where we actually
handle
> any v8 exceptions. I think there might be some code you have to do to cleanup
> the JS state after one is thrown.

If the called function throws an exception that isn't caught the function call
actually return NULL (0x000000). retval.IsEmpty() is just a wrapper around
checking that the intrinsic value is 0.

I tried catching a thrown exception. The problem with that is that as a
developer you want the thrown exception to show up in console.error as "Uncaught
exception" you see that you caused it. Even if i catch & re-throw in c++, it
doesn't show up in the console. That seems to happen only if it's uncaught.

http://codereview.chromium.org/173034/diff/36/40
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/173034/diff/36/40#newcode299
Line 299: DCHECK(false);
On 2009/08/19 21:42:17, Matt Perry wrote:
> use:
>   NOTREACHED() << error;
> that way the error will be printed to the console.

Done.

http://codereview.chromium.org/173034/diff/36/44
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/173034/diff/36/44#newcode86
Line 86: // if they have.
On 2009/08/19 21:42:17, Matt Perry wrote:
> I don't see why we don't just always validate. Shouldn't the callback args be
> valid regardless of whether the caller specified a callback?

Currently, the C++ implementation may have checked has_callback() decided not to
send anything back. We could change that.

[Matt Perry, 2009-08-19 23:00:31]

lgtm

http://codereview.chromium.org/173034/diff/36/39
File chrome/renderer/extensions/bindings_utils.h (right):

http://codereview.chromium.org/173034/diff/36/39#newcode110
Line 110: // the function call. If an exception is thrown NULL will be returned.
On 2009/08/19 22:55:02, rafaelw wrote:
> On 2009/08/19 21:42:17, Matt Perry wrote:
> > What do you mean be NULL? v8::Null()? Also, I don't see where we actually
> handle
> > any v8 exceptions. I think there might be some code you have to do to
cleanup
> > the JS state after one is thrown.
> 
> If the called function throws an exception that isn't caught the function call
> actually return NULL (0x000000). retval.IsEmpty() is just a wrapper around
> checking that the intrinsic value is 0.

Ok, in that case, can you say it returns an empty Handle? NULL doesn't really
make sense for this data type.

> I tried catching a thrown exception. The problem with that is that as a
> developer you want the thrown exception to show up in console.error as
"Uncaught
> exception" you see that you caused it. Even if i catch & re-throw in c++, it
> doesn't show up in the console. That seems to happen only if it's uncaught.

I'm surprised it manages to show up in the console that way. We should be at the
bottom of the stack when we call this function, so no one is below us to handle
exceptions and print them. Anyway, looks good.

http://codereview.chromium.org/173034/diff/36/44
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/173034/diff/36/44#newcode86
Line 86: // if they have.
On 2009/08/19 22:55:02, rafaelw wrote:
> On 2009/08/19 21:42:17, Matt Perry wrote:
> > I don't see why we don't just always validate. Shouldn't the callback args
be
> > valid regardless of whether the caller specified a callback?
> 
> Currently, the C++ implementation may have checked has_callback() decided not
to
> send anything back. We could change that.
> 

Makes sense.

[Aaron Boodman, 2009-08-20 22:20:50]

lgtm

http://codereview.chromium.org/173034/diff/61/70
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/173034/diff/61/70#newcode27
Line 27: chrome.Event = function(opt_eventName, argSchemas) {
Maybe name this opt_argSchemas to be consistent with the other arg?

http://codereview.chromium.org/173034/diff/61/71
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/173034/diff/61/71#newcode86
Line 86: // if they have.
This comment seems like the right thing to do, but doesn't seem to go with the
statement immediately following... ?

[rafaelw, 2009-08-22 05:59:28]

http://codereview.chromium.org/173034/diff/36/39
File chrome/renderer/extensions/bindings_utils.h (right):

http://codereview.chromium.org/173034/diff/36/39#newcode110
Line 110: // the function call. If an exception is thrown NULL will be returned.
On 2009/08/19 23:00:31, Matt Perry wrote:
> On 2009/08/19 22:55:02, rafaelw wrote:
> > On 2009/08/19 21:42:17, Matt Perry wrote:
> > > What do you mean be NULL? v8::Null()? Also, I don't see where we actually
> > handle
> > > any v8 exceptions. I think there might be some code you have to do to
> cleanup
> > > the JS state after one is thrown.
> > 
> > If the called function throws an exception that isn't caught the function
call
> > actually return NULL (0x000000). retval.IsEmpty() is just a wrapper around
> > checking that the intrinsic value is 0.
> 
> Ok, in that case, can you say it returns an empty Handle? NULL doesn't really
> make sense for this data type.
> 
> > I tried catching a thrown exception. The problem with that is that as a
> > developer you want the thrown exception to show up in console.error as
> "Uncaught
> > exception" you see that you caused it. Even if i catch & re-throw in c++, it
> > doesn't show up in the console. That seems to happen only if it's uncaught.
> 
> I'm surprised it manages to show up in the console that way. We should be at
the
> bottom of the stack when we call this function, so no one is below us to
handle
> exceptions and print them. Anyway, looks good.

Done.

http://codereview.chromium.org/173034/diff/61/70
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/173034/diff/61/70#newcode27
Line 27: chrome.Event = function(opt_eventName, argSchemas) {
On 2009/08/20 22:20:50, Aaron Boodman wrote:
> Maybe name this opt_argSchemas to be consistent with the other arg?

Done.

http://codereview.chromium.org/173034/diff/61/71
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/173034/diff/61/71#newcode86
Line 86: // if they have.
On 2009/08/20 22:20:50, Aaron Boodman wrote:
> This comment seems like the right thing to do, but doesn't seem to go with the
> statement immediately following... ?

Done.