Bug 25624 – REGRESSION(r33963-r33980): Function.toString preserves quote style, but it shouldn't

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[jasneet]

I Steps:
Go to http://www.greenmountaininn.com/?gclid=CMHY_rPpv5UCFQZqswodsj4hQg

II Issue:
Javascript code seen instead of normal content

III Conclusion:
The way single and double quotes are interpreted when one type of quotes are within another. Line sd+="<div style='"+rt+"'>"; is causing the issue.

IV Other Browsers:
IE7: ok
FF3: ok

V Nightly tested: 43092

Bug in Chromium : http://code.google.com/p/chromium/issues/detail?id=696

[jasneet]

Created attachment 30107 [details]
reduced testcase

[Alexey Proskuryakov]

This is a regression from Safari 3.

[Alexey Proskuryakov]

<rdar://problem/6870599>

[Alexey Proskuryakov]

Created attachment 30228 [details]
further reduced test case

[Alexey Proskuryakov]

This is a regression in how Function.toString() prints string literals - previously, double quotes were always used, matching Firefox - but now, the original style of quotes is preserved.

[David Kilzer (:ddkilzer)]

Internal autospade tool reports:

Works: r33963  Fails: r33980

[chris reiss]

starting on this ...

[chris reiss]

Created attachment 80505 [details]
fix

added a conversion function for this - hopefully in the right place and not too much wheel reinvention.

[WebKit Review Bot]

Attachment 80505 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/JavaScriptCore/runtime/FunctionPrototype.cpp:76:  Extra space before ) in for  [whitespace/parens] [5]
Source/JavaScriptCore/runtime/FunctionPrototype.cpp:85:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/FunctionPrototype.cpp:84:  Missing space before ( in switch(  [whitespace/parens] [5]
Source/JavaScriptCore/runtime/FunctionPrototype.cpp:87:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/FunctionPrototype.cpp:86:  Missing space before ( in switch(  [whitespace/parens] [5]
Source/JavaScriptCore/runtime/FunctionPrototype.cpp:107:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/FunctionPrototype.cpp:106:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 7 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Chris, I'm glad you made the effort to fix this, and I hate to say it, but do we really want to make this change?

It looks like the website this was reported against has since been fixed, and we've had no reports of this bug at other websites. I'd prefer not to go through contortions in JavaScriptCore to maintain compatibility with a poorly coded website that doesn't exist anymore.

In general, we made a choice a few years ago not to decompile or specially contort JavaScript in Function.prototype.toString. We just return the original string provided in source code. A bug or two has cropped up here and there from poorly coded websites, but we haven't seen one that justified going back on our strategy yet.

[Gavin Barraclough]

I don't think we intend to change our current behavior here.  This website has fixed its content, I don't think we are aware of further compatibility issues, and it is not clear that additional processing of the source is a good idea.

Since neither the spec nor web compatibility require us to do this, I think the bug is probably invalid, but it seems clear to be at least won't fix.