Bug 24291 - REGRESSION (r38635): Single line JavaScript comment prevents HTML button click handler execution
Summary: REGRESSION (r38635): Single line JavaScript comment prevents HTML button clic...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Cameron Zwarich (cpst)
URL:
Keywords: HasReduction, InRadar, Regression
: 24482 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-03-02 07:44 PST by Mihnea Ovidenie
Modified: 2009-03-10 15:49 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 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
Comment 1 Alexey Proskuryakov 2009-03-03 08:37:20 PST
Confirmed as a regression from 3.1.2.
Comment 2 Alexey Proskuryakov 2009-03-03 08:40:01 PST
Sorry, from 3.2.1
Comment 3 Cameron Zwarich (cpst) 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.
Comment 4 Cameron Zwarich (cpst) 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
Comment 5 Cameron Zwarich (cpst) 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.
Comment 6 Cameron Zwarich (cpst) 2009-03-03 21:17:18 PST
Created attachment 28255 [details]
Reduction

If the test passes, an alert is displayed with the message PASS.
Comment 7 Cameron Zwarich (cpst) 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.
Comment 8 Cameron Zwarich (cpst) 2009-03-10 04:09:24 PDT
*** Bug 24482 has been marked as a duplicate of this bug. ***
Comment 9 Alexey Proskuryakov 2009-03-10 04:24:13 PDT
Per the duplicate, this is <rdar://problem/6663472>.
Comment 10 Cameron Zwarich (cpst) 2009-03-10 14:30:40 PDT
Created attachment 28450 [details]
Proposed patch
Comment 11 Geoffrey Garen 2009-03-10 14:34:06 PDT
Comment on attachment 28450 [details]
Proposed patch

r=me
Comment 12 Cameron Zwarich (cpst) 2009-03-10 15:49:21 PDT
Landed in r41565.