514568 - ES5 strict mode: eval code gets its own variable environment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

From ES5 Annex C:

Strict mode eval code cannot instantiate variables or functions in the variable environment of the caller to eval. Instead, a new variable environment is created and that environment is used for declaration binding instantiation for the eval code (10.4.2).

Comment 1

[Robert Sayre]

Can we get an update here?

Comment 2

[Jim Blandy :jimb]

I haven't started on this. I will be as soon as I get the patches for bug 514574 and bug 537873 up for review.

Comment 3

[Jeff Walden [:Waldo]]

Yoink.

Comment 4

[Jeff Walden [:Waldo]]

Working on this, making progress -- a log slog since all our name analysis is function-based now, and disentangling that is not quite trivial.

Comment 5

[Jeff Walden [:Waldo]]

I should note that most of the other stuff but the name analysis (and subsequent use of it) is (I think) in good shape.  A bit ago I had a strawman patch that made things work correctly (but not performantly -- I was using a plain old object as varobj rather than a Call object) for the eval-in-strict-mode-for-global-code half of this, so stupid stuff like this worked:

  "use strict";
  eval("var x = 2");
  assertEq(typeof x, "undefined");

Of course you need the function complement to ship, but at least I know the general approach was moderately sensible.

Comment 6

[Brendan Eich [:brendan]]

Patch in progress, or just design sketch? Not looking to reboot anything but it could be that Jim or I make a constructive comment.

/be

Comment 7

[Jeff Walden [:Waldo]]

Well, the basic idea was to move JSFunction::u::i::names into JSScript, add a dash of abstraction beyond just exposing raw Shapes, do the work to make that happen and work for the script-corresponds-to-function cases that we already have, then (separate patch for readability and bisect-ability) make strict eval code also generate its own list of bindings.  Mostly "straightforward", just involves a dash of refactoring and hopefully not too many subtle bugs for the variety of different code types this would necessarily and unavoidably affect.

A wrinkle I've just run into with this, however, is that the name set is clearly not const, because it ends in a runtime-generated (and JSRuntime-scoped) emptyCallShape Shape.  I could make the empty script non-const, generate it at runtime, and be on my way again.  But should I?  I don't see why being able to encode scripts for storage in read-only memory is important except in a mini-optimization sense, but maybe I'm missing something.  (The long-term goal of exposing only const JSScript * is still nice, but it doesn't require that it be possible to open-code a JSScript with a { } literal.)

Comment 8

[Jeff Walden [:Waldo]]

I might actually have a patch that implements this now!  \o/ \o/ \o/  ( \o/ )

I'd started writing some fairly intensive tests a bit ago, now I'm cycling back to writing them, adjusting the ones I'd written where they're buggy, and so on.  This is really nitpicky, so it's going to take awhile to get them all done, and it is very likely they will expose bugs in the tests.  But I see a light at the end of the tunnel now, and it looks too bright to be an oncoming train (unless I'm much closer to being hit than I think ;-) ).

Comment 9

[Jeff Walden [:Waldo]]

"expose bugs in the patch", I meant.  (There will definitely be bugs aplenty in the tests -- definitely have been so far!)

Comment 10

[Jeff Walden [:Waldo]]

So in reviewing said patch during cleanup, it turns out the reason it works is that I'm not binding top-level names (still have a true || before the call to the method that would perform such binding), so they'd have to get resolved into place at runtime.  Maybe that's enough to get it working as a first pass.  But I'll see how much trouble I have to go to to optimize this, to see if I might as well go that extra step right now.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: 1 - Refactor name-binding code a bit to more clearly separate out top-level binding from non-top-level binding

Comment 12

[Jeff Walden [:Waldo]]

Attachment: 2 - Give strict mode eval frames a Call object, implementing strict mode eval binding semantics

Comment 13

[Jeff Walden [:Waldo]]

Attachment: 3 - A start on tests, some completed, some incomplete, doubtless more needed

Comment 14

[Jeff Walden [:Waldo]]

Attachment: 4 - Remove CallPropertyOp and inline it (with improvements) into its old callers

Comment 15

[Jeff Walden [:Waldo]]

Attachment: 5, incomplete - Optimize name binding for top-level names in strict mode eval code

Comment 16

[Jeff Walden [:Waldo]]

Now for the explanations:

Patches 1-3 that I just posted fix this bug, as far as I have been able to tell.  They build upon the patches in bug 614493, currently partway through the review process toward landing.

