Bug 89710 – CodeGenerator should throw a TypeError for missing non-optional Dictionary parameters

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Adam Klein]

CodeGenerator should throw a TypeError for missing non-optional Dictionary parameters

[Adam Klein]

Created attachment 148915 [details]
Patch

[Kentaro Hara]

Comment on attachment 148915 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2592
> +                my @typeCheck = ("!$name.isObject()");
> +                unshift(@typeCheck, "!$name.isUndefinedOrNull()") if $optional;
> +                push(@$outputArray, "    if (" . join(" && ", @typeCheck) . ")\n");

- The condition should be 'if (name.isUndefinedOrNull() || !name.isObject()) {  throw(); }', shouldn't it?

- I guess we do not need to change the condition depending on whether $optional or not. [Optional] just controls how a missing parameter is converted by the MAYBE_MISSING_PARAMETER() macro (https://trac.webkit.org/wiki/WebKitIDL#Optional). Maybe we can always use 'if (name.isUndefinedOrNull() || !name.isObject())'.

BTW, do we need to check both isUndefinedOrNull() and isObject()? If isObject() returns false for an undefined or null value, we can omit the isUndefinedOrNull() check (I am not sure though).

[Jon Lee]

Comment on attachment 148915 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2592
>> +                push(@$outputArray, "    if (" . join(" && ", @typeCheck) . ")\n");
> 
> - The condition should be 'if (name.isUndefinedOrNull() || !name.isObject()) {  throw(); }', shouldn't it?
> 
> - I guess we do not need to change the condition depending on whether $optional or not. [Optional] just controls how a missing parameter is converted by the MAYBE_MISSING_PARAMETER() macro (https://trac.webkit.org/wiki/WebKitIDL#Optional). Maybe we can always use 'if (name.isUndefinedOrNull() || !name.isObject())'.
> 
> BTW, do we need to check both isUndefinedOrNull() and isObject()? If isObject() returns false for an undefined or null value, we can omit the isUndefinedOrNull() check (I am not sure though).

I don't think the $optional check is necessary. Kentaro's right.

And isObject() is enough. isObject() and isUndefinedOrNull() check the exact same attribute, except one uses !, so as it stands now the logic will always be false.

[Adam Klein]

(In reply to comment #3)
> (From update of attachment 148915 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148915&action=review
> 
> >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2592
> >> +                push(@$outputArray, "    if (" . join(" && ", @typeCheck) . ")\n");
> > 
> > - The condition should be 'if (name.isUndefinedOrNull() || !name.isObject()) {  throw(); }', shouldn't it?
> > 
> > - I guess we do not need to change the condition depending on whether $optional or not. [Optional] just controls how a missing parameter is converted by the MAYBE_MISSING_PARAMETER() macro (https://trac.webkit.org/wiki/WebKitIDL#Optional). Maybe we can always use 'if (name.isUndefinedOrNull() || !name.isObject())'.
> > 
> > BTW, do we need to check both isUndefinedOrNull() and isObject()? If isObject() returns false for an undefined or null value, we can omit the isUndefinedOrNull() check (I am not sure though).
> 
> I don't think the $optional check is necessary. Kentaro's right.
> 
> And isObject() is enough. isObject() and isUndefinedOrNull() check the exact same attribute, except one uses !, so as it stands now the logic will always be false.

It's clear that there's some confusion here, perhaps my ChangeLog description was confusing?

Here are the cases I'm concerned with (I'll use real examples):

1. WebKitMutationObserver.observe. Its second argument is a Dictionary, which is not optional. If you fail to pass a second argument, or if you pass anything that is not an argument, we want to throw a TypeError. Indeed, for this case, !isObject() is sufficient, and that is the only code emitted.

2. Notification's constructor. Its second argument is an [Optional=DefaultIsUndefined] Dictionary. If you fail to pass a second argument, MAYBE_MISSING_PARAMETER yields undefined, which is then passed to Dictionary's constructor. We don't want to throw in this case, thus the need to check that !options.isUndefinedOrNull().

3. In addition, it seems (from various test cases that fail if I leave out the !options.isUndefinedOrNull(), such as those for IDBDatabase.createObjectStore()) that we want to support explicitly passing undefined or null in place of the optional argument. This is useful, for example, when building a framework on top of such an API, where the framework layer may itself take an optional argument and always pass it on to the underlying DOM API. This is why my change to the code generators adds the "!options.isUndefinedOrNull() " check to the if statement when $optional is true.

It seems we are all in agreement with (1). I assume we are also in agreement with (2), since this was the behavior of the code before this change and all tests passed. So perhaps all this confusion is around (3)?

[Kentaro Hara]

(In reply to comment #4)
> 1. WebKitMutationObserver.observe. Its second argument is a Dictionary, which is not optional. If you fail to pass a second argument, or if you pass anything that is not an argument, we want to throw a TypeError. Indeed, for this case, !isObject() is sufficient, and that is the only code emitted.
> 
> 2. Notification's constructor. Its second argument is an [Optional=DefaultIsUndefined] Dictionary. If you fail to pass a second argument, MAYBE_MISSING_PARAMETER yields undefined, which is then passed to Dictionary's constructor. We don't want to throw in this case, thus the need to check that !options.isUndefinedOrNull().
> 
> 3. In addition, it seems (from various test cases that fail if I leave out the !options.isUndefinedOrNull(), such as those for IDBDatabase.createObjectStore()) that we want to support explicitly passing undefined or null in place of the optional argument. This is useful, for example, when building a framework on top of such an API, where the framework layer may itself take an optional argument and always pass it on to the underlying DOM API. This is why my change to the code generators adds the "!options.isUndefinedOrNull() " check to the if statement when $optional is true.
> 
> It seems we are all in agreement with (1). I assume we are also in agreement with (2), since this was the behavior of the code before this change and all tests passed. So perhaps all this confusion is around (3)?

Thank you very much for the clarification. I agree with (1) and (2).

Regarding (3), what tests fail if we always check !options.isUndefinedOrNull()? (It still sounds a bit strange to switch the condition depending on whether $optional or not.)

[Adam Klein]

(In reply to comment #5)
> (In reply to comment #4)
> > 3. In addition, it seems (from various test cases that fail if I leave out the !options.isUndefinedOrNull(), such as those for IDBDatabase.createObjectStore()) that we want to support explicitly passing undefined or null in place of the optional argument. This is useful, for example, when building a framework on top of such an API, where the framework layer may itself take an optional argument and always pass it on to the underlying DOM API. This is why my change to the code generators adds the "!options.isUndefinedOrNull() " check to the if statement when $optional is true.
> 
> Regarding (3), what tests fail if we always check !options.isUndefinedOrNull()? (It still sounds a bit strange to switch the condition depending on whether $optional or not.)

Any test in storage/indexeddb/* that calls IDBDatabase.createObjectStore(..., null). I count at least 19 of these. I'm willing to buy the idea that the IndexedDB code is wrong (especially since the spec says that the second argument to createObjectStore is optional but _not_ nullable). But I'm not clear on what we should do if that is the case. Write custom code to appease any failing tests? Or update our behavior as part of cleaning up the CodeGenerator code?

[Kentaro Hara]

(In reply to comment #6)
> Any test in storage/indexeddb/* that calls IDBDatabase.createObjectStore(..., null). I count at least 19 of these. I'm willing to buy the idea that the IndexedDB code is wrong (especially since the spec says that the second argument to createObjectStore is optional but _not_ nullable). But I'm not clear on what we should do if that is the case. Write custom code to appease any failing tests? Or update our behavior as part of cleaning up the CodeGenerator code?

jsbell: any idea?

[Adam Klein]

Besides the IDB stuff, I think there's one other test that fails: fast/mediastream/peerconnection-iceoptions.html. It's testing exactly this behavior, and was added in http://trac.webkit.org/changeset/116127. tommyw, can you comment on whether this behavior is correct?

[Adam Klein]

Hmm, for PeerConnection00, this seems to be the relevant spec text: http://tools.ietf.org/html/draft-uberti-rtcweb-jsep-02#section-6.1.10, which is pretty vague ("A TBD exception will be thrown if |options| is malformed"). If jsbell is ok with it, I'm inclined to distinguish between null and undefined here.

[Adam Barth]

PeerConnection00 is stil very much an experimental API.  I would be inclined to align it with broader platform conventions.

[Adam Klein]

Here's some relevant WebIDL text: http://www.w3.org/TR/WebIDL/#TreatUndefinedAs (see [TreatUndefinedAs=Missing]. That's half of the behavior I discussed in (3). But it seems this is not recommended, so I'd expect a recent spec like IDB shouldn't use it. There's no TreatUndefinedAs=Null at all, fwiw.

[Tommy Widenflycht]

+1 to that.

(In reply to comment #10)
> PeerConnection00 is stil very much an experimental API.  I would be inclined to align it with broader platform conventions.

[Tommy Widenflycht]

FYI malformed in this case means "doesn't contain the correct keys or not an object"

(In reply to comment #9)
> Hmm, for PeerConnection00, this seems to be the relevant spec text: http://tools.ietf.org/html/draft-uberti-rtcweb-jsep-02#section-6.1.10, which is pretty vague ("A TBD exception will be thrown if |options| is malformed"). If jsbell is ok with it, I'm inclined to distinguish between null and undefined here.

[Joshua Bell]

Re: IndexedDB - it looks like Firefox (13) allows both null and undefined w/o raising exceptions. I'll follow up on the spec list to see if we want to make that nullable in the spec.

[Joshua Bell]

(In reply to comment #14)
> Re: IndexedDB - it looks like Firefox (13) allows both null and undefined w/o raising exceptions. I'll follow up on the spec list to see if we want to make that nullable in the spec.

And I suggested that the Web IDL for these arguments in IDB probably needs to be something like:

[TreatUndefinedAs=Null] optional IDBObjectStoreParameters? optionalParameters

So short version: I suspect we will want to keep the current behavior, but changing the WebKit IDL incantations to do so is fine. (If they match Web IDL incantations, even better.)

[Joshua Bell]

(In reply to comment #15)
> (In reply to comment #14)
> > Re: IndexedDB - it looks like Firefox (13) allows both null and undefined w/o raising exceptions. I'll follow up on the spec list to see if we want to make that nullable in the spec.

And, it turns out, the WebIDL spec updated basically while I was typing this.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=16725
http://dev.w3.org/2006/webapi/WebIDL/#es-dictionary

The ECMAScript -> Dictionary logic now starts with:

1. If Type(V) is not Undefined, Null or Object, then throw a TypeError.

... which lets null/undefined "fall through" into the steps for creating an empty dict and starting to populate it, which will be a no-op.

[Adam Klein]

Now that the spec matches the existing code, this patch has no reason for being.