[Extensions Bindings] Add support for updateArgumentsPreValidate

[Extensions Bindings] Add support for updateArgumentsPreValidate

A number of our custom bindings use a hook (updateArgumentsPreValidate)
to "massage" the arguments. This can be necessary when the arguments
passed by the extension are either ambiguous or don't strictly match
what we expect, and need to be converted in some way.

Add a new method on the js hook interface to allow bindings to register
a hook for this, in the same way they previously could. This is a little
complicated because it entails calling back into JS while handling a JS
call, and also modifying the arguments we're going to use.

The former complication, calling back into JS, means that we want to do
that synchronously, as opposed to asynchronously. In theory, since this
is all in a direct reaction to a JS API call, this is okay. In practice,
we default to safety and CHECK our assumptions. That is, we will still
refuse to execute if we shouldn't.

The latter complication, modifying the JS arguments we're going to use,
means that we have to move away from gin::Arguments (which don't allow
mutation) to a vector of v8 values.

Add a number of tests for the same.

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

[Devlin, 2016-12-23 00:25:21]

Description was changed from

==========
updateArgumentsPreValidate
BUG=
==========

to

==========
[Extensions Bindings] Add support for updateArgumentsPreValidate

A number of our custom bindings use a hook (updateArgumentsPreValidate)
to "massage" the arguments. This can be necessary when the arguments
passed by the extension are either ambiguous or don't strictly match
what we expect, and need to be converted in some way.

Add a new method on the js hook interface to allow bindings to register
a hook for this, in the same way they previously could. This is a little
complicated because it entails calling back into JS while handling a JS
call, and also modifying the arguments we're going to use.

The former complication, calling back into JS, means that we want to do
that synchronously, as opposed to asynchronously. In theory, since this
is all in a direct reaction to a JS API call, this is okay. In practice,
we default to safety and CHECK our assumptions. That is, we will still
refuse to execute if we shouldn't.

The latter complication, modifying the JS arguments we're going to use,
means that we have to move away from gin::Arguments (which don't allow
mutation) to a vector of v8 values.

Add a number of tests for the same.

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

[Devlin, 2016-12-23 00:39:05]

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

[Devlin, 2016-12-23 00:39:05]

Heya folks, another one for you.

This technically follow https://codereview.chromium.org/2583273002/, but most of
the content is sufficiently different that I think we can have them running in
parallel.

[jbroman, 2016-12-23 22:37:06]

I'll have a closer look after the holidays, but a couple initial (superficial)
thoughts:

https://codereview.chromium.org/2598123002/diff/40001/extensions/renderer/api...
File extensions/renderer/api_binding.cc (right):

https://codereview.chromium.org/2598123002/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding.cc:200: arguments->ThrowTypeError("Invalid
invocation");
Isn't this caught by the v8::TryCatch a few lines up?

https://codereview.chromium.org/2598123002/diff/40001/extensions/renderer/api...
File extensions/renderer/api_signature.cc (right):

https://codereview.chromium.org/2598123002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature.cc:44: void consume_argument() {
nit: This doesn't look like a simple getter/setter to me, so I think this ought
to be named ConsumeArgument.

[Devlin, 2016-12-28 18:13:17]

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

[Devlin, 2016-12-28 18:13:21]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-12-28 18:13:26]

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, 2016-12-28 18:45:24]

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

[commit-bot: I haz the power, 2016-12-28 18:45:25]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Devlin, 2016-12-28 19:02:39]

https://codereview.chromium.org/2598123002/diff/40001/extensions/renderer/api...
File extensions/renderer/api_binding.cc (right):

https://codereview.chromium.org/2598123002/diff/40001/extensions/renderer/api...
extensions/renderer/api_binding.cc:200: arguments->ThrowTypeError("Invalid
invocation");
On 2016/12/23 22:37:06, jbroman wrote:
> Isn't this caught by the v8::TryCatch a few lines up?

Right you are!  Fixed.

https://codereview.chromium.org/2598123002/diff/40001/extensions/renderer/api...
File extensions/renderer/api_signature.cc (right):

https://codereview.chromium.org/2598123002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature.cc:44: void consume_argument() {
On 2016/12/23 22:37:06, jbroman wrote:
> nit: This doesn't look like a simple getter/setter to me, so I think this
ought
> to be named ConsumeArgument.

Not entirely sure I agree (it's basically just incrementing the internal member,
and I think incrementers are allowed to be hacker_case since they're largely
setters) - but given it's a private API in a .cc file, I don't feel strongly at
all :)

Done.

[jbroman, 2017-01-02 19:46:38]

lgtm with some nits

