Bug 84217 – Binding: IDL type DOMString[] shouldn't match null

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

Binding: IDL type DOMString[] shouldn't match null

[Joshua Bell]

Created attachment 137641 [details]
Patch

[Joshua Bell]

haraken@ - r?

This is to support an upcoming addition of overloads for IDBObjectStore:

IDBIndex createIndex(DOMString name, DOMString[] keyPath, ...);
IDBIndex createIndex(DOMString name, DOMString keyPath, ...);

When called as createIndex("foo", null) or createIndex("foo", undefined), by my reading of WebIDL the expectation is that the second parameter is handled as a DOMString, e.g. as createIndex("foo", "null") or createIndex("foo", "undefined"). Therefore, the DOMString[] overload - which is a special case, pending real sequence<T>, T[] and T[]? support - should not match,

[Kentaro Hara]

What should happen in this case:

void func(float[] array);
void func(float f);

Before this patch, null will match 'array'. After this patch, null will match 'f'. It sounds strange to me. (I do not fully understand the Web IDL spec of overloading though. It is too complicated:-)

Maybe we should fix the matching order (i.e. check if null matches with DOMString, and then check if null matches with DOMString[] etc).

[Joshua Bell]

(BTW, I split this off from the main patch I'm working on and can land the rest w/o this, it'll just be incorrect in this edge case and I can include a FIXME in the test. So I'm not in a rush to land this.)

(In reply to comment #3)
> What should happen in this case:
> 
> void func(float[] array);
> void func(float f);
>
> Before this patch, null will match 'array'. After this patch, null will match 'f'. It sounds strange to me. (I do not fully understand the Web IDL spec of overloading though. It is too complicated:-)

Agreed that this will change, and that it's complicated - I don't fully understand it either.

I believe T[] should never match null, although T[]? can. So if float[] is matching null that'd be a bug.

This is by http://www.w3.org/TR/WebIDL/#es-array which starts off with a conversion to sequence<T>, and http://www.w3.org/TR/WebIDL/#es-sequence seems to let null fall into the Otherwise -> TypeError case

> Maybe we should fix the matching order (i.e. check if null matches with DOMString, and then check if null matches with DOMString[] etc).

Looking at http://www.w3.org/TR/WebIDL/#es-DOMString and http://www.w3.org/TR/WebIDL/#es-float then if it makes it that far either ToString() or ToNumber() will coerce null. 

The overloading part of the spec http://www.w3.org/TR/WebIDL/#idl-overloading seems rigorous but I haven't grokked it fully. http://www.w3.org/TR/WebIDL/#dfn-distinguishable indicates to me that deciding between DOMString and a sequence<T> or T[] overload should not depend on the order in the IDL definition. (I think I ended up in a previous patch putting the DOMString overload last since otherwise passing anything would match DOMString.) So from a WebIDL perspective there isn't ambiguity. 

The spec doesn't outright state how to match the overload list against the parameters being passed. My reading is that this is implied by the individual types - i.e. no two "distinguishable" types should match a parameter if the IDL is not ambiguous; rather, all but one in an unambiguous overload would produce a TypeError.

Thinking about the code generator output, that implies seems to me that DOMString is too greedy - it should not match for anthing that could match other types in the http://www.w3.org/TR/WebIDL/#dfn-distinguishable grid. So DOMString should not match anything that interface, object, dictionary, sequence<T>, T[] or Date can. But... that does not seem compatible with the Web.

More thought is required.

[Kentaro Hara]

CC-ing experts.

[Joshua Bell]

+vineet who's apparently working on sequence<T>

[Eric Seidel (no email)]

Comment on attachment 137641 [details]
Patch

Seems reasonable.

[Erik Arvidsson]

Comment on attachment 137641 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1227
> +            push(@andExpression, "((${value}.isObject() && asObject(${value})->inherits(&JSArray::s_info)))");

inherits is not the same as IsArray in the V8 case.

The following would match inherits but fail to match IsArray.

var obj = Object.create([]);

Does the spec require [[Class]] to be "Array" or does it require Array.prototype to be on the prototype chain?

[Eric Seidel (no email)]

Thank you arv.

[Erik Arvidsson]

(In reply to comment #8)
> Does the spec require [[Class]] to be "Array" or does it require Array.prototype to be on the prototype chain?

Replying myself:

"The value of the internal [[Prototype]] property of a platform array object must be the Array prototype object ([ECMA-262], section 15.4.4) from its associated global environment."

so the JSC path looks right but the V8 path looks wrong.

[Joshua Bell]

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1227
> > +            push(@andExpression, "((${value}.isObject() && asObject(${value})->inherits(&JSArray::s_info)))");
> 
> inherits is not the same as IsArray in the V8 case.
> 
> The following would match inherits but fail to match IsArray.
> 
> var obj = Object.create([]);
 
> "The value of the internal [[Prototype]] property of a platform array object must be the Array prototype object ([ECMA-262], section 15.4.4) from its associated global environment."
> 
> so the JSC path looks right but the V8 path looks wrong.

Isn't this backwards? Wouldn't the JSC path allow the Array prototype object to appear anywhere on the prototype chain, while the spec requires that the [[Prototype]] is the Array prototype object? So x = Object.create([]) would have x's prototype's prototype be the Array prototype, which would not match the spec. Object.create(Array.prototype) would match the spec.

All that said...

That clause in the spec is defining a platform array object, but doesn't dictate how ECMAScript types are mapped to T[] when calling, which is what this bug is about:


"An ECMAScript value V is converted to an IDL array value of type T[] as follows:

* If V is a platform array object whose element type is T, then return the array that the platform array object represents.
* Let values be the result of converting V to a sequence with element type T.
* Let n be the length of values.
* Return a new fixed length array of length n whose element values are the same as those in values."

Which is basically: if V is a platform array object, it's already a T[]. Otherwise, convert the ECMAScript object to a sequence<T> then into a T[].

http://dev.w3.org/2006/webapi/WebIDL/#es-sequence has the blather on ECMAScript object to sequence<T>, which has clauses for:

* A platform array object
* A native Array object
* A platform object that supports indexed properties
* Otherwise

The clause we're talking about would be "A native Array object". Is that well defined?

[Joshua Bell]

Created attachment 149979 [details]
Simple test case for Firefox

Added a test case which verifies the Firefox behavior for IDBDatabase::transaction() - calling with transaction(null) and transaction(undefined) are the same as transaction("null") and transaction("undefined") respectively, which agrees with the spec that DOMString[] and DOMStringList should not match null or undefined without additional qualifiers.

[Joshua Bell]

arv@ - any comments on the above?

In any case, the proposed patch doesn't change the already existing "is this an array?" tests, just removes "is this null?" test, so I'm not sure this patch should be rejected. (IMHO should file a separate bug to correct the "is this an array?" tests if necessary)

[Erik Arvidsson]

(In reply to comment #13)
> arv@ - any comments on the above?
> 
> In any case, the proposed patch doesn't change the already existing "is this an array?" tests, just removes "is this null?" test, so I'm not sure this patch should be rejected. (IMHO should file a separate bug to correct the "is this an array?" tests if necessary)

Fair enough.

However, can you use isArray (or whatever it is called in JSC) instead of inherits to make the binding code more similar across the VMs?

[Joshua Bell]

Created attachment 150001 [details]
Patch

[Joshua Bell]

(In reply to comment #14)
> However, can you use isArray (or whatever it is called in JSC) instead of inherits to make the binding code more similar across the VMs?

Yep, done!

...

Note that the attached patch doesn't go far enough to actually make the troublesome overload case in IndexedDB work. The overloads (sans attributes) are:

transaction(in DOMStringList storeNames, in DOMString mode)
transaction(in DOMString[] storeNames, in DOMString mode)
transaction(in DOMString storeName, in DOMString mode);

The attached patch corrects the behavior so an array type (i.e. DOMString[]) doesn't match null, but DOMStringList is still matched - the V8 code generator emits this test:

if ((args.Length() == 2 && (args[0]->IsNull() || V8DOMStringList::HasInstance(args[0]))))

... which is coming from:

elsif (IsWrapperType($type)) {
    push(@andExpression, "(${value}->IsNull() || V8${type}::HasInstance($value))");
}

Which, of course, is used for many types beyond DOMStringList - for example, the TestObj in the bindings tests. Basically, we appear to treat any wrapper type as Nullable. 

Any thoughts on this? Land the patch as-is, then pursue a follow-up patch to e.g. add a [NonNullable] attribute or something, in lieu of full Nullable support?

[Joshua Bell]

(In reply to comment #16)
> 
> push(@andExpression, "(${value}->IsNull() || V8${type}::HasInstance($value))");

I could also upload a patch that drops the IsNull() clause from the above and see what breaks. :)

[Erik Arvidsson]

(In reply to comment #16)
> Any thoughts on this? Land the patch as-is, then pursue a follow-up patch to e.g. add a [NonNullable] attribute or something, in lieu of full Nullable support?

Follow-up patch seems a lot safer.

[Joshua Bell]

FWIW, it looks like we only have about 34 non-IndexedDB uses of overloads in IDL which wouldn't be too bad to inspect/fix manually.

[Kentaro Hara]

Comment on attachment 150001 [details]
Patch

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

Looks OK, but I'd like to confirm one thing (again).

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1263
> -            push(@andExpression, "(${value}.isNull() || (${value}.isObject() && asObject(${value})->inherits(&JSArray::s_info)))");
> +            push(@andExpression, "(${value}.isObject() && isJSArray(${value}))");

As we discussed before, this will change behavior of float[] etc too. Is it expected?

[Joshua Bell]

(In reply to comment #20)
> (From update of attachment 150001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150001&action=review
> 
> Looks OK, but I'd like to confirm one thing (again).
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1263
> > -            push(@andExpression, "(${value}.isNull() || (${value}.isObject() && asObject(${value})->inherits(&JSArray::s_info)))");
> > +            push(@andExpression, "(${value}.isObject() && isJSArray(${value}))");
> 
> As we discussed before, this will change behavior of float[] etc too. Is it expected?

Whoops, meant to mark this as dependent. 

We probably would want to resolve http://webkit.org/b/90218 first, and then apply a similar qualifier to this case - i.e. if there are places that currently have an IDL overload with float[] we would mark it as [Nullable] float[] (or whatever approach we decide on for the other bug).

[Kentaro Hara]

(In reply to comment #21)
> We probably would want to resolve http://webkit.org/b/90218 first, and then apply a similar qualifier to this case - i.e. if there are places that currently have an IDL overload with float[] we would mark it as [Nullable] float[]

I am a bit confused... It seems that you are anyway going to change the behavior of the following case:

  void func(float[] array);
  void func(float f);

Before the patch, null will match 'array'. After the patch, null will match 'f'. To avoid the situation, you are trying to change the IDL to the following:

  void func(float[]? array);  // Will be accomplished by [Nullable]
  void func(float f);

In other words, it seems that you are trying to keep the current behavior by changing IDLs, which might not make sense. In my understanding, the spec requires that null should match 'array' for both float[] or float[]?.

[Joshua Bell]

(In reply to comment #22)
> In my understanding, the spec requires that null should match 'array' for both float[] or float[]?.

My interpretation of the spec is that null should *not* match float[], only float[]?

The spec is somewhat opaque. Other opinions? Discuss with Cameron?

[Kentaro Hara]

(In reply to comment #23)
> The spec is somewhat opaque. Other opinions? Discuss with Cameron?

Yeah, it seems better to ask experts before starting implementation.

[WebKit Review Bot]

Comment on attachment 150001 [details]
Patch

Attachment 150001 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13108255

New failing tests:
platform/chromium/compositing/layout-width-change.html
platform/chromium/compositing/render-surface-alpha-blending.html
platform/chromium/compositing/tiny-layer-rotated.html
platform/chromium/compositing/huge-layer-rotated.html
fast/loader/loadInProgress.html
platform/chromium/compositing/3d-corners.html
platform/chromium/compositing/video-frame-size-change.html
platform/chromium/compositing/perpendicular-layer-sorting.html
platform/chromium/compositing/plugins/webplugin-no-alpha.html
platform/chromium/compositing/child-layer-3d-sorting.html
platform/chromium/compositing/lost-compositor-context-permanently.html
platform/chromium/compositing/filters/background-filter-blur-outsets.html
platform/chromium/compositing/lost-compositor-context-with-video.html
platform/chromium/compositing/accelerated-drawing/alpha.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
platform/chromium/compositing/plugins/webplugin-alpha.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/backface-visibility-transformed.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html
fast/loader/unload-form-post-about-blank.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/img-layer-grow.html
platform/chromium/compositing/filters/background-filter-blur-off-axis.html
platform/chromium/compositing/filters/background-filter-blur.html
platform/chromium/compositing/lost-compositor-context-twice.html

[WebKit Review Bot]

Created attachment 150058 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Joshua Bell]

(In reply to comment #24)
> (In reply to comment #23)
> > The spec is somewhat opaque. Other opinions? Discuss with Cameron?
> 
> Yeah, it seems better to ask experts before starting implementation.

FYI, kicked off a thread:

http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/1373.html

[Joshua Bell]

... and response:

http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/1375.html

Which appears to support my interpretation (although we're certainly not implementing that algorithm in its entirety yet).

Further WebKit-centric thoughts? Bring this up on webkit-dev?

My proposal would be: 
(1) Proceed with https://bugs.webkit.org/show_bug.cgi?id=90218 - introduce [Nullable] for wrapper types, marking all existing IDLs that require it so there's no actual change to the generated files
(2) Modify the patch in this bug so that arrays can be marked with [Nullable] as well, similarly mark all existing IDLs that require it.
(3) Drop [Nullable] from the specific IndexedDB overloads that started all of this.

[Erik Arvidsson]

That's sounds like a good plan. One small comment here. We should just add support for ? instead of adding [Nullable]. We want WebKitIDL to get closer to WebIDL if possible.

[Joshua Bell]

(In reply to comment #29)
> That's sounds like a good plan. One small comment here. We should just add support for ? instead of adding [Nullable]. We want WebKitIDL to get closer to WebIDL if possible.

My only hesitation is that I don't think my proposed changes are a comprehensive 
implementation of Nullable. If we use a [Nullable] attribute there's a lovely process and place to document the current limitations, like we do for [Optional].

Otherwise, adding it to the parser shouldn't be a huge chore (he says foolishly).

[Erik Arvidsson]

(In reply to comment #30)
> (In reply to comment #29)
> > That's sounds like a good plan. One small comment here. We should just add support for ? instead of adding [Nullable]. We want WebKitIDL to get closer to WebIDL if possible.
> 
> My only hesitation is that I don't think my proposed changes are a comprehensive 
> implementation of Nullable. If we use a [Nullable] attribute there's a lovely process and place to document the current limitations, like we do for [Optional].

I see. I would generally want us to match WebIDL even though we have known bugs. I don't expect using [Nullable] instead of ? makes it clearer that it is not fully supported.
 
> Otherwise, adding it to the parser shouldn't be a huge chore (he says foolishly).

Yeah, fixing the parser is easy :-)

[Joshua Bell]

Created attachment 150635 [details]
Patch

[Joshua Bell]

Latest patch adds Nullable (?) support for T[] building on what was done over in http://webkit.org/b/90218 and as in that bug I did a before/after checks to ensure the generated bindings were identical. After tagging the IDLs with the "?" suffix where necessary, the only deviations were in the JSC bindings where the "is this an array?" test changed.

Regarding the latter, this only affected IDBObjectStore and IDBIndex (which are not enabled in any ports that use JSC yet) and NavigatorVibration which I believe is exposed in some ports (?) as navigator.webkitVibration() (fast/dom/navigator-vibration.html is a test). The change would be noticeable as a behavior difference in this scenario:

function Derived() {}
Derived.prototype = [];
var d = new Derived;
assert(d instanceof Array);
assert(Object.prototype.toString.call([]) === '[object Array]');
assert(Object.prototype.toString.call(d) !== '[object Array]');
navigator.webkitVibration(d); // Will dispatch to the overload that takes unsigned long rather than unsigned long[] via toUInt32()

This behavior should already be the case in V8. I can add a test for that somewhere, if there is a JSC port that exposes this API and it's considered worthwhile. (CC: kihong.kwon@)

I removed the IndexedDB behavior changes, will land those separately.

[Kentaro Hara]

Comment on attachment 150635 [details]
Patch

Looks OK.

> This behavior should already be the case in V8. I can add a test for that somewhere, if there is a JSC port that exposes this API and it's considered worthwhile. (CC: kihong.kwon@)

Let's do it in a follow-up patch if necessary.

[WebKit Review Bot]

Comment on attachment 150635 [details]
Patch

Clearing flags on attachment: 150635

Committed r121817: <http://trac.webkit.org/changeset/121817>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.