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.

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.

See bug 442333 comment 17 and bug 442333 comment 20. /be

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

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

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

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

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 on attachment 340588 [details] [diff] [review] What I think Brendan is proposing I think this looks good.

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 on attachment 340588 [details] [diff] [review] What I think Brendan is proposing New patch coming? /be

Comment on attachment 340588 [details] [diff] [review] What I think Brendan is proposing Yeah, back to the drawing board.

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).

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

>+ 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?

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

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.

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

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; } }

Does this bug need to block Firefox 3.1?

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 on attachment 349247 [details] [diff] [review] v4 Basically the organization you suggested. Please check it out, thanks.

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.

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?

(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 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?

Does Venkman rely on obj.__parent__?

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).

ping? is this ready?

Another swing; this should agree with our IRC chat.

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

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?

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.

merged in mc

(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 ?

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

v 1.9.1, 1.9.2

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

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)?

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

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