Hopefully some of this complexity can go away in the future.

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_binding.cc (right):

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding.cc:197: if (try_catch.HasCaught()) {
Why would this ever catch? Nothing about checking argument length or copying
handles into a vector should throw an exception. If no exception can be thrown
above, then the type error throwing logic below can be merged into here.

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_binding_hooks.cc (right):

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding_hooks.cc:44: v8::Local<v8::Function>
GetHandleRequestHook(const std::string& method_name,
super-nit: can these and GetHookFromMap be const?

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding_hooks.cc:73: (*map)[qualified_method_name];
nit: This should still fit on one line.

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding_hooks.cc:233: 
super-nit: for consistency with the PreValidationHook code above, delete this
empty line.

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_binding_types.h (right):

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding_types.h:28:
base::Callback<v8::Global<v8::Value>(v8::Local<v8::Function>,
Mind a comment alluding to why this returns a Global<Value> rather than a
Local<Value> as would be usual? Ordinarily I'd expect that (using an
EscapableHandleScope), so this signature seems odd except for the way it's
implemented using WebLocalFrame's interface.

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_binding_unittest.cc (right):

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding_unittest.cc:557: // Tests the
updateArgumentsPreValidate hook.
It looks like it's legal for this hook to throw an exception. Should we at least
have a unit test for the case where it does?

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

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/na...
extensions/renderer/native_extension_bindings_system.cc:125:
script_context->SafeCallFunction(function, argc, argv, helper.GetCallback());
WDYT of this, which seems to do the same but be somewhat shorter than the helper
class:

bool did_complete = false;
v8::Global<v8::Value> result;
auto callback = base::Bind([](v8::Isolate* isolate,
                              bool* did_complete_out,
                              v8::Local<v8::Value>* result_out,
                              const std::vector<v8::Local<v8::Value>>& results)
{
  *did_complete_out = true;
  // The locals are released after the callback is executed, so we need to
  // grab a persistent handle.
  if (!results.empty() && !results[0].IsEmpty())
    result_out->Reset(isolate, results[0]);
}, base::Unretained(isolate), base::Unretained(&did_complete),
base::Unretained(&result));

ScriptContext* script_context =
    ScriptContextSet::GetContextByV8Context(context);
CHECK(script_context);
script_context->SafeCallFunction(function, argc, argv, callback);
CHECK(did_complete) << "expected script to execute synchronously";
return result;

I don't feel strongly; just a thought.

We already CHECK if the callback isn't invoked before the helper could die, so I
don't think the weak-ptr indirection helps.

(I'm not thrilled with passing around Global here as Local is usually used to
pass things around on the stack, but I can see why it arises from the way
SafeCallFunction works. :/)

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

lazyboy@, wanna take a look?

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_binding.cc (right):

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding.cc:197: if (try_catch.HasCaught()) {
On 2017/01/02 19:46:38, jbroman wrote:
> Why would this ever catch? Nothing about checking argument length or copying
> handles into a vector should throw an exception. If no exception can be thrown
> above, then the type error throwing logic below can be merged into here.

Nope, shouldn't catch (and also should't fail).  Simplified.

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_binding_hooks.cc (right):

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding_hooks.cc:44: v8::Local<v8::Function>
GetHandleRequestHook(const std::string& method_name,
On 2017/01/02 19:46:38, jbroman wrote:
> super-nit: can these and GetHookFromMap be const?

Yep, done.

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding_hooks.cc:73: (*map)[qualified_method_name];
On 2017/01/02 19:46:38, jbroman wrote:
> nit: This should still fit on one line.

huh - wonder why git cl format didn't do this.  Done.

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding_hooks.cc:233: 
On 2017/01/02 19:46:38, jbroman wrote:
> super-nit: for consistency with the PreValidationHook code above, delete this
> empty line.

Done.

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_binding_types.h (right):

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding_types.h:28:
base::Callback<v8::Global<v8::Value>(v8::Local<v8::Function>,
On 2017/01/02 19:46:38, jbroman wrote:
> Mind a comment alluding to why this returns a Global<Value> rather than a
> Local<Value> as would be usual? Ordinarily I'd expect that (using an
> EscapableHandleScope), so this signature seems odd except for the way it's
> implemented using WebLocalFrame's interface.

Done.

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
File extensions/renderer/api_binding_unittest.cc (right):

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/ap...
extensions/renderer/api_binding_unittest.cc:557: // Tests the
updateArgumentsPreValidate hook.
On 2017/01/02 19:46:38, jbroman wrote:
> It looks like it's legal for this hook to throw an exception. Should we at
least
> have a unit test for the case where it does?

Done.

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

https://codereview.chromium.org/2598123002/diff/140001/extensions/renderer/na...
extensions/renderer/native_extension_bindings_system.cc:125:
script_context->SafeCallFunction(function, argc, argv, helper.GetCallback());
On 2017/01/02 19:46:38, jbroman wrote:
> WDYT of this, which seems to do the same but be somewhat shorter than the
helper
> class:
> 
> bool did_complete = false;
> v8::Global<v8::Value> result;
> auto callback = base::Bind([](v8::Isolate* isolate,
>                               bool* did_complete_out,
>                               v8::Local<v8::Value>* result_out,
>                               const std::vector<v8::Local<v8::Value>>&
results)
> {
>   *did_complete_out = true;
>   // The locals are released after the callback is executed, so we need to
>   // grab a persistent handle.
>   if (!results.empty() && !results[0].IsEmpty())
>     result_out->Reset(isolate, results[0]);
> }, base::Unretained(isolate), base::Unretained(&did_complete),
> base::Unretained(&result));
> 
> ScriptContext* script_context =
>     ScriptContextSet::GetContextByV8Context(context);
> CHECK(script_context);
> script_context->SafeCallFunction(function, argc, argv, callback);
> CHECK(did_complete) << "expected script to execute synchronously";
> return result;
> 
> I don't feel strongly; just a thought.
> 
> We already CHECK if the callback isn't invoked before the helper could die, so
I
> don't think the weak-ptr indirection helps.
> 
> (I'm not thrilled with passing around Global here as Local is usually used to
> pass things around on the stack, but I can see why it arises from the way
> SafeCallFunction works. :/)

I like it.  Done.

I've also added a TODO to express the desire to use Local instead (in the
comment in the types.h file).

[Devlin, 2017-01-04 17:57:36]

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

[commit-bot: I haz the power, 2017-01-04 17:57:51]

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-04 18:50:13]

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

[commit-bot: I haz the power, 2017-01-04 18:50:14]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-01-05 20:10:35]

lgtm w/ one q.

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

https://codereview.chromium.org/2598123002/diff/160001/extensions/renderer/ap...
extensions/renderer/api_binding_hooks.cc:245: run_js_.Run(handle_request,
context, parsed_v8_args.size(),
run_js_ RunJSFunctionSync now, but we don't care about the returned results
here. Should we use RunJSFunc (after constructing APIBindingHooks with it)
instead?

[Devlin, 2017-01-05 21:03:51]

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

https://codereview.chromium.org/2598123002/diff/160001/extensions/renderer/ap...
extensions/renderer/api_binding_hooks.cc:245: run_js_.Run(handle_request,
context, parsed_v8_args.size(),
On 2017/01/05 20:10:35, lazyboy wrote:
> run_js_ RunJSFunctionSync now, but we don't care about the returned results
> here. Should we use RunJSFunc (after constructing APIBindingHooks with it)
> instead?

I think using the sync version is fine here, since we (should) know that it can
execute synchronously.  If it ever fails, it means we're doing something wrong,
and could be indicative of a larger bug.  So I'd like to keep it as-is for now.

[Devlin, 2017-01-05 21:31:00]

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

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

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/2598123002/#ps160001
(title: "jbroman II")

[commit-bot: I haz the power, 2017-01-05 21:31:46]

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-05 21:44:07]

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

[commit-bot: I haz the power, 2017-01-05 21:44:08]

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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-01-05 21:47:56]

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

[commit-bot: I haz the power, 2017-01-05 21:48:43]

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

[Devlin, 2017-01-05 22:45:41]

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

[Devlin, 2017-01-05 22:45:44]

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

[Devlin, 2017-01-05 22:45:44]

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/2598123002/#ps180001
(title: "rebase")

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

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-06 02:12:33]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1483656344155550,
"parent_rev": "fc75b76dcee89b3650303f90981e96b10ebaef23", "commit_rev":
"f4c89ab6c9e2d821172cd1cded6163aefa894c11"}

[commit-bot: I haz the power, 2017-01-06 02:13:02]

Description was changed from

==========
[Extensions Bindings] Add support for updateArgumentsPreValidate

A number of our custom bindings use a hook (updateArgumentsPreValidate)
to "massage" the arguments. This can be necessary when the arguments
passed by the extension are either ambiguous or don't strictly match
what we expect, and need to be converted in some way.

Add a new method on the js hook interface to allow bindings to register
a hook for this, in the same way they previously could. This is a little
complicated because it entails calling back into JS while handling a JS
call, and also modifying the arguments we're going to use.

The former complication, calling back into JS, means that we want to do
that synchronously, as opposed to asynchronously. In theory, since this
is all in a direct reaction to a JS API call, this is okay. In practice,
we default to safety and CHECK our assumptions. That is, we will still
refuse to execute if we shouldn't.

The latter complication, modifying the JS arguments we're going to use,
means that we have to move away from gin::Arguments (which don't allow
mutation) to a vector of v8 values.

Add a number of tests for the same.

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

to

==========
[Extensions Bindings] Add support for updateArgumentsPreValidate

A number of our custom bindings use a hook (updateArgumentsPreValidate)
to "massage" the arguments. This can be necessary when the arguments
passed by the extension are either ambiguous or don't strictly match
what we expect, and need to be converted in some way.

Add a new method on the js hook interface to allow bindings to register
a hook for this, in the same way they previously could. This is a little
complicated because it entails calling back into JS while handling a JS
call, and also modifying the arguments we're going to use.

The former complication, calling back into JS, means that we want to do
that synchronously, as opposed to asynchronously. In theory, since this
is all in a direct reaction to a JS API call, this is okay. In practice,
we default to safety and CHECK our assumptions. That is, we will still
refuse to execute if we shouldn't.

The latter complication, modifying the JS arguments we're going to use,
means that we have to move away from gin::Arguments (which don't allow
mutation) to a vector of v8 values.

Add a number of tests for the same.

BUG=653596

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

[commit-bot: I haz the power, 2017-01-06 02:13:03]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/f4c89ab6c9e2d821172cd1cded61...