Binding: IDL type DOMString[] shouldn't match null
Created attachment 137641 [details] Patch
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,
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).
(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.
CC-ing experts.
+vineet who's apparently working on sequence<T>
Comment on attachment 137641 [details] Patch Seems reasonable.
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?
Thank you arv.
(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.
> > 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?
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.
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)
(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?
Created attachment 150001 [details] Patch
(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?
(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. :)
(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.
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 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?
(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).
(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[]?.
(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?
(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 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
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
(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
... 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.
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.
(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).
(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 :-)
Created attachment 150635 [details] Patch
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 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 on attachment 150635 [details] Patch Clearing flags on attachment: 150635 Committed r121817: <http://trac.webkit.org/changeset/121817>
All reviewed patches have been landed. Closing bug.