More incorrect decompilation with "new" operator (e4x/decompilation/regress-352013.js)

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: brendan)

Tracking

(Blocks: 1 bug, {regression, testcase})

Trunk
mozilla1.8.1
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
js> function() { new (x(z))(w) }
function () {
    new x(z)(w);
}

js> function () {
    new x(z)(w);
}
function () {
    (new x(z))(w);
}
(Assignee)

Comment 1

12 years ago
Where's the bug?

js> x = Function
function Function() {
    [native code]
}
js> z = "print(arguments[0])"
print(arguments[0])
js> w = 42
42
js> new x(z)(w)
42
js> (new x(z))(w)
42
js> new (x(z)(w))
42
typein:6: TypeError: x(z)(w) is not a constructor

See ECMA-262 Ed. 3 11.2, the grammar is tricky.  The upshot is that you can string property operators together till you get to an argument list, and once you've done that, no matter the space between the new and the MemberExpression Arguments, you're calling that constructor.

Any Arguments appended to that form a CallExpression.

/be
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → INVALID
(Reporter)

Comment 2

12 years ago
The first decompilation is incorrect; you forgot to test the original.  (Round-trip changes are often a hint that the original decompilation is incorrect.)

js> new (x(z))(w)
42
[object Object]

js> new x(z)(w)
42

js> function() { h = new (x(z))(w) }
function () {
    h = new x(z)(w);
}
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Comment 3

12 years ago
Sorry, taking.

/be
Assignee: general → brendan
Status: REOPENED → NEW
OS: Mac OS X 10.4 → All
Priority: -- → P2
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.8.1
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

12 years ago
Created attachment 237849 [details] [diff] [review]
fix

Jesse, if you could test this, I'd appreciate it.

/be
Attachment #237849 - Flags: review?(mrbkap)

Updated

12 years ago
Attachment #237849 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 5

12 years ago
Fixed on trunk.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #237849 - Flags: approval1.8.1?

Comment 6

12 years ago
Comment on attachment 237849 [details] [diff] [review]
fix

a=schrep
Attachment #237849 - Flags: approval1.8.1? → approval1.8.1+
(Reporter)

Comment 7

12 years ago
Are you sure you checked this in?  I still see the bug, and I don't see a checkin referencing this bug number.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

12 years ago
Sorry, I did forget to check it in.  Done now:

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.175; previous revision: 3.174
done


/be
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

12 years ago
Fixed on the 1.8 branch.

/be
Keywords: fixed1.8.1

Comment 10

12 years ago
Checking in regress-352013.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352013.js,v  <--  regress-352013.js
initial revision: 1.1
done

Flags: in-testsuite+

Comment 11

12 years ago
verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1

Comment 12

12 years ago
this regressed on 1.8, 1.9 20061002

expect: function ( ) { new ( x ( z ) ) ( w ) ; }
actual: function ( ) { new x ( z ) ( w ) ; } 

file new bug or reopen this?

Comment 13

12 years ago
still failing 1.8, 1.9 20061009. reopening and clearing verified flags to get an answer.
Status: VERIFIED → REOPENED
Keywords: verified1.8.1
Resolution: FIXED → ---
(Assignee)

Comment 14

