Closed Bug 915805 Opened 7 years ago Closed 5 years ago

eval invoked from within new Function(...)() has different scope than within outer eval

Categories

(Core :: JavaScript Engine, defect)

24 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: eric.promislow, Assigned: Waldo)

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

I compared the results of this one line of JS in two different js shells, from Moz 18 and Moz 24.

The line of code:
js> new Function("delete x; eval('var x = 42'); print(x)")()


Actual results:

typein:1:30 ReferenceError: x is not defined


Expected results:

42

Both versions give the same result (42) for
eval("delete x; eval('var x = 42'); print(x)")
Version: 18 Branch → 24 Branch
Assignee: nobody → general
Component: General → JavaScript Engine
OS: Windows 7 → All
Hardware: x86_64 → All
Attached patch Patch and testSplinter Review
The conditional there was getting way unwieldy, so I split it up a bunch.  The only bit needed to fix the bug is the addition of |funbox->hasExtensibleScope()| to the is-function checking.
Assignee: general → jwalden+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #804051 - Flags: review?(jorendorff)
That patch fixes the problem.
Comment on attachment 804051 [details] [diff] [review]
Patch and test

Review of attachment 804051 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review flag for now; I don't think this is the right fix.

If you want to land the cleanup right away, without the fix or the test, r=me for that.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1185,5 @@
> +        return false;
> +
> +    // Deoptimized names also aren't necessarily globals.
> +    if (pn->isDeoptimized())
> +        return false;

I think this is the case that should have hit. Why wasn't the name node in question deoptimized when we found out the scope was subject to direct eval?

Other things depend on isDeoptimized(). Are they broken too?

Alternatively, can we ditch PND_DEOPTIMIZED entirely, in favor of other information we're already tracking?

