Closed Bug 514568 Opened 11 years ago Closed 9 years ago

ES5 strict mode: eval code gets its own variable environment


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: jimb, Assigned: Waldo)


(Blocks 1 open bug)


(Whiteboard: [blocks ES5 strict mode][softblocker][fixed-in-tracemonkey])


(6 files, 5 obsolete files)

6.30 KB, patch
: review+
Details | Diff | Splinter Review
12.02 KB, patch
: review+
Details | Diff | Splinter Review
84.77 KB, patch
: review+
Details | Diff | Splinter Review
11.78 KB, patch
: review+
Details | Diff | Splinter Review
5.27 KB, patch
: review+
Details | Diff | Splinter Review
8.96 KB, patch
Details | Diff | Splinter Review
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).
Summary: ES5 strict mode: evol code gets its own variable environment → ES5 strict mode: eval code gets its own variable environment
Assignee: general → jim
blocking2.0: --- → beta5+
Can we get an update here?
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.
blocking2.0: beta5+ → beta6+
blocking2.0: beta6+ → betaN+
Assignee: jim → jwalden+bmo
OS: Linux → All
Hardware: x86 → All
Depends on: 592664
Blocks: 602994
Depends on: 604504
Working on this, making progress -- a log slog since all our name analysis is function-based now, and disentangling that is not quite trivial.
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.
Patch in progress, or just design sketch? Not looking to reboot anything but it could be that Jim or I make a constructive comment.

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.)
Depends on: 614325, 614333
Depends on: 614338
Depends on: 616294
Depends on: 614493
Whiteboard: blockes ES5 strict mode
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 ;-) ).
"expose bugs in the patch", I meant.  (There will definitely be bugs aplenty in the tests -- definitely have been so far!)
Depends on: 620130
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.
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.
Okay, I think this is good to go now.
Attachment #499037 - Attachment is obsolete: true
Attachment #500938 - Flags: review?(igor)
Attached patch 3 - TestsSplinter Review
I haven't run these against any other engines yet, but I'm pretty confident in them.
Attachment #499040 - Attachment is obsolete: true
Attachment #500940 - Flags: review?(igor)
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.
Attachment #499041 - Attachment is obsolete: true
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.
Attachment #499050 - Attachment is obsolete: true
Whiteboard: blockes ES5 strict mode → blocks ES5 strict mode
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.
Attachment #500938 - Flags: review?(igor) → review+
Whiteboard: blocks ES5 strict mode → [blocks ES5 strict mode][softblocker]
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.
Attachment #500939 - Flags: review?(igor) → review+
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.
Attachment #500940 - Flags: review?(igor) → review+
Whiteboard: [blocks ES5 strict mode][softblocker] → [blocks ES5 strict mode][softblocker][fixed-in-tracemonkey]
Target Milestone: --- → mozilla2.0b9
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:
Whiteboard: [blocks ES5 strict mode][softblocker][fixed-in-tracemonkey] → [blocks ES5 strict mode][softblocker]
I have no idea how this patch managed to work for me without any of this in place.
Attachment #501416 - Flags: review?(igor)
(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 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.
Attachment #501416 - Flags: review?(igor) → review+
Attachment #501417 - Flags: review?(igor) → review+
Whiteboard: [blocks ES5 strict mode][softblocker] → [blocks ES5 strict mode][softblocker][fixed-in-tracemonkey]
I backed this out because of tinderbox failures.
Whiteboard: [blocks ES5 strict mode][softblocker][fixed-in-tracemonkey] → [blocks ES5 strict mode][softblocker]

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/
Whiteboard: [blocks ES5 strict mode][softblocker] → [blocks ES5 strict mode][softblocker][fixed-in-tracemonkey]
Depends on: 624100
Depends on: 624110
You need to log in before you can comment on or make changes to this bug.