Patch 1 just helps with code organization, slightly -- I think it clarifies the binding logic a bit more, and it makes it more obvious where you'd insert special strict mode behavior at top level in eval code.

Patch 2 fixes this bug.  This mostly involves using the script bindings to construct a Call object for the frame.  This patch doesn't optimize top-level names.  Instead it relies on (I assume) JSOP_NAME plus the existence of the Call object to implement strict mode behavior.  This is easy.  It's also somewhat slow.  But for the immediate future I don't think that matters, as strict mode use (and strict mode eval use) is still thankfully rare.  This seems like something worth optimizing when the scope chain is rewritten.

Patch 3 is a start on tests.  I tried to be exhaustive in testing all the different ways eval can be used, at least as far as the spec is concerned.  The tests for eval called from a function, and eval called from global code, all pass -- strong evidence that patch 2 generally fixes this bug.  There might be optimizations I'm not aware of where this breaks in some edge cases, of course -- I'd be astounded if there weren't -- but the lion's share is clearly done.  The tests for eval called from "nothing" (web equivalent: setTimeout, setInterval, etc.) aren't written yet.  We don't have a shell primitive to test this, and I haven't designed or written one yet to use in these tests.  But it seems better to get everything out, even preliminarily, than to keep sitting on all this while working on the called-from-nothing case, which probably can be punted if required for time.

Patches 4 and 5 are not necessary, but they are improvements in this bug's vein which build on the first three.

Implementation complete, I next considered optimization per comment 10.  I quickly ran into the issue of [GS]etCallVar assuming a Call object.  To examine this behavior and potentially combat it, I needed to remove CallPropertyOp and put the real code into all its callers.  Patch 4 inlines the code from CallPropertyOp into all its callers.  So it's not a functionality change, just a readability change.  Except when it's not!  The current code, in the interest of generality, foregoes a lot of assertions it could be making, to perform range-checking and the like.  This refactoring adds those assertions, making this code both more readable and safer.  I don't think patch 5 is going to happen this release, so technically this patch isn't required.  But we should still take it now for the safety improvements and verifications it provides.

Last, patch 5.  Just performing binding in strict mode eval code isn't enough to fix this, because you run into a bunch of assertions that things like SetCallVar are only called on Call objects.  And Call objects rely on having a function around for getting the right offset to vars (after the argument count).  You could hack around some, perhaps even most of these things -- or you could introduce a new EvalCall class.  This would also let you do things like null out the addProperty and deleteProperty hooks (and stub out resolve) to catch possible bugs.  This turns into a chunk of work, and I'm only partway done, and I'm not sure I will finish now.  And it really isn't necessary -- JSOP_NAME isn't all that bad for this very-rare case, and we only need "correct" (not "fast") to release.

Anyway, that's the state of these patches.  More to come as bugs blocking this get resolved, but I did want to post what I have to get it out there, and to not leave this bug worrisomely un-attended-to.

Comment 17

[Jeff Walden [:Waldo]]

Attachment: 1 - Refactor name-binding code a bit to more clearly separate out top-level binding from non-top-level binding

Okay, I think this is good to go now.

Comment 18

[Jeff Walden [:Waldo]]

Attachment: 2 - Give strict mode eval frames a Call object, implementing strict mode eval binding semantics

Comment 19

[Jeff Walden [:Waldo]]

Attachment: 3 - Tests

I haven't run these against any other engines yet, but I'm pretty confident in them.

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 499041 [details] [diff] [review]
4 - Remove CallPropertyOp and inline it (with improvements) into its old callers

I moved this into bug 622053, since fixed.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 499050 [details] [diff] [review]
5, incomplete - Optimize name binding for top-level names in strict mode eval code

I don't think we have time to make this *fast*, with everything else yet to do for release.  Correct but not fast is good enough for now.

Comment 22

[Igor Bukanov]

Comment on attachment 500938 [details] [diff] [review]
1 - Refactor name-binding code a bit to more clearly separate out top-level binding from non-top-level binding

>+static bool
>+BindLocalVar(JSContext *cx, BindData *data, JSAtomListElement *ale, JSParseNode *pn,
>+             JSAtom *varname, JSTreeContext *tc)

Nit: "Var" in the name is misleading as the function also binds argument names. A better name would be BindFunctionLocal.

Comment 23

[Igor Bukanov]

Comment on attachment 500939 [details] [diff] [review]
2 - Give strict mode eval frames a Call object, implementing strict mode eval binding semantics

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>+    if (script->strictModeCode) {
>+        initialVarObj = NewCallObject(cx, &script->bindings, *initialVarObj, NULL);
>+        if (!initialVarObj)
>+            return false;
>+        initialVarObj->setPrivate(frame.fp());
>+
>+        if (frame.fp()->hasCallObj())
>+            frame.fp()->clearCallObj();

Nit: Comment before the "if" when frame.fp()->hasCallObj() is true.

Comment 24

[Igor Bukanov]

Comment on attachment 500940 [details] [diff] [review]
3 - Tests

This is really nice set of tests. Still it would be even better to haver a coverage for eval overwriting a function definition like function x() {}; eval('var x;')" or vise-versa. But this cab wait another bug.

Comment 25

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/3f2dc349aec9
http://hg.mozilla.org/tracemonkey/rev/d77e1225006b
http://hg.mozilla.org/tracemonkey/rev/bb5f9aa0356d

Comment 26

[Jeff Walden [:Waldo]]

Oops, call_trace is wrong for strict mode eval frames, and I wasn't seeing it in shell tests because they weren't managing to trace this case (I guess, something funky about this).  Fixing that to use |fp->script()| rather than |fp->fun()->script()| fixes that immediate problem but then points out that ScriptEpilogue also needs some changes.

None of that is super-complicated, but it was more than I wanted to do in a time-constrained orange fix, so I hacked out strict mode eval call object creation and will reenable it when I finish up that work:

http://hg.mozilla.org/tracemonkey/rev/b4552d5b76ef

Comment 27

[Jeff Walden [:Waldo]]

Attachment: Followup 1 - Trace Call objects correctly, keep eval scripts rooted, and put eval Call objects

I have no idea how this patch managed to work for me without any of this in place.

Comment 28

[Jeff Walden [:Waldo]]

Attachment: Followup 2 - Add a better idiom for checking the purpose of a Call object

Comment 29

[Igor Bukanov]

(In reply to comment #27)
> Created attachment 501416 [details] [diff] [review]
> Followup 1 - Trace Call objects correctly, keep eval scripts rooted, and put
> eval Call objects

Can you attach diff -b version of the patch?

Comment 30

[Jeff Walden [:Waldo]]

Attachment: Followup 1, diff -b

Comment 31

[Igor Bukanov]

Comment on attachment 501428 [details] [diff] [review]
Followup 1, diff -b

>diff --git a/js/src/jsinterpinlines.h b/js/src/jsinterpinlines.h
>@@ -722,13 +722,26 @@ ScriptEpilogue(JSContext *cx, JSStackFra
>+    if (fp->isEvalFrame() && fp->script()->strictModeCode) {
...
>+    } else {
>+        /*
>+         * Otherwise nly function frames have activation objects.  The parent

Nit: s/nly/only

>     if (fp->isFunctionFrame() && !fp->isEvalFrame() && !fp->isYielding())

Nit: to simplify code following split the initial "if" like 

if (fp->isEvalFrame()) {
    if (fp->script()->strictModeCode) {
    }
} else {

to avoid !fp->isEvalFrame() in the second if and adjust/move comments accordingly.

Comment 32

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/2e94f0b8d03c
http://hg.mozilla.org/tracemonkey/rev/feb6682a10af

Comment 33

[Robert Sayre]

I backed this out because of tinderbox failures.

Comment 34

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/1417c3808f04
http://hg.mozilla.org/tracemonkey/rev/27a0e3715095
http://hg.mozilla.org/tracemonkey/rev/01b0818e6763

Landed!  And stuck!

For future reference, the problems with the changes in comment 32.

First, Execute is used for more than just eval frames, so the if (strict) check had to be converted to an if (eval && strict) check.  Without that change non-eval evaluation frames got a call object whose private never got cleared, which occasionally resulted in erratic crashes (like all the other ones here) when we tried tracing that object way after the fact with a frame with a long-dead JSScript.

Second, the changes to ScriptEpilogue needed to be copied into js::mjit::Compiler::emitReturn, which I'd wrongly thought wasn't necessary but which clearly was, on closer inspection.  Again I don't know why this worked at all without this change.

But it's landed!  About time.  \o/

Comment 35

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/01b0818e6763
http://hg.mozilla.org/mozilla-central/rev/27a0e3715095
http://hg.mozilla.org/mozilla-central/rev/1417c3808f04