Created attachment 255925 [details] Example - Call select.add() with more arguments than needed Cannot compose a new email in Yahoo Mail when using an iPhone user agent string and WebKit r178097 or later. In particular, a JavaScript TypeError is thrown after clicking on the compose button. On a Mac, you can reproduce this issue by performing the following: 1. Open Safari with WebKit r178097 or later. 2. Open a new window. 4. Ensure the Develop menu is enabled (*) and choose Develop > User Agent > Safari iOS 8.1 - iPhone. 5. Navigate to mail.yahoo.com and sign in. 5. Click the compose button. A JavaScript TypeError is thrown and caught by Yahoo Mail's code as indicated by the presence of the following line in the Web Inspector console: "Error Reported (presentModalView): Type error" (*) You can enable the Develop menu by choosing Safari > Preferences to open the Safari preferences window. In the preferences window, click Advanced in the toolbar. Then check the checkbox "Show Develop menu in menu bar".
We throw a JavaScript TypeError when passing more arguments than required to select.add() and hence break Yahoo's compose editor. In particular, Yahoo mail calls select.add() with the following form, where S is some HTML select element: select.add(S, undefined, undefined, undefined, undefined)
<rdar://problem/21348421>
I also noticed that we prepend the (In reply to comment #1) > We throw a JavaScript TypeError when passing more arguments than required to > select.add() and hence break Yahoo's compose editor. In particular, Yahoo > mail calls select.add() with the following form, where S is some HTML select > element: > > select.add(S, undefined, undefined, undefined, undefined) *where S is some HTML option element
I also noticed that calling select.add(O, undefined) where O is some HTML option element prepends O to the list of options in WebKit r178097 or later. But it appends it to the end of the list of options in Chrome for Mac 44.0.2403.61 beta (64-bit) and Firefox for Mac 34.0.5.
Created attachment 255988 [details] [Patch] Work-in-progress Work-in-progress patch. Needs change log entries.
Comment on attachment 255988 [details] [Patch] Work-in-progress View in context: https://bugs.webkit.org/attachment.cgi?id=255988&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1456 > +sub LengthOfLongestFunctionParameterList Hmm. This will unfortunately not work when using variadic arguments: void f(DOMString a); void f(Node a, DOMString b, float... c); void f(); void f(Event a, DOMString b, optional DOMString c, float... d);
Would something like this work? diff --git a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm index e8cb245..e9e4788 100644 --- a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm +++ b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm @@ -1361,7 +1361,7 @@ sub GenerateParametersCheckExpression my $function = shift; my @andExpression = (); - push(@andExpression, "argsCount == $numParameters"); + push(@andExpression, "argsCount >= $numParameters"); my $parameterIndex = 0; my %usedArguments = (); foreach my $parameter (@{$function->parameters}) {
We don’t want to allow extra arguments in every function.
(In reply to comment #8) > We don’t want to allow extra arguments in every function. Why not? My understanding was that this is the expected behavior in Web IDL and EcmaScript. As far as I know, we already allow extra arguments for regular methods. I think we were only throwing for overloaded methods, which was inconsistent.
(In reply to comment #9) > (In reply to comment #8) > > We don’t want to allow extra arguments in every function. > > Why not? My understanding was that this is the expected behavior in Web IDL > and EcmaScript. As far as I know, we already allow extra arguments for > regular methods. > I think we were only throwing for overloaded methods, which was inconsistent. See for e.g. discussion at: http://lists.w3.org/Archives/Public/public-script-coord/2014AprJun/0187.html Also, doing document.createElement("a", "b", "c") currently doesn't throw and creates an <a> element.
Wow, I totally missed that change in 2014! I am so happy to hear it. I was pushing for this years ago, but others told me that this was OK for JavaScript itself, but not WebIDL.
(In reply to comment #7) > Would something like this work? > > diff --git a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm > b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm > index e8cb245..e9e4788 100644 > --- a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm > +++ b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm > @@ -1361,7 +1361,7 @@ sub GenerateParametersCheckExpression > my $function = shift; > > my @andExpression = (); > - push(@andExpression, "argsCount == $numParameters"); > + push(@andExpression, "argsCount >= $numParameters"); > my $parameterIndex = 0; > my %usedArguments = (); > foreach my $parameter (@{$function->parameters}) { This will not work as discussed in person today (07/22). With this change, looking at the generated code for the overload function HTMLSelectElement.add(), we have: EncodedJSValue JSC_HOST_CALL jsHTMLSelectElementPrototypeFunctionAdd(ExecState* exec) { size_t argsCount = exec->argumentCount(); JSValue arg0(exec->argument(0)); JSValue arg1(exec->argument(1)); if ((argsCount >= 1 && (arg0.isObject() && asObject(arg0)->inherits(JSHTMLElement::info()))) || (argsCount >= 2 && (arg0.isObject() && asObject(arg0)->inherits(JSHTMLElement::info())) && (arg1.isNull() || (arg1.isObject() && asObject(arg1)->inherits(JSHTMLElement::info()))))) return jsHTMLSelectElementPrototypeFunctionAdd1(exec); if ((argsCount >= 1 && (arg0.isObject() && asObject(arg0)->inherits(JSHTMLElement::info()))) || (argsCount >= 2 && (arg0.isObject() && asObject(arg0)->inherits(JSHTMLElement::info())))) return jsHTMLSelectElementPrototypeFunctionAdd2(exec); if (argsCount < 1) return throwVMError(exec, createNotEnoughArgumentsError(exec)); return throwVMTypeError(exec); } Notice jsHTMLSelectElementPrototypeFunctionAdd1(), jsHTMLSelectElementPrototypeFunctionAdd2() correspond to overloads HTMLSelectElement.add(HTMLElement, HTMLElement) and HTMLSelectElement.add(HTMLElement, long), respectively. Without loss of generality, suppose a web page executes HTMLSelectElement.add(O, 2). Then we will call jsHTMLSelectElementPrototypeFunctionAdd1(). But we should call jsHTMLSelectElementPrototypeFunctionAdd2().
(In reply to comment #12) > Without loss of generality, suppose a web page executes > HTMLSelectElement.add(O, 2). [...] *where O is an HTML option element.
Created attachment 256053 [details] Patch and updated layout tests
(In reply to comment #4) > I also noticed that calling select.add(O, undefined) where O is some HTML > option element prepends O to the list of options in WebKit r178097 or later. > But it appends it to the end of the list of options in Chrome for Mac > 44.0.2403.61 beta (64-bit) and Firefox for Mac 34.0.5. Filed bug #146566 for this issue.
Comment on attachment 256053 [details] Patch and updated layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=256053&action=review r=me % nits. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4510 > + # FIXME: Implement support for overloaded functions with variadic arguments. nit: overloaded *constructors* > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3964 > + if () Oh god :) That's terrible but this is not caused by your patch and your change did not make the variadic argument support worse, so OK. Thanks for adding bindings test coverage (now we know how broken it is). > LayoutTests/fast/dom/HTMLSelectElement/add.html:26 > +shouldThrow('testAdd("foo")'); I would specify the expected exception for cases like these: shouldThrow('testAdd("foo")', '"TypeError: Type error"'); The reasoning is that in those cases, it is clearly defined in Web IDL that a *TypeError* should be thrown.
Committed r186265: <http://trac.webkit.org/changeset/186265>