Closed Bug 609617 Opened 9 years ago Closed 9 years ago

delete(eval(...)) calls indirect eval

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: jandem, Assigned: brendan)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 7 obsolete files)

Following test case fails in SM:
----
var x = "fail";
function f() {
    var x = "pass"; 
    delete(eval("print(x)"));
}
f();
----
This works for typeof and void, and works in Chrome and Opera 11; evilpies confirmed it also calls direct eval in Besen.
Problem seems to be some constant folding done in the interpreter.
|void eval("print(x)")|
>callname "eval"
>string "print(x)"
>eval 1
|delete eval("print(x)")|, gets constant folded to eval("print(x)"), true
>callname "eval"
>string "print(x)"
>call 1

I think |call 1| should be |eval 1| in this case, too.
Attached patch fix (obsolete) — Splinter Review
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #488243 - Flags: review?(jimb)
Priority: -- → P2
Target Milestone: --- → mozilla2.0b8
Would it be better to fix this in MakeSetCall itself? This should be a direct call, too, I would think:

js> dis(function f() { eval('x')++; }
)
flags: LAMBDA HEAVYWEIGHT
main:
00000:  callname "eval"
00003:  string "x"
00006:  setcall 1
00009:  eleminc
00010:  pop
00011:  stop

Source notes:
  0:     6 [   6] pcbase   offset 6
  2:     9 [   3] pcbase   offset 9
js>
I tested the other cases, they all throw:

js> x=1
1
js> f() 
typein:1: SyntaxError: invalid assignment left-hand side
js> function f() { eval('x')++; }
js> f()                           
typein:5: SyntaxError: invalid assignment left-hand side

However it does seem better to be safe and use belt in addition to braces, lest our clown pants fall down again. Thanks, new patch in a minute.

/be
Attached patch fix, v2 (obsolete) — Splinter Review
Attachment #488243 - Attachment is obsolete: true
Attachment #488280 - Flags: review?(jimb)
Attachment #488243 - Flags: review?(jimb)
As noted on IRC, I don't want to make an early error and risk breaking some IE-only path in a script's control flow graph that does insane stuff like this.

/be
Here's another test case:

function g() { print("bleah") }
function f() { function g() { throw('food'); } eval('g()')++; } 
f()

This should throw food, not print "bleah".
Is this still r? me, or is there more to fix in the emitter?
We talked before comment 8:

[12:39pm] brendan: also gdb is stalling at startup for me -- time to reboot mbp
[12:40pm] brendan: jimb: argh, emitter depended on the other MakeSetCall cases doing their thing
[12:40pm] brendan: jimb: new patch when i reconnect
[12:40pm] brendan: will beef up test
[12:41pm] jimb: brendan: I put my example in the bug.

but I didn't withdraw r? -- I'll obsolete that patch soon. This bug is tricky due to emitter assumptions about AST shape, tricky enough to minimize change to minimize regression risk for Firefox 4.