12 years ago
(In reply to comment #13)
> still failing 1.8, 1.9 20061009. reopening and clearing verified flags to get
> an answer.

Sorry I missed the bugmail from the previous comment.  This is correct, the input expression is overparenthesized.

/be
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Status: RESOLVED → VERIFIED
Keywords: verified1.8.1

Comment 15

12 years ago
Checking in regress-352013.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352013.js,v  <--  regress-352013.js
new revision: 1.2; previous revision: 1.1
done
(Reporter)

Comment 16

12 years ago
No, the previous version of the testcase was correct.  "new ( x ( z ) ) ( w )" is not overparenthesized.

js> x = Function; z = "print(arguments[0])"; w = 42;
42
js> f = function() { return new ( x ( z ) ) ( w ) ; }
function () {
    return new x(z)(w);
}
js> f()
42
[object Object]
js> (eval("" + f))()
42
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 17

12 years ago
ok. I'll let you two fight it out. ;-)
Flags: blocking1.8.1?
Keywords: verified1.8.1
Brendan, does this actually block 1.8.1, remembering that we're trying to keep the RC3 baseline set to only-critical, not-ridealong patches? Can this wait for 1.8.1.1?
(Assignee)

Comment 19

12 years ago
This bug's patch did land on the 1.8 branch already:

revision 3.89.2.49
date: 2006/09/12 23:37:34;  author: brendan%mozilla.org;  state: Exp;  lines: +2
1 -3
Fix 352013, a=schrep.

Obviously it wasn't the full fix.  Jesse, sorry about the deja vu all over again.  We can leave this for 1.8.1.1.

/be
Flags: blocking1.8.1? → blocking1.8.1.1?
Keywords: fixed1.8.1

Comment 20

12 years ago
(In reply to comment #19)
> This bug's patch did land on the 1.8 branch already:

yes and it was fixed and verified:

verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux

now it has regressed on 1.8 and 1.9

reverting test.
Checking in regress-352013.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352013.js,v  <--  regress-352013.js
new revision: 1.3; previous revision: 1.2
done

if this bug isn't fixed on 1.8, it shouldn't have the fixed1.8.1 keyword imo.
(Assignee)

Comment 21

12 years ago
Anyone know what checkin regressed this?

/be
Status: REOPENED → ASSIGNED
Keywords: fixed1.8.1

Comment 22

12 years ago
(In reply to comment #21)

the rough range is 20060930-20061002. The only checkin to jsopcode.c in that range is bug 355004 but that is a wild *** guess.
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Keywords: regression
Whiteboard: was fixed, something unfixed it.

Updated

12 years ago
Flags: blocking1.8.1.1+ → wanted1.8.1.x+
This seems to have gotten forgotten. Marking a blocker so Brendan can decide whether this is actually important for the branch or whether we clear the "wanted" flag
Flags: blocking1.8.1.2+
(Assignee)

Updated

12 years ago
Flags: blocking1.8.1.2+ → wanted1.8.0.x?
(Assignee)

Comment 24

11 years ago
I see why I was confused. Comment 16's function assigned to f is overparenthesized but this one is not:

f = function() { return new ( x ( z ) ( w ) ); }

Yet it decompiles without the necessary parens around the new operand:

function () {
    return new x(z)(w);
}

Bob, that says a test is wrongly requiring parens where they are unnecessary, and we are missing a different test.

/be
(Assignee)

Comment 25

11 years ago
Created attachment 260437 [details] [diff] [review]
second fix

This really was a separate bug; attaching here to keep it all together with the comments.

/be
Attachment #260437 - Flags: review?(mrbkap)

Comment 26

11 years ago
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352013.js,v  <--  regress-352013.js
new revision: 1.4; previous revision: 1.3
(Assignee)

Comment 27

11 years ago
The test covers new (x(z))(w), (new x(z))(w), new (x(y)(z)), new (x(y).z), and (new x(y)).z, all good. For complete coverage, you'd want the "left", "middle", and "right" associated new forms for other "addressing modes":

[z]
.@a
.@n::a
.n::z
.n::[z]

"left" is (new x(y))..., "middle" is new (x(y))..., and "right" new (x(y)...).

/be

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?

Updated

11 years ago
Attachment #260437 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 28

11 years ago
Second fix landed on trunk:

js/src/jsopcode.c 3.223

Thoughts on branch approval likelihood?

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Blocks: 355044
(Reporter)

Updated

11 years ago
Blocks: 352789
(Reporter)

Updated

11 years ago
Blocks: 353146

Comment 29

11 years ago
Checking in regress-352013.js;
/cvsroot/mozilla/js/tests/e4x/decompilation/regress-352013.js,v  <--  regress-352013.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+

Comment 30

11 years ago
verified fixed on the trunk with both tests.
Status: RESOLVED → VERIFIED
Whiteboard: was fixed, something unfixed it.

Updated

11 years ago
Summary: More incorrect decompilation with "new" operator → More incorrect decompilation with "new" operator (e4x/decompilation/regress-352013.js)

Updated

11 years ago
Flags: wanted1.8.0.x?
You need to log in before you can comment on or make changes to this bug.