WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
84217
Binding: IDL type DOMString[] shouldn't match null
https://bugs.webkit.org/show_bug.cgi?id=84217
Summary
Binding: IDL type DOMString[] shouldn't match null
Joshua Bell
Reported
2012-04-17 17:33:47 PDT
Binding: IDL type DOMString[] shouldn't match null
Attachments
Patch
(8.85 KB, patch)
2012-04-17 17:37 PDT
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Simple test case for Firefox
(1.10 KB, text/html)
2012-06-28 11:50 PDT
,
Joshua Bell
no flags
Details
Patch
(10.06 KB, patch)
2012-06-28 13:46 PDT
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-04
(733.45 KB, application/zip)
2012-06-28 18:21 PDT
,
WebKit Review Bot
no flags
Details
Patch
(12.71 KB, patch)
2012-07-03 10:00 PDT
,
Joshua Bell
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Joshua Bell
Comment 1
2012-04-17 17:37:56 PDT
Created
attachment 137641
[details]
Patch
Joshua Bell
Comment 2
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,
Kentaro Hara
Comment 3
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).
Joshua Bell
Comment 4
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.
Kentaro Hara
Comment 5
2012-04-18 13:49:38 PDT
CC-ing experts.
Joshua Bell
Comment 6
2012-04-18 13:54:25 PDT
+vineet who's apparently working on sequence<T>
Eric Seidel (no email)
Comment 7
2012-04-19 16:21:37 PDT
Comment on
attachment 137641
[details]
Patch Seems reasonable.
Erik Arvidsson
Comment 8
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?
Eric Seidel (no email)
Comment 9
2012-04-19 16:35:02 PDT
Thank you arv.
Erik Arvidsson
Comment 10
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.
Joshua Bell
Comment 11
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?
Joshua Bell
Comment 12
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.
Joshua Bell
Comment 13
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)
Erik Arvidsson
Comment 14
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?
Joshua Bell
Comment 15
2012-06-28 13:46:03 PDT
Created
attachment 150001
[details]
Patch
Joshua Bell
Comment 16
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?
Joshua Bell
Comment 17
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. :)
Erik Arvidsson
Comment 18
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.
Joshua Bell
Comment 19
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.
Kentaro Hara
Comment 20
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?
Joshua Bell
Comment 21
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).
Kentaro Hara
Comment 22
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[]?.
Joshua Bell
Comment 23
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?
Kentaro Hara
Comment 24
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.
WebKit Review Bot
Comment 25
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
WebKit Review Bot
Comment 26
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
Joshua Bell
Comment 27
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
Joshua Bell
Comment 28
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.
Erik Arvidsson
Comment 29
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.
Joshua Bell
Comment 30
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).
Erik Arvidsson
Comment 31
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 :-)
Joshua Bell
Comment 32
2012-07-03 10:00:15 PDT
Created
attachment 150635
[details]
Patch
Joshua Bell
Comment 33
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.
Kentaro Hara
Comment 34
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.
WebKit Review Bot
Comment 35
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
>
WebKit Review Bot
Comment 36
2012-07-03 17:24:01 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug