Bug 42900 - [JSC] JavaScript parsing error when loading Equifax web page
Summary: [JSC] JavaScript parsing error when loading Equifax web page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Nobody
URL: http://www.equifax.com/credit-product...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-07-23 10:32 PDT by David Kilzer (:ddkilzer)
Modified: 2010-08-26 15:52 PDT (History)
4 users (show)

See Also:


Attachments
Original HTML source with <base> tag (109.17 KB, text/html)
2010-07-23 10:32 PDT, David Kilzer (:ddkilzer)
no flags Details
Webarchive of the web page (408.85 KB, application/x-webarchive)
2010-07-23 10:32 PDT, David Kilzer (:ddkilzer)
no flags Details
Partially reduced test case (8.50 KB, text/html)
2010-07-23 10:36 PDT, David Kilzer (:ddkilzer)
no flags Details
Reduction (55 bytes, text/html)
2010-07-23 10:46 PDT, David Kilzer (:ddkilzer)
no flags Details
Patch (5.20 KB, patch)
2010-08-26 13:41 PDT, Oliver Hunt
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2010-07-23 10:32:02 PDT
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.
Comment 1 David Kilzer (:ddkilzer) 2010-07-23 10:32:25 PDT
Created attachment 62445 [details]
Webarchive of the web page
Comment 2 David Kilzer (:ddkilzer) 2010-07-23 10:36:09 PDT
Created attachment 62446 [details]
Partially reduced test case

Only the offending <script></script> tag that doesn't parse.
Comment 3 David Kilzer (:ddkilzer) 2010-07-23 10:36:51 PDT
<rdar://problem/8227915>
Comment 4 David Kilzer (:ddkilzer) 2010-07-23 10:46:06 PDT
Created attachment 62448 [details]
Reduction

The issue appears to be parsing "-->" after a C-style comment "/* */" on the same line.
Comment 5 Zoltan Herczeg 2010-07-23 12:09:43 PDT
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.
Comment 6 Oliver Hunt 2010-07-23 12:11:52 PDT
(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.
Comment 7 Oliver Hunt 2010-07-23 12:14:19 PDT
> 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
Comment 8 Zoltan Herczeg 2010-07-23 12:17:13 PDT
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.
Comment 9 Oliver Hunt 2010-07-23 12:20:01 PDT
(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.
Comment 10 Zoltan Herczeg 2010-07-23 12:24:15 PDT
> 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.
Comment 11 Oliver Hunt 2010-08-26 12:42:57 PDT
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.
Comment 12 Oliver Hunt 2010-08-26 13:41:33 PDT
Created attachment 65612 [details]
Patch
Comment 13 Oliver Hunt 2010-08-26 14:27:33 PDT
Committed r66135
Comment 14 David Kilzer (:ddkilzer) 2010-08-26 15:52:56 PDT
(In reply to comment #13)
> Committed r66135

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