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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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) {
    }
}

Comment 1

[Jesse Ruderman]

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

Same bug?

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 252563 [details] [diff] [review]
Proposed fix

Those pesky non-top-most locals! Thanks,

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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.

Comment 5

[Brendan Eich [: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

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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 7

[Brendan Eich [:brendan]]

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

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Once more

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 252710 [details] [diff] [review]
Once more

Thanks, r=me.

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/extensions/regress-366668-01.js

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/extensions/regress-366668-02.js

Comment 13

[Bob Clary [:bc] (inactive)]

/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

[Bob Clary [:bc] (inactive)]

verified fixed 2007-02-17 1.9.0 windows/mac*/linux, note tests pass on 1.8.1 as well.