Bug 24291

Summary:

REGRESSION (r38635): Single line JavaScript comment prevents HTML button click handler execution

Product:

WebKit

Reporter:

Mihnea Ovidenie <mihnea>

Component:

WebCore JavaScript

Assignee:

Cameron Zwarich (cpst) <zwarich>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, ddkilzer, sam, zwarich

Priority:

Keywords:

HasReduction, InRadar, Regression

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Attachments:

Description	Flags
Reduction	none
Proposed patch (without tests)	none
Proposed patch	ggaren: review+

Mihnea Ovidenie

Reported 2009-03-02 07:44:05 PST

Hello, If i add a single line JavaScript comment at the end of a HTML button handler for click, the handler is not executed. I was able to reproduce the problem using the following sample: <HTML> <BODY> <input type="button" id="myButtonTest" value="Works" onClick="alert('OnClick Button1');" /> <input type="button" id="myButtonTest1" value="Doesn'tWork" onClick="alert('OnClick Button2');//;" /> </BODY> </HTML> While clicking on button "Works" displays the alert, clicking on the button "Doesn'tWork" does not bring up the alert message. Removing the //; comment or replacing it with /**/ solves the problem. I was able to reproduce the problem using: -> WebKit 41357 running on Mac Leopard -> Safari 4 both on Mac Leopard/Win -> Google Chrome on Win FF3/Opera 9.63 do not have the problem in both Mac/Win environments. Regards, Mihnea

Attachments
Reduction (56 bytes, text/html) 2009-03-03 21:17 PST, Cameron Zwarich (cpst)	no flags	Details
Proposed patch (without tests) (907 bytes, patch) 2009-03-04 00:50 PST, Cameron Zwarich (cpst)	no flags	Details Formatted Diff Diff
Proposed patch (6.18 KB, patch) 2009-03-10 14:30 PDT, Cameron Zwarich (cpst)	ggaren: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2009-03-03 08:37:20 PST

Confirmed as a regression from 3.1.2.

Alexey Proskuryakov

Comment 2 2009-03-03 08:40:01 PST

Sorry, from 3.2.1

Cameron Zwarich (cpst)

Comment 3 2009-03-03 19:33:49 PST

As expected, there is an unreported parsing error when attempting to parse the event handler. I'll try to use Bison's debugging to figure out why.

Cameron Zwarich (cpst)

Comment 4 2009-03-03 20:04:59 PST

This regressed between r38592 and r38645. I suspect it was caused by r38635: http://trac.webkit.org/changeset/38635

Cameron Zwarich (cpst)

Comment 5 2009-03-03 20:50:27 PST

This is the problem, at the top of constructFunction() in FunctionConstructor.cpp: UString program; if (args.isEmpty()) program = "(function(){})"; else if (args.size() == 1) program = "(function(){" + args.at(exec, 0).toString(exec) + "})"; else { program = "(function(" + args.at(exec, 0).toString(exec); for (size_t i = 1; i < args.size() - 1; i++) program += "," + args.at(exec, i).toString(exec); program += "){" + args.at(exec, args.size() - 1).toString(exec) + "})"; } This is possibly incorrect when anything in the middle has a single-line comment. The fix is to add newlines after substitutions. I'll assign this to myself.

Cameron Zwarich (cpst)

Comment 6 2009-03-03 21:17:18 PST

Created attachment 28255 [details] Reduction If the test passes, an alert is displayed with the message PASS.

Cameron Zwarich (cpst)

Comment 7 2009-03-04 00:50:43 PST

Created attachment 28258 [details] Proposed patch (without tests) Here's the fix. It needs a ChangeLog and some tests, for both the event handler and "new Function(...)" cases. I should be able to get to it by tomorrow evening.

Cameron Zwarich (cpst)

Comment 8 2009-03-10 04:09:24 PDT

*** Bug 24482 has been marked as a duplicate of this bug. ***

Alexey Proskuryakov

Comment 9 2009-03-10 04:24:13 PDT

Per the duplicate, this is <rdar://problem/6663472>.

Cameron Zwarich (cpst)

Comment 10 2009-03-10 14:30:40 PDT

Created attachment 28450 [details] Proposed patch

Geoffrey Garen

Comment 11 2009-03-10 14:34:06 PDT

Comment on attachment 28450 [details] Proposed patch r=me

Cameron Zwarich (cpst)

Comment 12 2009-03-10 15:49:21 PDT

Landed in r41565.

Note You need to log in before you can comment on or make changes to this bug.