Summary: | REGRESSION (r178097): JavaScript TypeError after clicking on compose button in Yahoo Mail | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||
Component: | WebCore Misc. | Assignee: | Daniel Bates <dbates> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, darin, gyuyoung.kim, mitz, sam, shiva.jm | ||||||||
Priority: | P1 | Keywords: | InRadar, Regression | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | iPhone / iPad | ||||||||||
OS: | Unspecified | ||||||||||
URL: | http://mail.yahoo.com | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=146566 | ||||||||||
Bug Depends on: | 139179 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Daniel Bates
2015-07-01 09:49:02 PDT
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) 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> |