Per ES5, eval called directly -- that is, only by an expression of the form "eval(...)" -- behaves differently from other, indirect, eval calls. How should |otherWindow.eval|, called directly from script not associated with |otherWindow|, act? ES5 doesn't address the possibility of multiple globals, so there's no spec to follow, as far as I know. We treat cross-compartment eval calls as indirect, even if syntactically they are direct calls (or we did before a patch for bug 604504 landed, but I'm pretty sure the removal of this treatment was wrong). But we inconsistently treat cross-window but not cross-origin eval calls as direct when they are syntactically direct. So if you have an eval from another window -- sometimes -- and you use it the wrong way, it behaves exactly as though it were *your* window's eval, which seems quite unexpected. What do other browsers do? Opera treats it as though it were an indirect eval occurring in the other window. WebKit and Chrome throw an EvalError (!) that the "this" object passed to eval must be the global object from which eval originated (blatantly wrong for direct eval calls in strict mode code). IE9 (after I add a talismanic HTML5 doctype, grumble) does what Opera does. Since ES5 more or less wanted to remove EvalError, and probably would have if it weren't backwards-incompatible with little benefit, WebKit and Chrome's behavior is unacceptable. So let's implement what Opera and IE do -- it's consistent with our cross-compartment eval story (or rather, what that story used to be, and will be again), and it's consistent with some of the other browsers out there.

Report on what 3.6 does, and 3.5 and 3 if different (probably they all match but it would be good to check). It's pretty bogus to treat a de-jure spec that was finalized only in the last year as somehow more normative than what Firefox did for years. Even moreso other browsers' behavior. /be

On the plus side: good catches on compartment-related regressions. And this bug's summary is right. I believe it's what 3.6 and earlier did. /be

The compartment-related regressions are non-existent, as I just noted in that bug, because eval would have to be wrapped for that to happen (maybe this was partial-compartment detritus?). (This means there's an opportunity to add an assertion that cross-compartment calls are indirect, to be done shortly.) Maybe this is why jorendorff removed the check in his counter-proposal, which I copied/adopted/adapted, just wish I'd noticed it at the time. Don't we regularly consult what other browsers do when deciding what behavior to implement, when the standards are insufficient to answer a question? 3.6 and 3.0 both (likely 3.5 as well) behave the way trunk currently does. I hadn't bothered checking because I was pretty sure the code for this behavior had been around for awhile and hadn't changed in relevant ways. I suspect it goes back to the original direct/indirect-eval commit.

Consulting other browsers is third. Consulting the spec is second, and not optional. Consulting what we do is first. Ok, so first or second doesn't matter. I caught up on the other bug after commenting here. Ok, so we are doing the same thing we always did. If JSOP_EVAL is *caller->pc(cx) in obj_eval it is a direct call, even if to the other window's eval. I'm not sure what might depend on this but if we've been this way for a long time, then near the end of Fx4 is not a good time to try to see what we can change. Harmony or an Ecma TR before it (then Harmony) should spec multiple globals. We can work on that and implement it on trunk after 4 branches. I don't see any win in changing things now. In this case, what we did *does* come first, since there is no complete spec and a reading of ES5 with multiple globals in mind, however saner than what we have done, is not a standard yet. /be

obj_eval RIP -- getting used to searching for /^eval/ -- nice. /be

(In reply to comment #4) > I caught up on the other bug after commenting here. Ok, so we are doing the > same thing we always did. If JSOP_EVAL is *caller->pc(cx) in obj_eval it is a > direct call, even if to the other window's eval. To correct a subtle inaccuracy, that bug was not merely a transparent reorganization whose sole effect was to make eval-implementing code better structured (to ease some of the implementation of bug 514568). It also fixed the issue (of as much compatibility concern as this one, I believe) where, when calling a non-script, non-eval function by the eval name, which itself then called eval, the eval call would be a direct eval. (See jorendorff's request for tests plus their presence in the final landing.) It was while fixing that bug that jorendorff and I noticed the bizarre behavior chronicled in this bug. (Even better than ^eval, get used to searching for ^EvalK. eval is the ghetto of indirect eval only now, and very, very few sites are stupid enough to use indirect eval to hit it. :-) )

(In reply to comment #6) > (In reply to comment #4) > > I caught up on the other bug after commenting here. Ok, so we are doing the > > same thing we always did. If JSOP_EVAL is *caller->pc(cx) in obj_eval it is a > > direct call, even if to the other window's eval. > > To correct a subtle inaccuracy, Who was inaccurate where what now huh? :-| > that bug was not merely a transparent > reorganization whose sole effect was to make eval-implementing code better > structured (to ease some of the implementation of bug 514568). It also fixed > the issue (of as much compatibility concern as this one, I believe) where, when > calling a non-script, non-eval function by the eval name, which itself then > called eval, the eval call would be a direct eval. Yeah, I remember that. But what has it to do with this bug? > (Even better than ^eval, get used to searching for ^EvalK. eval is the ghetto > of indirect eval only now, and very, very few sites are stupid enough to use > indirect eval to hit it. :-) ) Then why is indirect eval named "eval" instead of IndirectEval? /be

(In reply to comment #7) > > > I caught up on the other bug after commenting here. Ok, so we are doing > > > the same thing we always did. If JSOP_EVAL is *caller->pc(cx) in > > > obj_eval it is a direct call, even if to the other window's eval. > > > > To correct a subtle inaccuracy, > > Who was inaccurate where what now huh? :-| I referred to "we are doing the same thing we always did", which I took to refer to the post-bug 604504 state as compared to that before it. > Yeah, I remember that. But what has it to do with this bug? Nothing, other than that you summarized it in your comment slightly differently from what actually happened, unless I misread your comment, and I wished to leave the details clear for future readers. > Then why is indirect eval named "eval" instead of IndirectEval? I abandoned the obj_ prefix because the anachronistic rationale of Object.prototype.eval hadn't applied for awhile, but it seemed sensible to implement the eval function by a method of exactly that name for easiest searching. I expect readers looking for eval, in any flavor, will most often search for that name, possibly exactly (as I would if I were examining the v8 or Nitro codebases for their eval implementations). (Someone looking for direct eval might do differently, but I doubt he'd search for "direct eval" or somesuch but instead would look for eval in the parse-tree and code-generation paths, to pick out that special form's implementation.) So IndirectEval or what-have-you seemed slightly worse than just adding the pointer comment at the start of the method. Does that make sense?

(In reply to comment #8) > (In reply to comment #7) > > > > I caught up on the other bug after commenting here. Ok, so we are doing > > > > the same thing we always did. If JSOP_EVAL is *caller->pc(cx) in > > > > obj_eval it is a direct call, even if to the other window's eval. > > > > > > To correct a subtle inaccuracy, > > > > Who was inaccurate where what now huh? :-| > > I referred to "we are doing the same thing we always did", which I took to > refer to the post-bug 604504 state as compared to that before it. I actually thought about recapping the wrong-pc-inspected sideshow before I wrote that comment, but axed it to cut to the main issue: not gratuitously deviating from what we've done at this point in the release cycle. > > Yeah, I remember that. But what has it to do with this bug? > > Nothing, other than that you summarized it in your comment slightly differently > from what actually happened, unless I misread your comment, and I wished to > leave the details clear for future readers. It's so painfully clear now that the main point is probably unclear :-/. > > Then why is indirect eval named "eval" instead of IndirectEval? > > I abandoned the obj_ prefix because the anachronistic rationale of > Object.prototype.eval hadn't applied for awhile, but it seemed sensible to > implement the eval function by a method of exactly that name for easiest > searching. I expect readers looking for eval, in any flavor, will most often > search for that name, possibly exactly (as I would if I were examining the v8 > or Nitro codebases for their eval implementations). It doesn't make sense for eval to be the name of the native for indirect eval, but EvalKernel to be the name for direct eval, in my view. If people don't know what indirect eval is, they may be looking for direct eval's implementation. Matching names: DirectEval, IndirectEval; seems better. Even making "eval" the name of the direct helper, now called EvalKernel, and giving indirect eval's impl a longer name, seems better (but not as good as balanced names). /be

If you wish that, file a bug. (And let's both stop cluttering this one with discussion that's not on-point. ;-) ) But, before you do, please consider whether it's really worth bikeshedding the name and spending time to change it -- seems very low value to me.

I may file a bug or just rename with rs=you, but let's settle the substantive point about the function names instead of first bringing it up (you did), then trying to dismiss it (you are). In JS: eval(s) is direct-eval, obj.eval(s) is indirect-eval. In C++: eval is the indirect-eval name. ?!? Or to use callee expressions, instead of EvalKernel for eval and eval for obj.eval, it would be better to use eval for eval and obj_eval for obj.eval. This is not bikeshedding because it's not about paint color. It's about parallel constructions in two languages. /be

Paint color arguments would be "OperatorEval" vs. "DirectEval" (some early specs and discussions called direct eval "operator eval", and don't we all wish it were an operator? The compiler specializes for it as if). Again, this is not my point, and not nearly as big a deal as parallel constructions, although we all care about good naming. The tangent started in this bug when I noticed obj_eval was gone and /^eval(/ was the new regex to use in jsobj.cpp -- and that led to the caution from Waldo that eval the native function was indirect eval, and direct eval matched /^EvalK/. It seems to me that this is a minor WTF that we should fix. Not major, not within epsilon of zero either. Anyone else have a suggestion? /be

I think having the indirect variant be called "IndirectEval" would be extremely helpful. To my mind, indirect and direct eval are both different kinds of eval --- there isn't a "real" version and a "strange" version --- so having anything in C++ named plain 'eval' leaves a question hanging. So I would keep EvalKernel. Trying to extend existing patterns is a little better than bikeshedding. I introduced math_atan2_kernel, but if there's better established precedent for "common behavior to all the variants", then we should add to the pattern, not create a new one.

Paragraph 1 of comment 13 seems right, a plain "eval" name in C++ is confusing. Kernel/_kernel is ok (I was a kernel hacker once ;-), perhaps better than the vague Helper convention we overuse elsewhere, which always reminds me of Sam Kinnison's hilariously sarcastic name for Rodney Dangerfield's character in this scene: http://www.youtube.com/watch?v=Xfi4s8cjLFI&feature=related "Mr. Helper", lol. /be

But given IndirectEval, the obvious peer-name to search for is DirectEval, so while this is all small potatoes, DirectEval seems better than EvalKernel by a nose. It's possibly less good from the point of view of parallel constructions if you expect both to be native methods, though -- that p.o.v. wants EvalKernel. So I will go with the flow on Direct- vs. -Kernel. /be

I'd like to reiterate my request to "stop cluttering this one with discussion that's not on-point". Again, if you think a name change is desirable, please file a bug. Regardless whether any name changes occur, it seems impolite to me to change discussion for this long to a topic not implicated by the original bug summary.

Waldo: why not make the name change in the patch for this bug? I'll file another bug if you want. /be

I noticed a theoretical concern while working through this as well. If a JSAPI user decided to brain-transplant eval, suddenly that eval function would no longer ever be usable as a direct eval, only indirect. I'm told transplanting is theoretically generic enough to happen to any object (well, if you understand its class, I guess), so perhaps that's a latent restriction to be placed on transplant users. Whichever way it should be, the concern goes away with this patch. Re past comments, again, I'd prefer not to mix multiple concerns in the same bug, or the same patch, when it's not necessary. Please?

Waldo: you are ignoring peer feedback (comment 13) -- if you want it in another bug, file that bug. On this bug's substantive (and costly enough that it's not ignorable: code size and another global reserved slot): get agreement on the multiple globals semantic issue here before pushing the patch. http://wiki.ecmascript.org/doku.php?id=strawman:multiple_globals is a placeholder dherman created. /be

(In reply to comment #20) > Waldo: you are ignoring peer feedback (comment 13) -- if you want it in another > bug, file that bug. I was just offering an opinion, which Waldo could find persuasive or not; I didn't mean to r- his patch.

There was a clear intent about eval when drafting the ES5 spec. that I believe applies even to the multiple global objects situation. Of course, this says nothing about legacy compat. issue for Firefox or the actual implementation design but it may help still be helpful to consider: The starting point for understanding direct eval in ES5 is that a direct usage of eval was envisioned as being essentially an operator of the language rather than an actual function call. This eval "operator" does things such as access and create bindings in the callers variable environment that no function can normally do without special (ie, non-standard) support from the implementation. The purpose of the definition of "direct eval" is to identify the situations where what syntactically looks like a function call is actually an application of the eval operator. Comment 1 said: > Per ES5, eval called directly -- that is, only by an expression of the form "eval(...)" -- behaves differently from other, indirect, eval calls. That is an incomplete paraphrasing of the ES5 spec, In addition to appearing in that syntactic form, the current value of eval must also be the "standard built-in function defined in 15.1.2.1". In other words, both the syntactic form and the appropriate eval function value is necessary in order to have a direct eval occurrence. The intent of the spec. language was that an implementation only needs to do whatever is necessary to support the special eval "operator" semantics at a call site if it see a direct call to the name "eval" and if it can ensure (via a runtime guard or a compile time proof) that at runtime the visible value of "eval" is the built-in function. "indirect eval" is the normal behavior of an arbitrary invocation of the built-in eval function. Every call site is potentially a indirect call to eval so an indirect eval call is just a normal function call. The exceptional case is "direct eval", and its implementation might occur directly at the call site and not even do a normal call to the built-in function. While ES5 does not have the concept of multiple global object we can still reason about what it should mean to receive an eval-like function from an indeterminate source and that reasoning should encompass the multiple-global scenario: if such a function is in any way distinguishable from the standard built-in eval function (including behaving differently when actually called from the current site, eg accessing a different global object) from the perspective of the call site then it is not a "direct eval" call. From the caller's perspective this is just a regular function call. From the callee's perspective (assuming it is the built-in eval from a different global context) it is an indirect eval. By this logic, cross global context eval calls should always be indirect evals. Of course, when TC39 actually tries to formalize the concept of multiple global environments and looks at the browser legacy we may end up somewhere else.

(In reply to comment #22) > That is an incomplete paraphrasing of the ES5 spec, In addition to appearing > in that syntactic form, the current value of eval must also be the "standard > built-in function defined in 15.1.2.1". In other words, both the syntactic > form and the appropriate eval function value is necessary in order to have a > direct eval occurrence. I think we are in complete agreement. I was approaching from the point of view of what happens when the eval function is called, not from the syntactic side of what happens when a direct-looking "eval" is called. I sent mail to es-discuss an hour and a half ago about this, which seems not to have gone through. I'll resend and hopefully it'll make it this time.

(In reply to comment #23) > (In reply to comment #22) > > That is an incomplete paraphrasing of the ES5 spec, In addition to appearing > > in that syntactic form, the current value of eval must also be the "standard > > built-in function defined in 15.1.2.1". In other words, both the syntactic > > form and the appropriate eval function value is necessary in order to have a > > direct eval occurrence. > > I think we are in complete agreement. I was approaching from the point of view > of what happens when the eval function is called, not from the syntactic side > of what happens when a direct-looking "eval" is called. > > I sent mail to es-discuss an hour and a half ago about this, which seems not to > have gone through. I'll resend and hopefully it'll make it this time. Cool, then the issue isn't whether it is a direct or indirect call. It is which global object is used by the (indirect) eval call. I think there is a more general question here which is what TC39 need to clarify. Do built-in functions capture their global environment at the time of their creation or do they they operate in the dynamic context of an ambient global environment. I hope it is the former.

My es-discuss mail got spam-binned, but that's sorted out now: https://mail.mozilla.org/pipermail/es-discuss/2011-March/012915.html

(In reply to comment #22) > The starting point for understanding direct eval in ES5 is that a direct usage > of eval was envisioned as being essentially an operator of the language rather > than an actual function call. Sure -- this was in ES4 too, and it was based on the implementations in several browsers. > Comment 1 said: (Comment 0, rather.) > > Per ES5, eval called directly -- that is, only by an expression of the form > "eval(...)" -- behaves differently from other, indirect, eval calls. > > That is an incomplete paraphrasing of the ES5 spec, In addition to appearing > in that syntactic form, the current value of eval must also be the "standard > built-in function defined in 15.1.2.1". In other words, both the syntactic > form and the appropriate eval function value is necessary in order to have a > direct eval occurrence. This is the key -- the "is the standard built-in function" must mean === object identity. In this case we must have the reserved slot to retain the original value of 'eval' in the global object, etc., as the patch implements. > By this logic, cross global context eval calls should always be indirect evals. > Of course, when TC39 actually tries to formalize the concept of multiple > global environments and looks at the browser legacy we may end up somewhere > else. Not that we can get the future right, today, but it seems to me the current spec (understood as requiring function object identity) is a strong precedent. /be

(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > That is an incomplete paraphrasing of the ES5 spec, In addition to appearing > > > in that syntactic form, the current value of eval must also be the "standard > > > built-in function defined in 15.1.2.1". In other words, both the syntactic > > > form and the appropriate eval function value is necessary in order to have a > > > direct eval occurrence. > > > > I think we are in complete agreement. I was approaching from the point of view > > of what happens when the eval function is called, not from the syntactic side > > of what happens when a direct-looking "eval" is called. > > > > I sent mail to es-discuss an hour and a half ago about this, which seems not to > > have gone through. I'll resend and hopefully it'll make it this time. > > Cool, then the issue isn't whether it is a direct or indirect call. It is > which global object is used by the (indirect) eval call. No, the issue is the meaning of "is the standard built-in function" in 15.1.2.1.1 "Direct Call to Eval". > I think there is a more general question here which is what TC39 need to > clarify. Do built-in functions capture their global environment at the time of > their creation or do they they operate in the dynamic context of an ambient > global environment. I hope it is the former. That is a good question to be sure of. It's the former, there are lots of ways to tell. But answering that more general question does not answer the possibly-wrong-headed question about the meaning of "is the standard built-in function" in 15.1.2.1.1. /be

I've sort of lost track of this bug, but AIUI the situation is: 1. We all actually agree that eval(x) should be direct eval ONLY if `eval` evaluates to the built-in eval function object for the enclosing global scope. 2. That is what Jeff's patch is meant to implement. If I'm wrong, please let me know. :)

OK. The situation now is: 1. Abstractly, I agree with what this patch implements, which is that (a) it's not direct eval if `eval` evaluates to some other global's eval function, and not the one in lexical scope at the eval call site; (b) that kind of unqualified indirect eval should get that eval-function-object's global as `this` and as the scope. 2. I don't know what TC39 thinks or how the other peers are disposed toward this patch, so I'll have to at least skim the discussion before stamping this. In the meantime, here are the nits: In jsinterp.cpp, DirectEval: >- JS_ASSERT(vp[0].toObject().getFunctionPrivate() == evalfun); >- JS_ASSERT(IsAnyBuiltinEvalFunction(evalfun)); >+ JS_ASSERT(IsAnyBuiltinEvalFunction(vp[0].toObject().getFunctionPrivate())); >+ JS_ASSERT(vp[0] == vp[0].toObject().getGlobal()->getReservedSlot(JSRESERVED_GLOBAL_EVAL)); > > JSStackFrame *caller = cx->fp(); > JS_ASSERT(caller->isScriptFrame()); >- AutoFunctionCallProbe callProbe(cx, evalfun, caller->script()); >+ JS_ASSERT(IsBuiltinEvalFunctionForScope(&caller->scopeChain(), vp[0])); >+ AutoFunctionCallProbe callProbe(cx, vp[0].toObject().getFunctionPrivate(), caller->script()); After the patch, the first two assertions are pretty much the same as the last one. Drop the first two? In jsobj.cpp: > bool > IsAnyBuiltinEvalFunction(JSFunction *fun) Delete this function? AFAICT it's now only used in the assertion above that I wanted you to delete. But either way is fine with me. In js_InitObjectClass: >+ JS_ASSERT((obj->getClass()->flags & JSCLASS_GLOBAL_FLAGS) == JSCLASS_GLOBAL_FLAGS); >+ JS_ALWAYS_TRUE(js_SetReservedSlot(cx, obj, JSRESERVED_GLOBAL_EVAL, ObjectValue(*evalobj))); The first line is a little off, because JSCLASS_GLOBAL_FLAGS includes some of the bits in JSCLASS_RESERVED_SLOTS_MASK but not all of them. I would just assert JSCLASS_IS_GLOBAL here. Then maybe make js_SetReservedSlot assert that the slot is valid for the object's class, so you don't have to assert that separately here. (But if that is too much of a pain to do in this bug, OK.) Separately, why is js_SetReservedSlot guaranteed to succeed here? It seems like it might have to allocate slots, which can fail. Regarding comment 13, I'm indifferent. The first lines of the function are a comment that says "NB: This method handles only indirect eval: direct eval is handled by JSOP_EVAL." That works for me.

(In reply to comment #29) > 2. I don't know what TC39 thinks or how the other peers are disposed toward > this patch, so I'll have to at least skim the discussion before stamping > this. I think everyone who spoke up agreed that this is the right behavior. The browsers that throw EvalError are out of whack since ES5 removed throws of that as an option (but kept the exception for compatibility). The break with Firefox and other past SpiderMonkey releases should be minor incompatibility, or an outright fix. We'll find out. > In jsobj.cpp: > > bool > > IsAnyBuiltinEvalFunction(JSFunction *fun) > > Delete this function? AFAICT it's now only used in the assertion above that I > wanted you to delete. > > But either way is fine with me. We don't keep deadwood -- review comments call it out all the time and patch authors remove it. This nicely avoids the Any micro-debate. > In js_InitObjectClass: > >+ JS_ASSERT((obj->getClass()->flags & JSCLASS_GLOBAL_FLAGS) == JSCLASS_GLOBAL_FLAGS); > >+ JS_ALWAYS_TRUE(js_SetReservedSlot(cx, obj, JSRESERVED_GLOBAL_EVAL, ObjectValue(*evalobj))); > > The first line is a little off, because JSCLASS_GLOBAL_FLAGS includes some of > the bits in JSCLASS_RESERVED_SLOTS_MASK but not all of them. I would just > assert JSCLASS_IS_GLOBAL here. Which is spelled obj->isGlobal(), better than open-coding here. > Separately, why is js_SetReservedSlot guaranteed to succeed here? It seems like > it might have to allocate slots, which can fail. You're right, JS_NewGlobalObject does not ensure all reserved slots. Maybe it should. Andreas? > Regarding comment 13, I'm indifferent. The first lines of the function are a > comment that says "NB: This method handles only indirect eval: direct eval is > handled by JSOP_EVAL." That works for me. If we need Any to hammer home a point about a predicate, why is this function's name so short as 'eval'? Is it just a homage to the spec? The spec is a kind of implementation, with its own trade-offs (and bugs -- ask Allen). Anyway, it's not important enough to spend more words on, but I still see something out of whack when a long name needs a gratuitous "Any" while "eval" for indirect eval passes without qualification. I will stifle now. Waldo, you should have the last word. /be

Comment on attachment 516503 [details] [diff] [review] Oops, test file got dropped from the patch r=me with the nits addressed. Btw, by "either way" I didn't mean to say that it's OK to keep dead code. I meant that either keeping both the assertion and the function, or deleting both, would be OK.

The function's not dead code as I added JS_IsBuiltinEvalFunction for xpconnect's use (this is its way of ensuring chrome eval never leaks, seems plausible other embeddings might want the predicate for similar reasons). Re naming, it occurs to me that either "builtin" or "function" is superfluous in the names. An engine-internal predicate wouldn't care that a user-defined function is named eval, or something goofy like that. If we say "eval", surely we mean the/an eval function and no other concept. So I removed "Function" from the method names, which reduces verbosity without losing clarity. And since this puts us even better than status quo, I added "Any" to the is-any-eval-function method since we have the budget for it, and it makes the two methods clearly conceptually distinguished by their names. Thus we have js::IsAnyBuiltinEval to detect whether a function is an eval as defined by the spec, and we have js::IsBuiltinEvalForScope to detect whether a function is the eval for the provided scope object's global. I had forgotten globals don't get their slots pre-reserved. I agree we should do this and will file a followup to do that. Landed with those changes: http://hg.mozilla.org/tracemonkey/rev/9439b1936151 Re eval naming, maybe I'm just too familiar with eval as the spec defines it, but it does seem to me to be two things: eval-the-operator and eval-the-method. The former is defined where all ops are defined, trailing from there shortly to EvalKernel. The latter is defined as all methods are defined. It's easy to get from either one to the other, as jorendorff notes, so I've left it as is.