446026 - restore utility of eval(s, o)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Brian Crowder]

Bug 442333 remove eval(o, s) entirely, but parts of it are useful.  With appropriate restrictions we should find a way to restore this feature (perhaps under another function name, other than eval).

I've proposed 1.9.1 as the target milestone for this work.

Comment 1

[Brian Crowder]

It sounds like what's hoped-for here is something like evalInSandbox() for content.  I'm adding Jesse since he may have some thoughts on this.

Comment 2

[Brendan Eich [:brendan]]

See bug 442333 comment 17 and bug 442333 comment 20.

/be

Comment 3

[Brian Crowder]

Sorry, I'm still not sure from those examples precisely what behavior is needed here.

Comment 4

[Brendan Eich [:brendan]]

The behavior that used to be there, on any Firefox 3.0.x release.

For 3.1 the idea in the latter part of bug 442333 comment 19 is still worth trying. That was what I thought this bug was for, with nothing touching a release until this bug was fixed. That's why I wrote

> Regressing and then restoring "later" risks never. If this is a bad enough
> change for some people, we should avoid regressing.

in bug 442333 comment 27.

/be

Comment 5

[Brian Crowder]

Does that wrapping change not still incompatibly break extensions with wonky code like the code in bug 457068?

Comment 6

[Brian Crowder]

Attachment: What I think Brendan is proposing

This is probably wrong, but I want to see if it's the right -idea-.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 340588 [details] [diff] [review]
What I think Brendan is proposing

This looks right, mrbkap should review too. Testing is the thing.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 340588 [details] [diff] [review]
What I think Brendan is proposing

I think this looks good.

Comment 9

[moz_bug_r_a4]

In the eval(s, o) case, the proposed code creates a With object using o as
parent, not proto.  But it regresses the private variables problem.

Also, with the patch, eval(s, o) clobbers caller's scope chain with o.

eval("", {alert:1});
alert(1);

With the patch, this throws a type error "alert is not a function".

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 340588 [details] [diff] [review]
What I think Brendan is proposing

New patch coming?

/be

Comment 11

[Brian Crowder]

Comment on attachment 340588 [details] [diff] [review]
What I think Brendan is proposing

Yeah, back to the drawing board.

Comment 12

[Brian Crowder]

Attachment: v2

Ok, with this patch the original problem is pretty straightforward.  I still end up making the With object's parent be the Function object in the "private variables" case.  The proto is the global object.  This basically doesn't solve the problem.

My next attempt was to make the parent object be null in the argc >= 2 case, but this totally breaks the o(s, environmentObject) because again, the proto is the global object, and there is no parent.

If I make the With object's proto be the original scopeobj, we're back in the same boat on the private variables case; you'll still be able to invade functions.  The scope-chain clobbering was a bug that I have since fixed.  New patch coming that does this (still leaves Peter Michaux's private variables case broken).

Comment 13

[Brian Crowder]

