Bug 131240 – [Bindings] "nullable" sequence<T> support is incomplete

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Antonio Gomes]

sample foo.idl:

[
    Conditional=XXX,
] interface Foo {
    [RaisesException] void fooIt(sequence<unsigned long>? seq);
}

Generated JSFoo.cpp code:

EncodedJSValue JSC_HOST_CALL jsFooPrototypeFunctionFooIt(ExecState* exec)
{
    JSValue thisValue = exec->hostThisValue();
    JSFoo* castedThis = jsDynamicCast<JSFoo*>(thisValue);
    if (!castedThis)
        return throwVMTypeError(exec);
    ASSERT_GC_OBJECT_INHERITS(castedThis, JSFoo::info());
    Foo& impl = castedThis->impl();
    if (exec->argumentCount() != 1)
        return throwVMError(exec, createNotEnoughArgumentsError(exec));
    ExceptionCode ec = 0;
    Vector<unsigned> seq(toNativeArray<unsigned>(exec, exec->argument(0)));
    if (exec->hadException())
        return JSValue::encode(jsUndefined());
    impl.fooIt(seq, ec);
    setDOMException(exec, ec);
    return JSValue::encode(jsUndefined());
}

In JSDOMBindings.h, "toNativeArray" bails out early if JSValue's (i.e. argument0) getObject is null, throwing a VM error "Value is not a sequence".

According to WebIDL [1] there is no restriction for nullable sequences.

[1] http://www.w3.org/TR/WebIDL/#dfn-nullable-type

[Darin Adler]

Do we have one of these nullable sequences in our bindings already? Can we make a regression test showing that null doesn’t work for that?

[Antonio Gomes]

Created attachment 228758 [details]
patch

I choose to add a bool parameter, named "acceptsNullable", to JSDOMBindings::toNativeArray method, and set it to "true" from the generated code upon the presence of "?" in the idl definition.

It might be that we should simply make toNativeArray to accept "null" arrays always, similarly to toRefPtrNativeArray.

Please let me know.

[Antonio Gomes]

(In reply to comment #1)
> Do we have one of these nullable sequences in our bindings already? Can we make a regression test showing that null doesn’t work for that?

I am afraid it is not there yet, but this discrepancy between toNativeArray and toRefPtrNative array was what brought my attention to this.

[Darin Adler]

I’m not really happy doing this patch when there is no binding yet that uses it. It prevents us from doing any real testing. I suggest we hold off on this until we are actually using it in at least one binding.

[Antonio Gomes]

(In reply to comment #4)
> I’m not really happy doing this patch when there is no binding yet that uses it. It prevents us from doing any real testing. I suggest we hold off on this until we are actually using it in at least one binding.

Hi Darin. Would you be ok with a less intrusive approach/patch, where no changes to the perl code is made, as proposed in comment #2?

Reasons I particularly insist are:

- it seems logical to me to make toNativeArray in pair with toRefPtrNativeArray in that sense (acceptance of null as a valid sequence valid). Particularly, toRefPtrNativeArray does take "null" ok as an empty sequence, while toNativeArray does not.

- there is support for nullable sequences in in the generated code for this***, so it seems logical to make both methods actually called by it (i.e. toNativeArray and toRefPtrNativeArray) to support it.

- there are a couple of specifications making use of this nullable sequence:

1) 
https://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#options-object-
concept

2) http://www.khronos.org/registry/webcl/specs/latest/1.0/#3.5
if you look at
 void enqueueNDRangeKernel(...,
                    sequence<CLuint>?                     globalWorkOffset, 
                    sequence<CLuint>                      globalWorkSize,
                    optional sequence<CLuint>?            localWorkSize,
                    optional sequence<WebCLEvent>?        eventWaitList,
                    optional WebCLEvent?                  event);

"eventWaitList" goes through toRefPtrNativeArray code path and can take "null" just fine. globalWorkSize does through toNativeArray, and fails to accept it.




***see that if (arg.isNull() || ...) gets added.

[Antonio Gomes]

Created attachment 228919 [details]
simpler patch

Patch that does not change the code generator, and simply makes toNativeArray in pair with toRefPtrNativeArray.

Darin, would you mind to look?

[Darin Adler]

Comment on attachment 228919 [details]
simpler patch

This change is completely reasonable. But it needs a real regression test, testing its effect on an actual binding.

If this change has no effect on any actual generated binding, we should not make the change at this time, until it does have an effect.

[Darin Adler]

(In reply to comment #5)
> - there are a couple of specifications making use of this nullable sequence:
> 
> 1) 
> https://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#options-object-
> concept
> 
> 2) http://www.khronos.org/registry/webcl/specs/latest/1.0/#3.5
> if you look at
>  void enqueueNDRangeKernel(...,
>                     sequence<CLuint>?                     globalWorkOffset, 
>                     sequence<CLuint>                      globalWorkSize,
>                     optional sequence<CLuint>?            localWorkSize,
>                     optional sequence<WebCLEvent>?        eventWaitList,
>                     optional WebCLEvent?                  event);
> 
> "eventWaitList" goes through toRefPtrNativeArray code path and can take "null" just fine. globalWorkSize does through toNativeArray, and fails to accept it.

We should land this change when we are landing one of these bindings, so the change has an actual effect on WebKit rather than a theoretical one.

[Antonio Gomes]

Comment on attachment 228919 [details]
simpler patch

Ok. Removing the r? flags.

[Antonio Gomes]

Created attachment 232133 [details]
simpler patch (2) - with WebSocket.idl changes

It turns out WebSocket.idl can benefit from this patch. Changes incorporated.

[Antonio Gomes]

Comment on attachment 232133 [details]
simpler patch (2) - with WebSocket.idl changes

Darin, could you look again?

[Antonio Gomes]

(In reply to comment #11)
> (From update of attachment 232133 [details])
> Darin, could you look again?

Friendly re-ping.

[Darin Adler]

Comment on attachment 232133 [details]
simpler patch (2) - with WebSocket.idl changes

View in context: https://bugs.webkit.org/attachment.cgi?id=232133&action=review

> Source/WebCore/bindings/scripts/test/TestObj.idl:212
> +    void    methodWithAndWithoutNullableSequence(sequence<unsigned long> arrayArg, sequence<unsigned long>? nullableArrayArg);
> +    void    methodWithAndWithoutNullableSequence2(unsigned long[] arrayArg, unsigned long[]? nullableArrayArg);

Should not have four spaces after "void".

[Darin Adler]

Comment on attachment 232133 [details]
simpler patch (2) - with WebSocket.idl changes

View in context: https://bugs.webkit.org/attachment.cgi?id=232133&action=review

> Source/WebCore/Modules/websockets/WebSocket.idl:37
> +    Constructor(DOMString url, [Default=Undefined] optional sequence<DOMString>? protocols),

Do we have test coverage for this?

[Antonio Gomes]

Fixed by <https://webkit.org/b/131240>.