Bug 85326 – WebIDL: overloaded methods prevent number -> string conversion

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Alec Flett]

(From e-mail sent to kentaro@ and abarth@)

Here's the situation:
We have an API that previously took a number as it's 2nd parameter, and we'd like to change it to accept a string, as per the latest IndexedDB spec.

So I changed this:
        IDBTransaction transaction(in DOMStringList storeNames, in unsigned short mode)
            raises (IDBDatabaseException);
        IDBTransaction transaction(in DOMString[] storeNames, in unsigned short mode)
            raises (IDBDatabaseException);
        IDBTransaction transaction(in DOMString storeName, in unsigned short mode)
            raises (IDBDatabaseException);
to this:

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

Figuring I'd still catch the numeric values, that they'd be converted to strings... i.e. that if someone passed in 0, that I'd get the string "0".

But it looks like the IDL compiler is generating code to pick the right callback that checks arg[1]->IsObject(), and when someone passes in a number, it's not an object so none of the callbacks are called:
    if ((args.Length() == 2 && (args[0]->IsNull() || V8DOMStringList::HasInstance(args[0])) && (args[1]->IsNull() || args[1]->IsUndefined() || args[1]->IsString() || args[1]->IsObject())))
        return transaction1Callback(args);
    if ((args.Length() == 2 && (args[0]->IsNull() || args[0]->IsArray()) && (args[1]->IsNull() || args[1]->IsUndefined() || args[1]->IsString() || args[1]->IsObject())))
        return transaction2Callback(args);
    if ((args.Length() == 2 && (args[0]->IsNull() || args[0]->IsUndefined() || args[0]->IsString() || args[0]->IsObject()) && (args[1]->IsNull() || args[1]->IsUndefined() || args[1]->IsString() || args[1]->IsObject())))
        return transaction3Callback(args);

Oddly, we made a very similar change to a nearby function, (setVersion) changing an unsigned short to a string, but there is no overload... so the generated code calls     STRING_TO_V8PARAMETER_EXCEPTION_BLOCK without any checks.

So this means that the conversion from number to string works for non-overloaded functions, but not for overloaded functions. 

it seems like for this very specific case, we could simply add IsNumber() as another check in the overload checks, (around line 1288 in CodeGeneratorV8) but I don't know what the repercussions would be.

[Alec Flett]

[leaving unassigned because I'm pretty new to WebKit and not quite ready to dive into both JavaScriptCore *and* V8, but maybe in a few weeks when I'm more up to speed]

[Joshua Bell]

In lieu of implementing http://dev.w3.org/2006/webapi/WebIDL/#dfn-overload-resolution-algorithm we could:

* Enable StrictTypeChecking as a parameter attribute
* Mark existing overload DOMString parameters with [StrictTypeChecking]
* Change (using V8 as example) GenerateParametersCheckExpression:

old:
        if ($codeGenerator->IsStringType($type)) {
            push(@andExpression, "(${value}->IsNull() || ${value}->IsUndefined() || ${value}->IsString() || ${value}->IsObject())");

new:
        if ($codeGenerator->IsStringType($type)) {
            if ($parameter->extendedAttributes->{"StrictTypeChecking"}) {
                push(@andExpression, "(${value}->IsNull() || ${value}->IsUndefined() || ${value}->IsString() || ${value}->IsObject())");

This would get us the "anything can coerce to DOMString" behavior that non-overloaded methods have. This isn't urgent - we work around this in IndexedDB by having duplicate overloads for e.g. numbers and strings; it just multiplies the number of overloads in the IDL and impl which is a maintenance issue.

[Joshua Bell]

(In reply to comment #2)
> * Enable StrictTypeChecking as a parameter attribute
> * Mark existing overload DOMString parameters with [StrictTypeChecking]

This may be necessary even if the full overloading algorithm from the spec is implemented, as there are legacy interfaces that are ambiguous overloads in WebIDL, e.g.:

./html/canvas/CanvasRenderingContext2D.idl:
 void setStrokeColor(DOMString color, float alpha)
 void setStrokeColor(float grayLevel, float alpha)

./html/canvas/CanvasRenderingContext2D.idl:
 void setFillColor(DOMString color, float alpha)
 void setFillColor(float grayLevel, float alpha)

./html/canvas/CanvasRenderingContext2D.idl:
 void setShadow(float width, float height, float blur, DOMString color, float alpha)
 void setShadow(float width, float height, float blur, float grayLevel, float alpha)

... and possibly:
 ./xml/XMLHttpRequest.idl: void send(...)

Otherwise, it looks like all the overloads in WebCore IDLs involving DOMString can be disambiguated based on earlier arguments, or are intentional to work around this issue.

[Ian 'Hixie' Hickson]

Is it possible to just drop those? They're not specced and I don't believe anyone else implements them...

[Joshua Bell]

(In reply to comment #4)
> Is it possible to just drop those? They're not specced and I don't believe anyone else implements them...

I tried digging through the history to see where these came from, but wasn't able to go back far enough. They certainly look legacy, but did get documented at least here: https://developer.mozilla.org/en-US/docs/DOM/CanvasRenderingContext2D#WebKit-specific_methods

Definitely seems worthwhile to try and get usage numbers for them. But that's getting off topic...

[Joshua Bell]

Created attachment 167870 [details]
Patch

[Joshua Bell]

Plausible patch. Not high priority, but we were failing a W3C test case because of this:

http://w3c-test.org/webapps/IndexedDB/tests/submissions/Opera/list_ordering.htm

[Kentaro Hara]

Comment on attachment 167870 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Enable legacy behavior of only matching undefined/null/string/object(and not number,
> +        boolean, etc) via the StrictTypeChecking attribute.

Ideally we want to kill the legacy behavior, but it looks reasonable to land this patch first.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:303
> +    // FIXME: Remove legacy constants. http://webkit.org/b/85315

Nit: Also let's add "this is not thread-safe".

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:344
> +    // FIXME: Remove legacy constants. http://webkit.org/b/85315

Ditto.

[Build Bot]

Comment on attachment 167870 [details]
Patch

Attachment 167870 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14252007

[Joshua Bell]

Created attachment 168018 [details]
Patch

[Joshua Bell]

Comment on attachment 168018 [details]
Patch

Fail was due to misplacing the unused arguments check in the JSC code generator. Re-uploaded, will wait for the bots to report green.

[WebKit Review Bot]

Comment on attachment 168018 [details]
Patch

Clearing flags on attachment: 168018

Committed r131063: <http://trac.webkit.org/changeset/131063>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.