Add NativeValueTraits for ScriptValue to V8Bindings.

Add NativeValueTraits for ScriptValue to V8Bindings.

Argument conversion, particularly variadic argument conversion, goes
through toNativeArray<>(), which means that this is needed to support
idl for arguments of type "any...".

R=haraken@chromium.org
BUG=None

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179856

[gavinp, 2014-08-05 23:06:20]

Hara-san: PTAL.
Hirano-san: FYI.

You have seen this bindings code before; in the CL
https://codereview.chromium.org/434543002/ . Here it is carved out with
significant unit tests.

There's work still to be done to test the IDL side of it; but this is a big
improvement over where we were.

WDYT?

[haraken, 2014-08-06 00:55:11]

https://codereview.chromium.org/440243003/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/440243003/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.h:616: return
ScriptValue(ScriptState::current(isolate), value);

I'm a bit concerned about this. In order to call ScriptState::current(), it must
be guaranteed that the caller site is in a correct ScriptState. I'm not sure if
it's guaranteed for nativeValue() (today and in the future).

Where do you need the nativeValue for ScriptValue, specifically?

[gavinp, 2014-08-06 01:30:08]

On 2014/08/06 00:55:11, haraken wrote:
>
https://codereview.chromium.org/440243003/diff/1/Source/bindings/core/v8/V8Bi...
> File Source/bindings/core/v8/V8Binding.h (right):
> 
>
https://codereview.chromium.org/440243003/diff/1/Source/bindings/core/v8/V8Bi...
> Source/bindings/core/v8/V8Binding.h:616: return
> ScriptValue(ScriptState::current(isolate), value);
> 
> I'm a bit concerned about this. In order to call ScriptState::current(), it
must
> be guaranteed that the caller site is in a correct ScriptState. I'm not sure
if
> it's guaranteed for nativeValue() (today and in the future).
> 
> Where do you need the nativeValue for ScriptValue, specifically?

"Argument conversion, particularly variadic argument conversion, goes
through toNativeArray<>(), which means that this is needed to support
idl for arguments of type "any..."."

The IDL converts the variadic arguments using variadic arrays. You reviewed this
same code in https://codereview.chromium.org/434543002/ , and without it the
Cache.idl entry "addAll(any...)" does not work, as the generated code cannot
compile.

[gavinp, 2014-08-06 01:32:23]

On 2014/08/06 01:30:08, gavinp wrote:
> On 2014/08/06 00:55:11, haraken wrote:
> >
>
https://codereview.chromium.org/440243003/diff/1/Source/bindings/core/v8/V8Bi...
> > File Source/bindings/core/v8/V8Binding.h (right):
> > 
> >
>
https://codereview.chromium.org/440243003/diff/1/Source/bindings/core/v8/V8Bi...
> > Source/bindings/core/v8/V8Binding.h:616: return
> > ScriptValue(ScriptState::current(isolate), value);
> > 
> > I'm a bit concerned about this. In order to call ScriptState::current(), it
> must
> > be guaranteed that the caller site is in a correct ScriptState. I'm not sure
> if
> > it's guaranteed for nativeValue() (today and in the future).
> > 
> > Where do you need the nativeValue for ScriptValue, specifically?
> 
> "Argument conversion, particularly variadic argument conversion, goes
> through toNativeArray<>(), which means that this is needed to support
> idl for arguments of type "any..."."
> 
> The IDL converts the variadic arguments using variadic arrays. You reviewed
this
> same code in https://codereview.chromium.org/434543002/ , and without it the
> Cache.idl entry "addAll(any...)" does not work, as the generated code cannot
> compile.

My development process was to look at how the bindings for a single "any"
argument are made, and copy that; so I'm now concerned that we don't properly
support arguments of type "any" in a safe way?

[haraken, 2014-08-06 01:37:09]

On 2014/08/06 01:32:23, gavinp wrote:
> On 2014/08/06 01:30:08, gavinp wrote:
> > On 2014/08/06 00:55:11, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/440243003/diff/1/Source/bindings/core/v8/V8Bi...
> > > File Source/bindings/core/v8/V8Binding.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/440243003/diff/1/Source/bindings/core/v8/V8Bi...
> > > Source/bindings/core/v8/V8Binding.h:616: return
> > > ScriptValue(ScriptState::current(isolate), value);
> > > 
> > > I'm a bit concerned about this. In order to call ScriptState::current(),
it
> > must
> > > be guaranteed that the caller site is in a correct ScriptState. I'm not
sure
> > if
> > > it's guaranteed for nativeValue() (today and in the future).
> > > 
> > > Where do you need the nativeValue for ScriptValue, specifically?
> > 
> > "Argument conversion, particularly variadic argument conversion, goes
> > through toNativeArray<>(), which means that this is needed to support
> > idl for arguments of type "any..."."
> > 
> > The IDL converts the variadic arguments using variadic arrays. You reviewed
> this
> > same code in https://codereview.chromium.org/434543002/ , and without it the
> > Cache.idl entry "addAll(any...)" does not work, as the generated code cannot
> > compile.
> 
> My development process was to look at how the bindings for a single "any"
> argument are made, and copy that; so I'm now concerned that we don't properly
> support arguments of type "any" in a safe way?

