Bug 7726 - REGRESSION: orbitz calendar fails (JavaScript function serialization/parsing)
Summary: REGRESSION: orbitz calendar fails (JavaScript function serialization/parsing)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Darin Adler
URL: http://www.orbitz.com/
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2006-03-11 16:40 PST by Justin Garcia
Modified: 2017-04-23 22:57 PDT (History)
4 users (show)

See Also:


Attachments
reduced test case (462 bytes, text/html)
2006-03-12 02:37 PST, Alexey Proskuryakov
no flags Details
another test case (172 bytes, text/html)
2006-03-12 02:53 PST, Alexey Proskuryakov
no flags Details
patch that fixes the issues I called (a), (b), and (c) (15.68 KB, patch)
2006-03-13 21:32 PST, Darin Adler
no flags Details | Formatted Diff | Diff
patch that fixes the issues I called (a), (b), and (c) (16.30 KB, patch)
2006-03-14 08:10 PST, Darin Adler
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 2006-03-11 16:40:36 PST
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.
Comment 1 Justin Garcia 2006-03-11 16:41:50 PST
Oops, forgot an 'o' in "choose".
Comment 2 Alexey Proskuryakov 2006-03-12 02:37:58 PST
Created attachment 7019 [details]
reduced test case
Comment 3 Alexey Proskuryakov 2006-03-12 02:53:29 PST
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.
Comment 4 Darin Adler 2006-03-12 22:02:46 PST
Seems like (2) is by far the bigger issue.
Comment 5 Darin Adler 2006-03-13 09:23:44 PST
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!
Comment 6 Darin Adler 2006-03-13 19:44:38 PST
(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.
Comment 7 Darin Adler 2006-03-13 21:19:56 PST
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.
Comment 8 Darin Adler 2006-03-13 21:32:03 PST
Created attachment 7056 [details]
patch that fixes the issues I called (a), (b), and (c)
Comment 9 Darin Adler 2006-03-13 21:35:50 PST
(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.
Comment 10 Darin Adler 2006-03-14 04:31:23 PST
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.
Comment 11 Darin Adler 2006-03-14 04:35:05 PST
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.
Comment 12 Darin Adler 2006-03-14 08:10:35 PST
Created attachment 7060 [details]
patch that fixes the issues I called (a), (b), and (c)
Comment 13 Alice Liu 2006-03-20 07:56:11 PST
<rdar://problem/4483878>
Comment 14 Maciej Stachowiak 2006-03-22 20:51:10 PST
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.
Comment 15 Darin Adler 2006-03-23 22:25:43 PST
(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).
Comment 16 Andrew Buston 2017-04-23 22:57:38 PDT
spam removed