Assertion failure: rval[strlen(rval)-1] == '}'

RESOLVED WORKSFORME

Status

()

Core
JavaScript Engine
--
critical
RESOLVED WORKSFORME
11 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

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

Trunk
crash, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
js> function() { return ({x setter: function(){} | 5 }) }
Assertion failure: rval[strlen(rval)-1] == '}', at jsopcode.c:3875

Updated

11 years ago
Status: NEW → ASSIGNED
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All

Updated

11 years ago
Assignee: general → crowder
Status: ASSIGNED → NEW

Comment 1

11 years ago
What is the expected result?

Updated

11 years ago
Status: NEW → ASSIGNED
The value of that setter is not a function, it's a string.  Debug to see what rval contains.

/be
So this is another case that must fall back on the old setter: syntax.  IOW, as with the first LOCAL_ASSERT, transform into an if that tests opposite of the asserted condition.

/be

Comment 4

11 years ago
Created attachment 241808 [details] [diff] [review]
another old-school getter/setter decompilation case

I also cleaned up some logic that is unnecessary (pasted from above and added in my last patch in this area without noticing the redundancy).  I'll rip it out, if it's bad form to piggyback.
Attachment #241808 - Flags: review?(brendan)
(Reporter)

Comment 5

11 years ago
I don't think converting the assertion to an if creates the correct test.  Consider:

({x setter: function(){} | function(){} })

Comment 6

11 years ago
I get an invalid setter usage error for that case, which seems fine?
(Reporter)

Comment 7

11 years ago
You'd get "invalid setter usage" for the stuff in comment 0 too if you try to evaluate the object naked.  That's why it's wearing a function.

Comment 8

11 years ago
This decompilation seems okay, though.  No meaning is lost and the code is still broken in the same way as it was before.

js> function() { return ({x setter: function(){} | function(){} }) }
function () {
    return {set x() {} | function () {}};
}
(In reply to comment #8)
> This decompilation seems okay, though.  No meaning is lost and the code
> is still broken in the same way as it was before.
> 
> js> function() { return ({x setter: function(){} | function(){} }) }
> function () {
>     return {set x() {} | function () {}};
> }

No, that's a syntax error whereas the old syntax is a runtime error. Different meaning.  See comment 3, use the old syntax here.

/be

Comment 10

11 years ago
So I should look for the first { of the function match/count braces to the end, and if there is anything after the last }, I should use old getter/setter output?
Talking to crowder on IRC, thinking for three seconds in a row about this (:-), I think we should make non-function-expression initialisers for old-style getter: and setter: property initialisers be syntax errors.

/be
Comment on attachment 241808 [details] [diff] [review]
another old-school getter/setter decompilation case

- for now in light of new thinking.  Hope we can get away with this incompatible change.  It's not just making a runtime error be a syntax error -- up till now you could write o = {p getter: f} where f was a function declared previously.

Not sure how to keep that working and make decompilation simple.

Also not sure we want this incompatibility in js1.7src/1.8.1.1 -- by default, we should *not* make so incompatible (albeit to non-standard API) a change.

/be
Attachment #241808 - Flags: review?(brendan) → review-

Comment 13

11 years ago
Is this really a critical bug?

Comment 14

11 years ago
I think this one is beyond my feeble knowledge of the parser, hoping someone else will pick up.
Assignee: crowder → general
Status: ASSIGNED → NEW
(Reporter)

Updated

11 years ago
No longer blocks: 349611
(Reporter)

Updated

11 years ago
Blocks: 349611

Comment 15

11 years ago
This bug has either morphed or been fixed:

js> function() { return ({x setter: function(){} | 5 }) }
function () {
    return {x setter: function () {} | 5};
}
(Reporter)

Comment 16

11 years ago
What do you mean by "morphed"?  That looks fine to me.

This bug was probably fixed in one of the many other getter/setter decompilation bugs (bug 381101, bug 356083, etc).
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → WORKSFORME

Comment 17

11 years ago
Is the loss of parenthesis an issue here?
(Reporter)

Comment 18

11 years ago
The parens around the entire object literal?  Those aren't needed because after "return", a "{" opens an object literal rather than a code block.

Updated

10 years ago
Flags: in-testsuite?

Comment 19

10 years ago
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-356106.js,v  <--  regress-356106.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.