I think I've made them safe by making sure that ScriptValue is always associated
with a correct ScriptState.

The problem here is that there is a risk of associating ScriptValue with an
incorrect ScriptState. If any call site of nativeValue() is not in a correct
ScriptState, we will end up creating a ScriptValue with an incorrect
ScriptState. This can lead to cross-world wrapper leakage.

[gavinp, 2014-08-06 01:42:36]

On 2014/08/06 01:32:23, gavinp wrote:
> On 2014/08/06 01:30:08, gavinp wrote:
> > On 2014/08/06 00:55:11, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/440243003/diff/1/Source/bindings/core/v8/V8Bi...
> > > File Source/bindings/core/v8/V8Binding.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/440243003/diff/1/Source/bindings/core/v8/V8Bi...
> > > Source/bindings/core/v8/V8Binding.h:616: return
> > > ScriptValue(ScriptState::current(isolate), value);
> > > 
> > > I'm a bit concerned about this. In order to call ScriptState::current(),
it
> > must
> > > be guaranteed that the caller site is in a correct ScriptState. I'm not
sure
> > if
> > > it's guaranteed for nativeValue() (today and in the future).
> > > 
> > > Where do you need the nativeValue for ScriptValue, specifically?
> > 
> > "Argument conversion, particularly variadic argument conversion, goes
> > through toNativeArray<>(), which means that this is needed to support
> > idl for arguments of type "any..."."
> > 
> > The IDL converts the variadic arguments using variadic arrays. You reviewed
> this
> > same code in https://codereview.chromium.org/434543002/ , and without it the
> > Cache.idl entry "addAll(any...)" does not work, as the generated code cannot
> > compile.
> 
> My development process was to look at how the bindings for a single "any"
> argument are made, and copy that; so I'm now concerned that we don't properly
> support arguments of type "any" in a safe way?

Nevermind. The spec authors have changed the API so I no longer need "any..."
arguments. That's... Amazing.

[haraken, 2014-08-06 01:42:36]

Discussed offline. The ServiceWorker API has changed, so it looks like we no
longer need this CL.

[gavinp, 2014-08-07 01:29:25]

I spoke too soon: still needed in the downstream patch for sequence<any>.

What are we going to do if we can't patch NativeValueTraits? I would like to be
able to bind properly here...

(re-opened issue)

[haraken, 2014-08-07 06:22:05]

LGTM

I checked all call sites of nativeValue(). The nativeValue() is used only by
toNativeArray() and toNativeArguments(). The toNativeArray() and
toNativeArguments() are used only by auto-generated code. Thus it's guaranteed
that we're in a correct ScriptState when we're in the nativeValue().

[gavinp, 2014-08-08 17:27:32]

Thanks Hara-san!

I am going to land this, because it is a fix for the issue at hand. However, I
share the concern that these methods could be called from bad contexts; so
please include me on downstream reviews for that, or ask for my help fixing
them. I'm really interested in bindings now that I've run into limitations twice
in two IDL files I've written.

[gavinp, 2014-08-08 17:28:35]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-08-08 17:29:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gavinp@chromium.org/440243003/1

[gavinp, 2014-08-08 17:29:37]

On 2014/08/08 17:27:32, gavinp wrote:
> Thanks Hara-san!
> 
> I am going to land this, because it is a fix for the issue at hand. However, I
> share the concern that these methods could be called from bad contexts; so
> please include me on downstream reviews for that, or ask for my help fixing
> them. I'm really interested in bindings now that I've run into limitations
twice
> in two IDL files I've written.

BTW, I think one of the easiest ways to fix this is: make a downstream IDL only
include file with the "unsafe" bindings, then a DEPS rule to guard against any
use of that #include from anything other than IDL output.

Thoughts?

[commit-bot: I haz the power, 2014-08-08 18:54:21]

Change committed as 179856

[haraken, 2014-08-09 00:49:28]

On 2014/08/08 17:29:37, gavinp wrote:
> On 2014/08/08 17:27:32, gavinp wrote:
> > Thanks Hara-san!
> > 
> > I am going to land this, because it is a fix for the issue at hand. However,
I
> > share the concern that these methods could be called from bad contexts; so
> > please include me on downstream reviews for that, or ask for my help fixing
> > them. I'm really interested in bindings now that I've run into limitations
> twice
> > in two IDL files I've written.
> 
> BTW, I think one of the easiest ways to fix this is: make a downstream IDL
only
> include file with the "unsafe" bindings, then a DEPS rule to guard against any
> use of that #include from anything other than IDL output.
> 
> Thoughts?

Ideally we want to limit the usage of ScriptState::current to bindings/.
However, there are a bunch of code that is calling ScriptState::current
(directly and indirectly) from core/, and we're not yet getting there. We should
aim at getting there.