Bug 7726 – REGRESSION: orbitz calendar fails (JavaScript function serialization/parsing)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Justin Garcia]

Goto:
http://www.orbitz.com/
Click inside the text field for the "Leave" or "Return" date.

A calendar should appear to let you chose a date graphically.  It doesn't in TOT, but does in 10.4.5.

[Justin Garcia]

Oops, forgot an 'o' in "choose".

[Alexey Proskuryakov]

Created attachment 7019 [details]
reduced test case

[Alexey Proskuryakov]

Created attachment 7020 [details]
another test case

Looks like two separate issues:
1) Firefox doesn't serialize onClick handler as an anonymous function, but WebKit does;
2) WebKit has lost the ability to parse such.

[Darin Adler]

Seems like (2) is by far the bigger issue.

[Darin Adler]

Regarding (1):

The code that constructs the function is JSLazyEventListener::parseCode in kjs_events.cpp. It uses the built-in Function object as a constructor to make the function, so the function has no name. However, to match Gecko it would be nice to supply a name. To do this we'd need to do two things. First, we'd need to supply a name to the JSLazyEventListener constructor and add the name parameter to createHTMLEventHandler. Second, we'd need to create some way to construct such a function, since the built-in Function object as a constructor does not support setting the function's identifier.

Regarding (2):

The reason we don't parse a function expression any more and we used to is that we share the same parser for functions and for programs. We fixed a bug where we would allow function expressions in programs.

For a program, a function expression (without an identifier) is not legal, and should lead to a parse error. When parsing a function using the built-in Function object as a constructor, the JavaScript standard says that we should parse a FunctionBody, not a Program (section 10.1.2). But that's apparently not what Gecko is doing either, because a FunctionBody requires braces, so the test case wouldn't work if we followed exactly what the specification says.

I don't know how to change our JavaScript parser to work in different modes for these two purposes; there may be a feature that lets us do that in bison or a simple way to change the parser. But more importantly, I don't know what mode to invoke the parser in when using the Function object as a constructor!

[Darin Adler]

(In reply to comment #5)
> because a FunctionBody requires braces

I was wrong. A FunctionBody does not require braces.

Maciej points out that in this line of code:

    new Function("function (event) {}")

the text "function (event) { }" is the body of a function. So there's a body of a function there that contains another function in it!

As far as I can tell from reading the JavaScript specification, that *should* be a syntax error because that's a FunctionExpression, not a FunctionDeclaration. So I think that on issue (2) Firefox is wrong and WebKit is right! I'd love to get more data about that. We might want to file a Mozilla bug report.

[Darin Adler]

This boils down to incorrect code on the Orbitz site that just happens to work in other browsers. Someone should report that to Orbitz.

But it's relatively easy to fix JavaScriptCore so this is not a problem. I see a few differences from Gecko:

    a) Functions made with "new Function" in Gecko get the name "anonymous", while JavaScriptCore leaves them without names.
    b) Functions for event handlers are named after the attribute in Gecko, while WebCore leaves them without names.
    c) Gecko puts function declarations at the beginnings of new lines when serializing. JavaScriptCore does not.
    d) Gecko allows you to parse a function with no function name in the expression passed "new Function".
    e) Gecko formats the functions a bit differently (brace on the same line as the function name).

To fix the bug, all we have to do is resolve (a), which is a tiny patch to JavaScriptCore. I have changes for (a), (b), and (c) in my tree now. I think (d) is something we get right and Gecko gets wrong. I think (e) is something we might want to consider doing, just to reduce the chance of a another bug like this one where someone depends on the details of behavior a bit too much.

[Darin Adler]

Created attachment 7056 [details]
patch that fixes the issues I called (a), (b), and (c)

[Darin Adler]

(In reply to comment #8)
> Created an attachment (id=7056) [edit]
> patch that fixes the issues I called (a), (b), and (c)

For performance, the "anonymous" should probably be added to the list in KJS_IDENTIFIER_EACH_PROPERTY_NAME_GLOBAL in identifier.h and then we should use anonymousPropertyName instead of "anonymous", which is annoying, since a function name is not really a property name.

[Darin Adler]

It's also possible that the name "anonymous" should appear in the serialized function, but should not be the function's name internally. Should do more testing to see if that's so.

[Darin Adler]

In a way this is related to bug 6279, where we gave the internal functions names. A bug we fixed about named function parsing was bug 4698. A change that might have introduced this problem was the rework of the JavaScript parser to match the specification and fix conflicts, which I did on 2005-09-27.

[Darin Adler]

Created attachment 7060 [details]
patch that fixes the issues I called (a), (b), and (c)

[Alice Liu]

<rdar://problem/4483878>

[Maciej Stachowiak]

Code change looks good, but:

1) "It's also possible that the name "anonymous" should appear in the serialized
function, but should not be the function's name internally. Should do more
testing to see if that's so."

Did you do this testing?


2) This needs test cases that cover the three issues you fixed.

I am not sure what the deal is with (d), the parameter to Function should be a function body, not a function expression or declaration, I believe.

r=me on the assumption that you address 1 and 2.

[Darin Adler]

(In reply to comment #14)
> Code change looks good, but:
> 
> 1) "It's also possible that the name "anonymous" should appear in the
> serialized
> function, but should not be the function's name internally. Should do more
> testing to see if that's so."
> 
> Did you do this testing?

Turns out there's no such thing as "the function's name internally". The serialized form is the only place the function name shows up!

> 2) This needs test cases that cover the three issues you fixed.

I wrote test cases for (a) and (b).

[Andrew Buston]

spam removed