@@ +1220,5 @@
> +    switch (pn->getOp()) {
> +      case JSOP_NAME:     op = JSOP_GETGNAME; break;
> +      case JSOP_SETNAME:  op = JSOP_SETGNAME; break;
> +      case JSOP_SETCONST:
> +        /* Not supported. */

Convert to // comment style?
Attachment #804051 - Flags: review?(jorendorff)
Landed the cleanup bits, with the bug-fixing/patch aspects removed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/180712bf3fbd

PND_DEOPTIMIZED is primarily used for names inside with-statements (irrelevant to Function).  It should have been set at Parser.cpp:1365 -- except that Parser::leaveFunction is never called for Function() code (anything that uses Parser::standaloneFunctionBody, to be precise).  This deoptimize gunk is the only bit of Parser::leaveFunction that'd do anything of interest in Parser::standaloneFunctionBody.  (Note that Parser::leaveFunction generates function bindings, but Parser::standaloneFunctionBody has a separate call covering that concern.)

All of which militates, I think, for making Parser::standaloneFunctionBody call Parser::leaveFunction, for consistency at the very least.  That'd fix the deoptimized concern here, if we trust PND_DEOPTIMIZED.  I don't -- its meaning is vague and under-defined, and we should remove it.  But that's more than I have time to do, immediately, here, so if we went that route, this bug would probably end up sticking around "awhile".

So I'm inclined to say we should land the mini-fix here, to fix the issue, and have followups to remove PND_DEOPTIMIZED.  isDeoptimized() is only used in BindNameToSlotHelper as a short-circuit, beyond the TryConvertFreeName use here.  Address that one use case, and the whole thing can go away.  But I don't have time to spare to figure that out now, ergo I think (not super-strongly, but enough, given this was reported "in the field") we should make the small change in this patch now.
Whiteboard: [leave open]
In https://hg.mozilla.org/mozilla-central/rev/180712bf3fbd why is there no
call to funbox->hasExtensibleScope() like there was in

https://bugzilla.mozilla.org/attachment.cgi?id=804051 ?
Specifically because comment 3 thought that wasn't the right fix, but was okay with the cleanup-y aspects of the change being landed.  (Note the commit message talks only about cleanup, not about fixing the issue reported here.)  We're still sort of discussing whether the fix, if not quite the right one, is nonetheless reasonable to take as a shorter-term expedient fix.  See comment 4.
Having looked a bit more at the paths where PND_DEOPTIMIZED is set, I'm now convinced that it used to be pretty coherent actually. PND_DEOPTIMIZED is supposed to mean "this Identifier can refer to a binding in a dynamic scope": either a with-block, or a scope that can be extended by direct eval or a function-statement. (It does not mean that isUsed() will be false, though!)

You know, we're stuck with isDeoptimized for the time being. We depend on it. It's dangerous for it to have a bug.

> So I'm inclined to say we should land the mini-fix here, to fix the issue, and have followups to remove
> PND_DEOPTIMIZED.  isDeoptimized() is only used in BindNameToSlotHelper as a short-circuit, beyond the
> TryConvertFreeName use here.

Awfully weird to have these two sentences together without a very convincing argument that the other call site does not also cause bugs!

Not fixing the actual bug would just come back to bite us.

You don't have to call leaveFunction to do it. Make a copy called leaveStandaloneFunction and blow away everything except the 10 lines we need. Should be pretty quick and clean, I think.
Attached patch Patch + Test (obsolete) — Splinter Review
In Parser.cpp, there are 2 ways functionBody can be invoked. One is from functionArgsAndBodyGeneric (taken by function()) and the other is from standaloneFunctionBody (taken by the Function constructor).

After the functionBody call, the former goes through finishFunctionDefinition and leaveFunction, which do some things the latter forgets. I added the relevant code to standaloneFunctionBody, and it fixed the issue.
Attachment #8609611 - Flags: review?(jorendorff)
Comment on attachment 8609611 [details] [diff] [review]
Patch + Test

Review of attachment 8609611 [details] [diff] [review]:
-----------------------------------------------------------------

Great patch. I'm sorry for the slow review.

::: js/src/frontend/Parser.cpp
@@ +844,4 @@
>      if (!FoldConstants(context, &pn, this))
>          return null();
>  
> +    fn->pn_pos.end = pos().end;

Why is this better than the existing code, which says
    fn->pn_body->pn_pos = pn->pn_pos;
?

It seems not worth changing in the same patch as other things...

::: js/src/tests/ecma_5/Function/Function-with-eval.js
@@ +8,5 @@
> +var summary =
> +    "Local variables inside functions should resolve correctly even if the " +
> +    "definition is hidden inside an eval";
> +
> +print(BUGNUMBER + ": " + summary);

You can delete all this boilerplate if you want (lines 6-17). I usually do, because it's redundant with information that's available in the Mercurial history.

@@ +27,5 @@
> +                  "}');" +
> +            "return foo;"
> +         )()(),
> +         915805);
> +         

Please delete the whitespace on this blank line (line 31).
Attachment #8609611 - Flags: review?(jorendorff) → review+
Attached patch Patch + TestSplinter Review
> > +    fn->pn_pos.end = pos().end;
> 
> Why is this better than the existing code, which says
>     fn->pn_body->pn_pos = pn->pn_pos;
> ?
I added that line because I think this is the other thing that was forgotten. The function returned the fn object with pn_pos always (0,0), I don't think that should be the case. The function() path sets it like that at the beginning of finishFunctionDefinition.
I deleted "fn->pn_body->pn_pos = pn->pn_pos" since "fn->pn_body->append(pn)" already sets pn_pos.end (and start should already be set). This can again be seen in finishFunctionDefinition, which does just the append call.
 
> You can delete all this boilerplate if you want (lines 6-17). I usually do,
> because it's redundant with information that's available in the Mercurial
> history.
OK. I added the info as the commit message and I deleted the trailing whitespace. I hope I did it correctly.
Attachment #8609611 - Attachment is obsolete: true
Bug erroneously marked [leave-open] at merge time.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.