Incorrect decompilation of "for (let x in x.p)"

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
PowerPC
Mac OS X
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
Decompilation incorrectly puts ".p" on the loop variable, causing a syntax error:

js> function() { for(let x in x.p) { } }   
function () {
    for (let x.p in x.p) {
    }
}

Toss in an assignment and it copies the entire assignment as well as sticking some extra .p's in:

js> function() { for(let x in (x = x.p)) { } } 
function () {
    for (let x.p = x.p in x.p = x.p) {
    }
}
(Reporter)

Comment 1

11 years ago
js> function() { let (w) { with({x: w.something }) { } } }
function () {
    let (w) {
        with (mething, x:w.something}) {
        }
    }
}

Same bug?
(Assignee)

Comment 2

11 years ago
Created attachment 252563 [details] [diff] [review]
Proposed fix

So, the problem here is the mini-optimization that JSOP_GETLOCALPROP does: it pushes an offset that's not at the end of the sprinter. As a result, the next time we push a string, SprintPut gets confused and overwrites whatever happened to be at the old location. This was causing the JSOP_INITPROP (and for-in) code to become unhappy when it looked at the stack for what was there before.

All other uses of PushOff refer to newly-Sprinted locations.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #252563 - Flags: review?(brendan)
Comment on attachment 252563 [details] [diff] [review]
Proposed fix

Those pesky non-top-most locals! Thanks,

/be
Attachment #252563 - Flags: review?(brendan) → review+
(Assignee)

Comment 4

11 years ago
Created attachment 252684 [details] [diff] [review]
Better fix, I think

Here's the interdiff between the first and second patches:
               BEGIN_LITOPX_CASE(JSOP_GETLOCALPROP, 2)
                 i = GET_UINT16(pc);
                 LOCAL_ASSERT((uintN)i < ss->top);
-                lval = GetLocal(ss, i, JS_FALSE);
+                lval = GetLocal(ss, i, JS_TRUE);
                 if (!lval)
                     return NULL;
                 todo = SprintCString(&ss->sprinter, lval);

I think we need to retract, since we're Sprinting lval right after the GetLocal, but I'm not entirely sure I understand the retracting bit of the decompiler. Brendan, please tell me if this isn't needed.
Attachment #252563 - Attachment is obsolete: true
Attachment #252684 - Flags: review?(brendan)
So wait, if GetLocal always calls QuoteString, which always sprints at the end, how was the original code broken by lval being at an offset below the topmost sprintstack'ed offset?

/be
(Assignee)

Comment 6

11 years ago
It doesn't *always* sprint. It only sprints in the DVG case, where it has to generate the name from the block object atom.
Comment on attachment 252684 [details] [diff] [review]
Better fix, I think

This is good, thanks -- but now there's no caller of GetLocal that passes JS_FALSE for retract, so please get rid of that by evaluating partially with it equal to true.

/be
(Assignee)

Comment 8

11 years ago
Created attachment 252710 [details] [diff] [review]
Once more
Attachment #252684 - Attachment is obsolete: true
Attachment #252710 - Flags: review?(brendan)
Attachment #252684 - Flags: review?(brendan)
Comment on attachment 252710 [details] [diff] [review]
Once more

Thanks, r=me.

/be
Attachment #252710 - Flags: review?(brendan) → review+
(Assignee)

Comment 10

11 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 11

11 years ago
Created attachment 254266 [details]
js1_7/extensions/regress-366668-01.js

Comment 12

11 years ago
Created attachment 254267 [details]
js1_7/extensions/regress-366668-02.js

Updated

11 years ago
Flags: in-testsuite+

Comment 13

11 years ago
/cvsroot/mozilla/js/tests/js1_7/extensions/regress-366668-01.js,v  <--  regress-366668-01.js

/cvsroot/mozilla/js/tests/js1_7/extensions/regress-366668-02.js,v  <--  regress-366668-02.js

Comment 14

11 years ago
verified fixed 2007-02-17 1.9.0 windows/mac*/linux, note tests pass on 1.8.1 as well.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

11 years ago
No longer blocks: 349611
(Reporter)

Updated

11 years ago
Blocks: 349611
You need to log in before you can comment on or make changes to this bug.