Bug 160091 – Parameters to Node.replaceChild() / insertBefore() should be mandatory

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Parameters to Node.replaceChild() / insertBefore() should be mandatory:
- https://dom.spec.whatwg.org/#node

The compatibility risk should be low since Firefox and Chrome both agree with the specification and because it does not make much sense to omit parameters when using this API.

[Chris Dumez]

Created attachment 284350 [details]
Patch

[Darin Adler]

Comment on attachment 284350 [details]
Patch

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

> Source/WebCore/bindings/js/JSNodeCustom.cpp:118
> +    if (UNLIKELY(state.argumentCount() < 2))
> +        return state.vm().throwException(&state, createNotEnoughArgumentsError(&state));

Test cases don’t seem to express the difference between checking argument count and checking for undefined. Are you sure that this should be checking for argument count and not undefined?

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 284350 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284350&action=review
> 
> > Source/WebCore/bindings/js/JSNodeCustom.cpp:118
> > +    if (UNLIKELY(state.argumentCount() < 2))
> > +        return state.vm().throwException(&state, createNotEnoughArgumentsError(&state));
> 
> Test cases don’t seem to express the difference between checking argument
> count and checking for undefined. Are you sure that this should be checking
> for argument count and not undefined?

1. The parameters are mandatory so checking for argumentCount() is the right thing to do. We should throw a TypeError and complain about the number of parameters being insufficient if not enough parameters are passed.

2. If the JS passes undefined, then it is another matter. If the parameter is a non-nullable Node, then we should throw a TypeError and complain about the parameter type being invalid. If the parameter is a nullable node, then undefined should be treated as null.

I am adding the argument check for 1.
For 2., the existing calls to JSNode::toWrapped() followed by null checks should be sufficient.

[Darin Adler]

Comment on attachment 284350 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSNodeCustom.cpp:118
>>> +        return state.vm().throwException(&state, createNotEnoughArgumentsError(&state));
>> 
>> Test cases don’t seem to express the difference between checking argument count and checking for undefined. Are you sure that this should be checking for argument count and not undefined?
> 
> 1. The parameters are mandatory so checking for argumentCount() is the right thing to do. We should throw a TypeError and complain about the number of parameters being insufficient if not enough parameters are passed.
> 
> 2. If the JS passes undefined, then it is another matter. If the parameter is a non-nullable Node, then we should throw a TypeError and complain about the parameter type being invalid. If the parameter is a nullable node, then undefined should be treated as null.
> 
> I am adding the argument check for 1.
> For 2., the existing calls to JSNode::toWrapped() followed by null checks should be sufficient.

I’m surprised that this is what mandatory parameters mean in the latest specifications.

I was under the impression that in the modern specifications, undefined values for parameters and omitted trailing parameters were treated identically, to match more closely the behavior of functions within the JavaScript specification itself and the flavor of the JavaScript language. Could you point me to the wording the latest draft of WebIDL’s JavaScript bindings section that makes this clear?

[Chris Dumez]

(In reply to comment #4)
> Comment on attachment 284350 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284350&action=review
> 
> >>> Source/WebCore/bindings/js/JSNodeCustom.cpp:118
> >>> +        return state.vm().throwException(&state, createNotEnoughArgumentsError(&state));
> >> 
> >> Test cases don’t seem to express the difference between checking argument count and checking for undefined. Are you sure that this should be checking for argument count and not undefined?
> > 
> > 1. The parameters are mandatory so checking for argumentCount() is the right thing to do. We should throw a TypeError and complain about the number of parameters being insufficient if not enough parameters are passed.
> > 
> > 2. If the JS passes undefined, then it is another matter. If the parameter is a non-nullable Node, then we should throw a TypeError and complain about the parameter type being invalid. If the parameter is a nullable node, then undefined should be treated as null.
> > 
> > I am adding the argument check for 1.
> > For 2., the existing calls to JSNode::toWrapped() followed by null checks should be sufficient.
> 
> I’m surprised that this is what mandatory parameters mean in the latest
> specifications.
> 
> I was under the impression that in the modern specifications, undefined
> values for parameters and omitted trailing parameters were treated
> identically, to match more closely the behavior of functions within the
> JavaScript specification itself and the flavor of the JavaScript language.
> Could you point me to the wording the latest draft of WebIDL’s JavaScript
> bindings section that makes this clear?

undefined and omitted parameters are usually treated identically yes, but only for *optional* parameters. Let me look up the spec.

[Chris Dumez]

So, the spec would be:
-> http://heycam.github.io/webidl/#es-operations

which points to the overload resolution algorithm:
-> http://heycam.github.io/webidl/#es-overloads

If we consider this operation:
Node replaceChild(Node node, Node child);

And if it is called like so:
n.replaceChild(undefined, undefined);

1. maxArg is 2
2. argcount is 2
3. Does not remove anything from S because the entry in S and length 2 and argCount is 2
4. We do not throw the TypeError related to insufficient arguments because S is not empty.

[Build Bot]

Comment on attachment 284350 [details]
Patch

Attachment 284350 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1735104

New failing tests:
fast/dom/move-nodes-across-documents.html

[Build Bot]

Created attachment 284357 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Build Bot]

Comment on attachment 284350 [details]
Patch

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

New failing tests:
fast/dom/move-nodes-across-documents.html

[Build Bot]

Created attachment 284358 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Chris Dumez]

(In reply to comment #6)
> So, the spec would be:
> -> http://heycam.github.io/webidl/#es-operations
> 
> which points to the overload resolution algorithm:
> -> http://heycam.github.io/webidl/#es-overloads
> 
> If we consider this operation:
> Node replaceChild(Node node, Node child);
> 
> And if it is called like so:
> n.replaceChild(undefined, undefined);
> 
> 1. maxArg is 2
> 2. argcount is 2
> 3. Does not remove anything from S because the entry in S and length 2 and
> argCount is 2
> 4. We do not throw the TypeError related to insufficient arguments because S
> is not empty.

Also note that the coded I used is what our bindings generator would have generated:
sub GenerateArgumentsCountCheck
{
    my $outputArray = shift;
    my $function = shift;
    my $interface = shift;

    my $numMandatoryParams = @{$function->parameters};
    foreach my $param (reverse(@{$function->parameters})) {
        if ($param->isOptional or $param->isVariadic) {
            $numMandatoryParams--;
        } else {
            last;
        }
    }
    if ($numMandatoryParams >= 1)
    {
        push(@$outputArray, "    if (UNLIKELY(state->argumentCount() < $numMandatoryParams))\n");
        push(@$outputArray, "        return throwVMError(state, createNotEnoughArgumentsError(state));\n");
    }
}

[Chris Dumez]

Created attachment 284359 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284359 [details]
Patch

Clearing flags on attachment: 284359

Committed r203610: <http://trac.webkit.org/changeset/203610>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.