Bug 13623 - Decompilation of function doesn't compile with "++(x,y)"
Summary: Decompilation of function doesn't compile with "++(x,y)"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
Depends on:
Blocks: 13638
  Show dependency treegraph
 
Reported: 2007-05-08 01:38 PDT by Jesse Ruderman
Modified: 2007-05-25 10:50 PDT (History)
1 user (show)

See Also:


Attachments
Patch for toString()ing pre/postfix/typeof operators that contain grouping expression (14.57 KB, patch)
2007-05-24 06:37 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch for toString()ing pre/postfix/typeof operators that contain grouping expression. (8.52 KB, patch)
2007-05-24 06:57 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
The correct patch (same as the first one, except the LayoutTests/ChangeLog) (14.07 KB, patch)
2007-05-25 05:12 PDT, Kimmo Kinnunen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Ruderman 2007-05-08 01:38:47 PDT
> f = function(){ ++(x,y) };
function () 
{
  ++x, y;
}

> eval("" + f)
SyntaxError: Parse error

I think the best way to avoid this "decompilation of a function fails to compile" bug is to reject the initial function at compile time, like Firefox does.

Found by jsfunfuzz.
Comment 1 Eric Seidel (no email) 2007-05-10 04:36:16 PDT
Except there is no "compile time" in JavaScriptCore.  JSC parses the source to make an abstract syntax tree and then walks the tree for execution.
Comment 2 Jesse Ruderman 2007-05-10 12:02:00 PDT
"Parse time" then? :)
Comment 3 Kimmo Kinnunen 2007-05-24 06:37:44 PDT
Created attachment 14702 [details]
Patch for toString()ing pre/postfix/typeof operators that contain grouping expression

The patch makes errors like these produce run-time exception similar to the other varians of this error, like "function f() { (g())++; }"

SyntaxError for eval("function() {++x, y; }") is another issue, caused by treating 'function' as function declaration and not function expression. Firefox treats anonymous functions that appear in SourceElement context as  function expressions.
Comment 4 Kimmo Kinnunen 2007-05-24 06:43:38 PDT
Comment on attachment 14702 [details]
Patch for toString()ing pre/postfix/typeof operators that contain grouping expression

Seems that I got some conflict fragments in LayoutTests/ChangeLog..
Comment 5 Kimmo Kinnunen 2007-05-24 06:57:42 PDT
Created attachment 14703 [details]
Patch for toString()ing pre/postfix/typeof operators that contain grouping expression. 

Same patch as above. Seems that the conflict markers in LayoutTests/ChangeLog had been committed to the repository. I removed them locally, and that's why they show up in the diff. (conflict markers not removed in this)

About the previous Firefox comment: I'm no expert in how firefox/spidermonkey works, but that's my guess why the anonymous function expression works in the SourceElement context.
Comment 6 Darin Adler 2007-05-24 09:44:20 PDT
Comment on attachment 14703 [details]
Patch for toString()ing pre/postfix/typeof operators that contain grouping expression. 

r=me
Comment 7 David Kilzer (:ddkilzer) 2007-05-24 09:51:35 PDT
(In reply to comment #5)
> Same patch as above. Seems that the conflict markers in LayoutTests/ChangeLog
> had been committed to the repository. I removed them locally, and that's why
> they show up in the diff. (conflict markers not removed in this)

That's my bad.  I just removed them.

Comment 8 Jesse Ruderman 2007-05-24 12:19:55 PDT
(In reply to comment #0)
> > eval("" + f)
> SyntaxError: Parse error

(In reply to comment #3)
> SyntaxError for eval("function() {++x, y; }") is another issue, caused by
> treating 'function' as function declaration and not function expression.
> Firefox treats anonymous functions that appear in SourceElement context as 
> function expressions.

Oops, I forgot that I needed parentheses there.  Firefox trunk requires parentheses too (see https://bugzilla.mozilla.org/show_bug.cgi?id=376052).
Comment 9 Sam Weinig 2007-05-24 17:54:02 PDT
Landed in r21473 and r21393 ?
Comment 10 Kimmo Kinnunen 2007-05-24 21:40:07 PDT
(In reply to comment #9)
> Landed in r21473 and r21393 ?
> 

I don't understand the comment/question, but I believe the patch has not yet been applied (r21742). The mentioned revs are different patches (fixing different bugs)
Comment 11 Kimmo Kinnunen 2007-05-24 21:57:26 PDT
The first patch is ok but has the LayoutTests/ChangeLog problem.
The second patch was a old patch for different bug. 

Sorry for the mixup. I'll upload the first patch again with correct ChangeLog..
Comment 12 Kimmo Kinnunen 2007-05-25 05:12:54 PDT
Created attachment 14719 [details]
The correct patch (same as the first one, except the LayoutTests/ChangeLog)
Comment 13 Sam Weinig 2007-05-25 10:50:59 PDT
Landed, for real this time, in r21761.