Bug 42900 – [JSC] JavaScript parsing error when loading Equifax web page

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Kilzer (:ddkilzer)]

Created attachment 62444 [details]
Original HTML source with <base> tag

* SUMMARY
When loading <http://www.equifax.com/credit-product-list/> a JavaScript parsing syntax error occurs on line 1689 of the main resource when parsing <script></script> content.

The other odd issue (maybe related to this) that occurs with the Web Inspector enabled is that 2 copies of "/credit-product-list/" appear in the Resources list (as well as the Activity window in Safari 5).  The second one is displayed as a broken image.  And clicking on the parsing/reference errors in the Inspector shows the second copy of the main resource ("/credit-product-list/") instead of the first.

* STEPS TO REPRODUCE
1. Launch Safari 5 or a WebKit nightly build.
2. Open URL:  http://www.equifax.com/credit-product-list/

* RESULTS
The "Customer Service" drop-down menu doesn't work because of a JavaScript parsing error earlier in the page.

* REGRESSION
Unknown.  Occurs with Safari 5 on Snow Leopard 10.6.4 (10F569) as well as with WebKit nighty build r63958.

* NOTES
Google Chrome does not report any JavaScript parsing errors when loading the same page.

[David Kilzer (:ddkilzer)]

Created attachment 62445 [details]
Webarchive of the web page

[David Kilzer (:ddkilzer)]

Created attachment 62446 [details]
Partially reduced test case

Only the offending <script></script> tag that doesn't parse.

[David Kilzer (:ddkilzer)]

<rdar://problem/8227915>

[David Kilzer (:ddkilzer)]

Created attachment 62448 [details]
Reduction

The issue appears to be parsing "-->" after a C-style comment "/* */" on the same line.

[Zoltan Herczeg]

This is working as it should be. Or at least how the lexer works for a long-long time.

In JavaScript, "<!--" means a comment until the next line, (same as //)

<script type="text/javascript">
<!-- comment until the next line 
/* normal comment */
--> /* parse error */ </script>

You can even reduce the test case to:

<script type="text/javascript"><!--
--> or anything </script>

I will check the ECMA standard, a moment.

[Oliver Hunt]

(In reply to comment #5)
> This is working as it should be. Or at least how the lexer works for a long-long time.
> 
> In JavaScript, "<!--" means a comment until the next line, (same as //)
> 
> <script type="text/javascript">
> <!-- comment until the next line 
> /* normal comment */
> --> /* parse error */ </script>
> 
> You can even reduce the test case to:
> 
> <script type="text/javascript"><!--
> --> or anything </script>
> 
> I will check the ECMA standard, a moment.

ECMA262 doesn't specify it but based on testing <!-- and --> should both be treated as though the start single line comments.

[Oliver Hunt]

> ECMA262 doesn't specify it but based on testing <!-- and --> should both be treated as though the start single line comments.


Which we do do, although we require --> to be at the beginning of a line for it to be consider a comment.  I presume at some point we've made multiline comments be treated as stuff on the line (setting m_atLineStart to false).  However neither mozilla nor chrome hjave the beginning of the line requirement

[Zoltan Herczeg]

I didn't see such thing in ECMA-262, chapter 7.4 (comments), but the following link mention <!-- as a single line comment: http://www.javascripter.net/faq/comments.htm

If you want to disable this feature, just delete the folowing lines:
http://trac.webkit.org/browser/trunk/JavaScriptCore/parser/Lexer.cpp#L561

I am not sure this is the right thing, maybe sime old sites will not like it.

[Oliver Hunt]

(In reply to comment #8)
> I didn't see such thing in ECMA-262, chapter 7.4 (comments), but the following link mention <!-- as a single line comment: http://www.javascripter.net/faq/comments.htm
> 
> If you want to disable this feature, just delete the folowing lines:
> http://trac.webkit.org/browser/trunk/JavaScriptCore/parser/Lexer.cpp#L561
> 
> I am not sure this is the right thing, maybe sime old sites will not like it.

It's not in the spec, i've already harassed brendan and he says that they just didn't get around to it.

[Zoltan Herczeg]

> It's not in the spec, i've already harassed brendan and he says that they just didn't get around to it.

Ok I got it, just we had a mid-air collision, and the order of the comments are messed up.

[Oliver Hunt]

Have a fix -- alas it's a a little more convoluted than "-->" is a line comment -- it's only a line comment if there's no preceeding code, so whitespace and multiline comments are allowed.

what joy.

[Oliver Hunt]

Created attachment 65612 [details]
Patch

[Oliver Hunt]

Committed r66135

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> Committed r66135

<http://trac.webkit.org/changeset/66135>