(In reply to comment #12)
> Ok, with this patch the original problem is pretty straightforward.

Rather, with the original patch....

Comment 14

[moz_bug_r_a4]

>+        if (indirectCall || argc >= 2) {
>+            callerScopeChain = scopeobj ? scopeobj : js_GetScopeChain(cx,
caller);

1347 out:
1348 #if JS_HAS_EVAL_THIS_SCOPE
1349     /* Restore OBJ_GET_PARENT(scopeobj) not callerScopeChain in case of
Call. */
1350     if (setCallerScopeChain) {
1351         caller->scopeChain = callerScopeChain;

The scope chain is still clobbered after calling eval(s, o).

>+                if (scopeobj)
>+                    scopeobj = js_NewWithObject(cx, scopeobj, NULL, -1);

js_NewWithObject(cx, scopeobj, NULL, -1) creates a With object whose parent is
the scopeobj's parent, thus the private variables problem is not solved.  What
happens if we create a With object using o's global object as parent?

Comment 15

[Brian Crowder]

I have a patch in my queue that fixes both of the items from comment #14.  Will post shortly.

Comment 16

[Brian Crowder]

Attachment: v3

The following testcase:

====================
var b = 45;

// Getting "private" variables
var obj = (function() {
  var a = 21;
  return {
    // public function must reference 'a'
    fn: function() {a;}
  };
})();

var foo;

try {
  eval('bar = b; foo=a', obj.fn);
} catch (e) {
  print("caught: " + e);
}
print(foo + " | " + bar); // 21

eval("", {print:1})
print(1);
====================

Prints:
caught: ReferenceError: a is not defined
undefined | 45
1

in the shell.  I believe this is correct for the cases tested.

Comment 17

[moz_bug_r_a4]

With v3 patch, the scope chain is clobbered with o's global object.

testcase:
var x = "global";
(function() {
  var x = "local";
  (function() {
    print(x);
    eval("", {});
    print(x);
  })();
})();

Prints:
local
global

Comment 18

[moz_bug_r_a4]

Do we really need to touch caller->scopeChain?  In the old code, we did not
touch caller->scopeChain at all in the eval(s, o) case, right?  So, I'm
wondering if what we need to do would be only wrapping o in a With object.

Something like this?:

    if (!scopeobj) {
        ...
    }
    else {
        ok = js_CheckPrincipalsAccess(cx, scopeobj,
                                      caller->script->principals,
                                      cx->runtime->atomState.evalAtom);
        if (!ok)
            goto out;

        scopeobj = js_NewWithObject(cx, scopeobj,
                                    JS_GetGlobalForObject(cx, scopeobj), -1);
        if (!scopeobj) {
            ok = JS_FALSE;
            goto out;
        }
    }

Comment 19

[Robert Sayre]

Does this bug need to block Firefox 3.1?

Comment 20

[Brendan Eich [:brendan]]

This is an API break. Old API to boot. We should try to fix, which is what I was insisting on in the other bug.

/be

Comment 21

[Brian Crowder]

Attachment: v4

Comment 22

[Brian Crowder]

Comment on attachment 349247 [details] [diff] [review]
v4

Basically the organization you suggested.  Please check it out, thanks.

Comment 23

[Brian Crowder]

So the patch in v4 seems to address the original Peter Michaux issue (privacy violation through closures).  I'm not sure, though, whether or not it will save Venkman or other extensions; if they're using the Michaux-style privacy violation intentionally, it surely won't.

Comment 24

[moz_bug_r_a4]

v4 patch looks good to me.  (I cannot edit the review flag.)

By wrapping o in a With object, we skip __parent__ regardless of whether or not
it is a Call object.  So there is a difference from the old behavior:

  //document.body.__parent__ is document
  alert(eval("body", document.body));

  Fx3.0.1:
  [object HTMLBodyElement]

  Trunk with v4 patch:
  ReferenceError: body is not defined

Is this a problem?

Comment 25

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #24)
> v4 patch looks good to me.  (I cannot edit the review flag.)

I've granted your account the necessary privileges, you should be able to edit it now.

Comment 26

[Brian Crowder]

Comment on attachment 349247 [details] [diff] [review]
v4

mrbkap:  Any thoughts on comment #24?  I think that's the -intent- of using the With object here, but I also think it constitutes an API-breakage.  Perhaps less severe?

Comment 27

[Caio Tiago Oliveira (asrail)]

Does Venkman rely on obj.__parent__?

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 349247 [details] [diff] [review]
v4

>--- mozilla.5b81a5dc7485/js/src/jsobj.cpp	2008-11-20 11:21:51.000000000 -0800
>+        ok = js_CheckPrincipalsAccess(cx, scopeobj,
>+                                      caller->script->principals,
>+                                      cx->runtime->atomState.evalAtom);

I think this should move after the computation of |principals| and use that value, since this will incorrectly report errors for cloned function objects whose original functions were compiled as chrome (or without principals, as we sometimes do).

Comment 29

[Robert Sayre]

ping? is this ready?

Comment 30

[Brian Crowder]

Attachment: per discussion from IRC

Another swing; this should agree with our IRC chat.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 352640 [details] [diff] [review]
per discussion from IRC

Thanks.

Comment 32

[Reed Loden [:reed]]

So, this is fixed-in-tracemonkey but checkin-needed... Should it still be landed on the trunk by itself, or should it wait until another tracemonkey landing is done?

Comment 33

[Brian Crowder]

No one ever answered the question from comment #27....

reed:  I don't know when the next merge from tracemonkey will be.  If it's in the next day or so, then I think waiting for that is fine.

Comment 34

[Robert Sayre]

merged in mc

Comment 35

[Serge Gautherie (:sgautherie)]

(In reply to comment #34)
> merged in mc

That was
http://hg.mozilla.org/mozilla-central/rev/e905fe64c1b2

(In reply to comment #33)
> No one ever answered the question from comment #27....
> 
> reed:  I don't know when the next merge from tracemonkey will be.  If it's in
> the next day or so, then I think waiting for that is fine.

Same question(s) for 1.9.1 checkin: wait for global or do individual ?

Comment 36

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/67944d1b207d

Comment 37

[Bob Clary [:bc] (inactive)]

Checking in js1_5/Scope/regress-446026-01.js
Checking in js1_5/Scope/regress-446026-02.js
http://hg.mozilla.org/mozilla-central/rev/55ae5a45a1d1

Comment 38

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 39

[Daniel Veditz [:dveditz]]

Since this was broken in a 1.9.0.x point release should we also land the fix on 1.9.0.x?

Comment 40

[Samuel Sidler (old account; do not CC)]

We won't block on this for now.

Brendan: Is this something we should land on the 1.9.0 branch (keeping in mind bug 446030 and ensuring it stays fixed)?

Comment 41

[Brendan Eich [:brendan]]

How did we survive not fixing this on the 1.9.0 branch? Are the add-ons authors who complained using only the bleeding edge, and never looking back? Do they have no users on the last stable release?

/be

Comment 42

[Brian Crowder]

No idea, no one ever responded to my queries in bug 457068