Bug 84217

Summary: Binding: IDL type DOMString[] shouldn't match null
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: New BugsAssignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, arv, code.vineet, dglazkov, eric, haraken, japhet, jochen, kihong.kwon, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 90218    
Bug Blocks: 90474    
Attachments:
Description Flags
Patch
none
Simple test case for Firefox
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-04
none
Patch none

Description Joshua Bell 2012-04-17 17:33:47 PDT
Binding: IDL type DOMString[] shouldn't match null
Comment 1 Joshua Bell 2012-04-17 17:37:56 PDT
Created attachment 137641 [details]
Patch
Comment 2 Joshua Bell 2012-04-17 17:42:54 PDT
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,
Comment 3 Kentaro Hara 2012-04-17 17:56:28 PDT
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).
Comment 4 Joshua Bell 2012-04-18 13:45:38 PDT
(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.
Comment 5 Kentaro Hara 2012-04-18 13:49:38 PDT
CC-ing experts.
Comment 6 Joshua Bell 2012-04-18 13:54:25 PDT
+vineet who's apparently working on sequence<T>
Comment 7 Eric Seidel (no email) 2012-04-19 16:21:37 PDT
Comment on attachment 137641 [details]
Patch

Seems reasonable.
Comment 8 Erik Arvidsson 2012-04-19 16:29:41 PDT
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?
Comment 9 Eric Seidel (no email) 2012-04-19 16:35:02 PDT
Thank you arv.
Comment 10 Erik Arvidsson 2012-04-19 16:36:25 PDT
(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.
Comment 11 Joshua Bell 2012-05-25 09:48:25 PDT
> > 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?
Comment 12 Joshua Bell 2012-06-28 11:50:12 PDT
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.
Comment 13 Joshua Bell 2012-06-28 11:54:14 PDT
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)
Comment 14 Erik Arvidsson 2012-06-28 12:08:40 PDT
(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?
Comment 15 Joshua Bell 2012-06-28 13:46:03 PDT
Created attachment 150001 [details]
Patch
Comment 16 Joshua Bell 2012-06-28 13:57:17 PDT
(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?
Comment 17 Joshua Bell 2012-06-28 13:59:40 PDT
(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. :)
Comment 18 Erik Arvidsson 2012-06-28 14:00:46 PDT
(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.
Comment 19 Joshua Bell 2012-06-28 14:07:08 PDT
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.
Comment 20 Kentaro Hara 2012-06-28 16:55:22 PDT
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?
Comment 21 Joshua Bell 2012-06-28 16:58:15 PDT
(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).
Comment 22 Kentaro Hara 2012-06-28 17:13:49 PDT
(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[]?.
Comment 23 Joshua Bell 2012-06-28 17:27:46 PDT
(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?
Comment 24 Kentaro Hara 2012-06-28 17:40:54 PDT
(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.
Comment 25 WebKit Review Bot 2012-06-28 18:21:01 PDT
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
Comment 26 WebKit Review Bot 2012-06-28 18:21:06 PDT
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
Comment 27 Joshua Bell 2012-06-29 09:30:41 PDT
(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
Comment 28 Joshua Bell 2012-06-29 10:21:13 PDT
... 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.
Comment 29 Erik Arvidsson 2012-06-29 10:27:47 PDT
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.
Comment 30 Joshua Bell 2012-06-29 10:33:09 PDT
(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).
Comment 31 Erik Arvidsson 2012-06-29 10:36:11 PDT
(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 :-)
Comment 32 Joshua Bell 2012-07-03 10:00:15 PDT
Created attachment 150635 [details]
Patch
Comment 33 Joshua Bell 2012-07-03 10:15:34 PDT
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.
Comment 34 Kentaro Hara 2012-07-03 17:01:29 PDT
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.
Comment 35 WebKit Review Bot 2012-07-03 17:23:53 PDT
Comment on attachment 150635 [details]
Patch

Clearing flags on attachment: 150635

Committed r121817: <http://trac.webkit.org/changeset/121817>
Comment 36 WebKit Review Bot 2012-07-03 17:24:01 PDT
All reviewed patches have been landed.  Closing bug.