Bug 85326 - WebIDL: overloaded methods prevent number -> string conversion
Summary: WebIDL: overloaded methods prevent number -> string conversion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-01 17:28 PDT by Alec Flett
Modified: 2012-10-11 08:03 PDT (History)
8 users (show)

See Also:


Attachments
Patch (48.73 KB, patch)
2012-10-09 16:07 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (48.95 KB, patch)
2012-10-10 09:47 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Flett 2012-05-01 17:28:07 PDT
(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.
Comment 1 Alec Flett 2012-05-01 17:28:50 PDT
[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]
Comment 2 Joshua Bell 2012-07-19 12:02:59 PDT
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.
Comment 3 Joshua Bell 2012-09-24 12:23:45 PDT
(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.
Comment 4 Ian 'Hixie' Hickson 2012-09-24 12:54:56 PDT
Is it possible to just drop those? They're not specced and I don't believe anyone else implements them...
Comment 5 Joshua Bell 2012-10-09 15:05:33 PDT
(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...
Comment 6 Joshua Bell 2012-10-09 16:07:25 PDT
Created attachment 167870 [details]
Patch
Comment 7 Joshua Bell 2012-10-09 16:08:13 PDT
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
Comment 8 Kentaro Hara 2012-10-09 16:56:14 PDT
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.
Comment 9 Build Bot 2012-10-09 18:49:14 PDT
Comment on attachment 167870 [details]
Patch

Attachment 167870 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14252007
Comment 10 Joshua Bell 2012-10-10 09:47:12 PDT
Created attachment 168018 [details]
Patch
Comment 11 Joshua Bell 2012-10-10 09:48:13 PDT
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.
Comment 12 WebKit Review Bot 2012-10-11 08:03:43 PDT
Comment on attachment 168018 [details]
Patch

Clearing flags on attachment: 168018

Committed r131063: <http://trac.webkit.org/changeset/131063>
Comment 13 WebKit Review Bot 2012-10-11 08:03:48 PDT
All reviewed patches have been landed.  Closing bug.