Bug 146515 – REGRESSION (r178097): JavaScript TypeError after clicking on compose button in Yahoo Mail

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

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".

[Daniel Bates]

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)

[Daniel Bates]

<rdar://problem/21348421>

[Daniel Bates]

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

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 255988 [details]
[Patch] Work-in-progress

Work-in-progress patch. Needs change log entries.

[Chris Dumez]

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);

[Chris Dumez]

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}) {

[Darin Adler]

We don’t want to allow extra arguments in every function.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Darin Adler]

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.

[Daniel Bates]

(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().

[Daniel Bates]

(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.

[Daniel Bates]

Created attachment 256053 [details]
Patch and updated layout tests

[Daniel Bates]

(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.

[Chris Dumez]

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.

[Daniel Bates]

Committed r186265: <http://trac.webkit.org/changeset/186265>