/be
(In reply to comment #7)
> Here's another test case:
> 
> function g() { print("bleah") }
> function f() { function g() { throw('food'); } eval('g()')++; } 
> f()
> 
> This should throw food, not print "bleah".

js> dis(f)
flags: HEAVYWEIGHT
main:
00000:  deflocalfun 0 function g() {throw "food";}
00005:  nullblockchain
00006:  nop
00007:  callname "eval"
00010:  string "g()"
00013:  setcall 1
00016:  eleminc
00017:  pop
00018:  stop

Source notes:
  0:     6 [   6] funcdef  function 0 (function g() {throw "food";})
  2:    13 [   7] pcbase   offset 6
  4:    16 [   3] pcbase   offset 9

I think this is doing the right thing. We don't want to call eval('g()') and let the eval have whatever effects it wants, and then return a non-reference, and only then throw. We simply must avoid an early error (compile-time all-paths error checking breaks IE-only calls-as-lvalues).

/be
(In reply to comment #10)
> I think this is doing the right thing. We don't want to call eval('g()') and
> let the eval have whatever effects it wants, and then return a non-reference,
> and only then throw.

Why don't we?  Per 11.3.1 we 1) call the eval function to produce a result, 2) don't throw because that result's not an attempted modification of an eval or arguments binding, 3) get a value from the result and call ToNumber on it, 4) add 1 to that, then 5) try to assign that value to the result of the eval function but throw because it's not a Reference.  The spec seems reasonably clear about what must be done.
Attached patch fix, v3 (obsolete) — Splinter Review
Main thing here is that all VB-ism "setcall" usages are strict mode errors. Take that, ancient IE extension! The test tries to cover all cases.

I fussed over ReportStrictModeError in really fussy ways, fussily, and uncapitalized the one capitalized (not counting acronyms) js.msg message.

/be
Attachment #488280 - Attachment is obsolete: true
Attachment #488334 - Flags: review?(jimb)
Attachment #488280 - Flags: review?(jimb)
(In reply to comment #11)
> (In reply to comment #10)
> > I think this is doing the right thing. We don't want to call eval('g()') and
> > let the eval have whatever effects it wants, and then return a non-reference,
> > and only then throw.
> 
> Why don't we?  Per 11.3.1 we 1) call the eval function to produce a result, 2)
> don't throw because that result's not an attempted modification of an eval or
> arguments binding, 3) get a value from the result and call ToNumber on it, 4)
> add 1 to that, then 5) try to assign that value to the result of the eval
> function but throw because it's not a Reference.  The spec seems reasonably
> clear about what must be done.

You're right, however: this would require redoing our code generation to work differently from how JSOP_SETCALL works. I'm not going do that here.

You could file a bug; I'd argue WONTFIX.

/be
Okay, filed as bug 609756, which we can circle back to after the 4.0 release.
Attachment #488334 - Attachment is obsolete: true
Attachment #488353 - Flags: review?(jimb)
Attachment #488334 - Flags: review?(jimb)
Attachment #488353 - Attachment description: fix, v3a (rebased and with better -[1234] assertEq "not reached" telltales) → fix, v3a (rebased and with better assertEq "not reached" telltales)
Attachment #488353 - Attachment is patch: true
Attachment #488353 - Attachment mime type: application/octet-stream → text/plain
(In reply to comment #10)
> I think this is doing the right thing. We don't want to call eval('g()') and
> let the eval have whatever effects it wants, and then return a non-reference,
> and only then throw. We simply must avoid an early error (compile-time
> all-paths error checking breaks IE-only calls-as-lvalues).

We already do call the eval. JSOP_SETCALL does the call, and then always throws.

(As an aside, having that eleminc there following code that pushes only one value gives me the creeps...)
(In reply to comment #16)
> (In reply to comment #10)
> > I think this is doing the right thing. We don't want to call eval('g()') and
> > let the eval have whatever effects it wants, and then return a non-reference,
> > and only then throw. We simply must avoid an early error (compile-time
> > all-paths error checking breaks IE-only calls-as-lvalues).
> 
> We already do call the eval. JSOP_SETCALL does the call, and then always
> throws.

You're right -- so what is causing the bug you wrote a testcase to demonstrate?

> (As an aside, having that eleminc there following code that pushes only one
> value gives me the creeps...)

JSOP_SETCALL used to push two stack items, but since it always throws, it pushes nothing ;-). It does not bother to adjust the stack pointer, even (hmm, that could be a problem -- cc'ing igor).

/be
(In reply to comment #17)
> (In reply to comment #16)
> > We already do call the eval. JSOP_SETCALL does the call, and then always
> > throws.
> 
> You're right -- so what is causing the bug you wrote a testcase to demonstrate?

Duh, just indirect eval. Will adjust test and add fix in v4 patch.

/be
(In reply to comment #17)
> You're right -- so what is causing the bug you wrote a testcase to demonstrate?

I'm confused. Here's how the current code behaves:

js> function g() { print("bleah") }
js> function f() { function g() { throw('food'); } eval('g()')++; } 
js> f()
bleah
typein:2: SyntaxError: invalid assignment left-hand side
js> 

It does so exactly *because* JSOP_SETCALL performs the call. The eval is incorrectly deemed indirect, and the "bleah" definition is used. It is always JSOP_SETCALL that throws, never the eval'ed code.

(In reply to comment #11)
> Why don't we?  Per 11.3.1 we 1) call the eval function to produce a result, 2)
> don't throw because that result's not an attempted modification of an eval or
> arguments binding, 3) get a value from the result and call ToNumber on it, 4)
> add 1 to that, then 5) try to assign that value to the result of the eval
> function but throw because it's not a Reference.  The spec seems reasonably
> clear about what must be done.

Except that ES5 Chapter 16 actually requires us not to call the function. To comply with ES5, we would need to throw an error, not call the eval code. We should deviate from ES5 by treating the error as non-early, not by doing the eval. So I think the fix for the put-to-non-reference cases is simply to change JSOP_SETCALL to always throw a BAD_LEFTSIDE_OF_DONKEY --- delete the Invoke call.

(I think this contradicts things I've said above; please treat this comment as my current understanding if so.)
(In reply to comment #18)
> Duh, just indirect eval. Will adjust test and add fix in v4 patch.

Okay, great.
My eyes failed to see the "bleah" output in my tiny eye-strain-o-vision terminal font -- so I thought we were throwing before eval'ing. Better get my eyes checked :-/.

> To comply with ES5, we would need to throw an error, not call the eval code. We
> should deviate from ES5 by treating the error as non-early, not by doing the
> eval. So I think the fix for the put-to-non-reference cases is simply to change
> JSOP_SETCALL to always throw a BAD_LEFTSIDE_OF_DONKEY --- delete the Invoke
> call.

So now that you know I missed the "bleah" and thought we did this, your argument is why I tried to say "works already". Waldo argued back and apart from the spec requiring something in Chapter 16 that is not (we think) web-compatible, and which makes for less interoperation (IE has Reference-returning built-ins, other browsers do not), I think he has a point.

Probably the right thing is to fix the direct -> indirect eval change and not try to throw a late-but-earlier-than-the-call error. Given that we can't do the early error, the rest of ES5 is clear: the call happens, and then only if it didn't return a Reference (no call in our system can), we throw.

/be
Yeah, I think that's probably right. Those puts are always erroneous; the only effect of Ch. 16's language is to make them early. Since we have decided not to make them early, the fallback position is to make them late, which entails doing the eval as per ES5 --- meaning direct.
Attached patch fix, v4 (obsolete) — Splinter Review
JSOP_SETCALL is now a suffix op emitted after JSOP_{CALL,EVAL,FUN{APPLY,CALL}}, which avoids loss of direct eval and simplifies things generally. It now truly is the op that always and only throws.

I did not fix bug 609756, but that is easier to fix now. It requires reordering eleminc to come before setcall, e.g.

/be
Attachment #488353 - Attachment is obsolete: true
Attachment #488617 - Flags: review?(jimb)
Attachment #488353 - Flags: review?(jimb)
Attached patch fix, v4a (obsolete) — Splinter Review
Ok, I would be shocked if some XUL JS that is fastloaded used JSOP_SETCALL, which changed arity, but just in case, bump JSXDR_BYTECODE_VERSION.

/be
Attachment #488617 - Attachment is obsolete: true
Attachment #488625 - Flags: review?(jimb)
Attachment #488617 - Flags: review?(jimb)
Priority: P2 → P1
Comment on attachment 488625 [details] [diff] [review]
fix, v4a

js> function f() { return 42; }  
js> function g() { f() = 3; }    
js> g()
typein:2: SyntaxError: invalid assignment left-hand side
js> 

That should be a ReferenceError. I think that's the case even when it's detected early, so just changing js.msg should be enough.

Doesn't this fix bug 609756, too?

js> function f() { print("hi"); }
js> function g() { f()++; }
js> g()
hi
typein:2: SyntaxError: invalid assignment left-hand side
js>
Attachment #488625 - Flags: review?(jimb) → review+
(In reply to comment #25)
> Comment on attachment 488625 [details] [diff] [review]
> fix, v4a
> 
> js> function f() { return 42; }  
> js> function g() { f() = 3; }    
> js> g()
> typein:2: SyntaxError: invalid assignment left-hand side
> js> 
> 
> That should be a ReferenceError. I think that's the case even when it's
> detected early, so just changing js.msg should be enough.

We've always done SyntaxError -- will fix.

> Doesn't this fix bug 609756, too?
> 
> js> function f() { print("hi"); }
> js> function g() { f()++; }
> js> g()
> hi
> typein:2: SyntaxError: invalid assignment left-hand side
> js>

That bug should be specifically about the case jorendorff came up with at bug 609756 comment 10:

    var x = 'ok';
    function f() {
        return {valueOf: function() {x = 'ok';}};
    }
    try {
        eval("x = 'bad'; f()++;");
        throw "expected an error";
    } catch (exc) {
        assert(exc instanceof ReferenceError);
    }
    assertEq(x, 'ok');  // fails

I'll adjust the summary.

/be
Attached patch patch to commit (obsolete) — Splinter Review
Attachment #488625 - Attachment is obsolete: true
Attachment #489053 - Flags: review+
http://hg.mozilla.org/tracemonkey/rev/0f78c4619855

/be
Whiteboard: fixed-in-tracemonkey
Looks good to me!
http://hg.mozilla.org/mozilla-central/rev/0f78c4619855
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 871046
You need to log in before you can comment on or make changes to this bug.