RESOLVED FIXED 24291
REGRESSION (r38635): Single line JavaScript comment prevents HTML button click handler execution 
https://bugs.webkit.org/show_bug.cgi?id=24291
Summary REGRESSION (r38635): Single line JavaScript comment prevents HTML button clic...
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
DetailsFormatted DiffDiff
Proposed patch (6.18 KB, patch)
2009-03-10 14:30 PDT, Cameron Zwarich (cpst) 		ggaren: review+
DetailsFormatted DiffDiff
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.