531675 - Ignore the second argument of eval()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #495325 +++

Currently SpiderMonkey supports an eval form like eval(source, scope) with unclear semantic. Since other browsers do not support this and it is possible to emulate such call using indirect eval forms, we should try to remove this as it was suggested in the bug 528116 comment 15.

Comment 1

[Jim Blandy :jimb]

Does emulation really require *indirect* eval forms?  I thought one used 'with (o) { eval(expr) }'.  Indirect eval is forbidden in strict mode code.

Comment 2

[Mark S. Miller]

Were the existing Mozilla 2-arg eval really "eval(source, scope)", it would be an asset rather than a horror. It is a horror because it breaks encapsulation. If a function closure is provided as the second argument, than Mozilla's 2-arg eval will use its captured scope as the scope in question. Hopefully, once this is retired, there will never again be a way for non-privileged code (such as a debugger) to emulate it.

In ES5, "with (o) { eval(expr) }" is, as you say, approximately a 2-arg eval in which o's properties are added to the scope in which expr is evaluated. At top level it emulates the 2-arg eval you have in mind quite well.

Indirect eval does not help emulate either flavor of 2-arg eval except the special case of "eval(expr, window)".

Indirect eval is not forbidden or altered by strict mode. Direct eval does differ in behavior depending on the strictness of the caller. Indirect eval does not.

Comment 3

[Mark S. Miller]

The above was unclear. The parenthetical "debugger" is an example of privileged code, which may do magic things exempt from normal language rules.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #2)
> Were the existing Mozilla 2-arg eval really "eval(source, scope)", it would be
> an asset rather than a horror. It is a horror because it breaks encapsulation.
> If a function closure is provided as the second argument, than Mozilla's 2-arg
> eval will use its captured scope as the scope in question.

Hi Mark, no to worry -- we already fixed this in Firefox 3.5 -- no closure scope leakage, even though two-arg eval persisted in that release. See bug 446026 (a comedy of errors, to be sure).

We even fixed two-arg eval by breaking it in Firefox 3.0.2 (see below).

Two-argument eval is still a horror, but there's no encapsulation violation:

js> var b = 45;
js> // Getting "private" variables
js> var obj = (function() {
  var a = 21;
  return {
    // public function must reference 'a'
    fn: function() {a;}
  };
})();
js> var foo;
js> try {
  eval('bar = b; foo=a', obj.fn);
} catch (e) {
  print("caught: " + e);
}
caught: ReferenceError: a is not defined
js> print(foo + " | " + bar); // 21
undefined | 45
js> 
js> eval("", {print:1})
js> print(1);
1

Since we already:

(a) broke two-argument eval in Firefox 3.0.x (bug 442333; leaving bug 457068 unfixed in its wake), and then

(b) put back an incompatible but at least not encapsulation-violating two-argument eval in Firefox 3.5,

therefore it seems to me that the mozilla-central nightly builds (Firefox 3.7pre) are the right place to put this old extension out of its misery.

I could even make the case for breaking 3.6, but not without another beta to find out the hard way what add-on or app depends on some aspect of two-arg eval as shipped in 3.5.

Igor: comment 0 mentions indirect eval as a way to emulate eval(s, o), but we are conforming to ES5 in Firefox 3.6 by making indirect eval == global eval, per bug 495325. So there's no way to emulate the two-argument eval in all its inconsistent-across-releases semantics.

It would be better to throw an exception if extra arguments are passed, but the time to do that was last decade.

The moral is: don't extend standard functions with extra arguments. ECMA-262 warns against this, but the extension is so old it preceded ES3 at least. ES3 says in Clause 15's intro:

"Unless otherwise specified in the description of a particular function, if a function or constructor described in this section is given more arguments than the function is specified to allow, the behaviour of the function or constructor is undefined. In particular, an implementation is permitted (but not required) to throw a TypeError exception in this case.

NOTE Implementations that add additional capabilities to the set of built-in functions are encouraged to do so by adding new functions rather than adding new parameters to existing functions."

ES1 and ES2 lacked such language.

This doesn't excuse anything. We started down this road in conversations in Ecma TC39 ("ECMA" TC39 TG1 in those days ;-) around the time ES3 was under way. We were trying to incubate a "better eval". But a bad strain got out of the Netscape lab. Time to kill it and clean everything with bleach -- or fire.

/be

Comment 5

[Jim Blandy :jimb]

(In reply to comment #1)
> Does emulation really require *indirect* eval forms?  I thought one used 'with
> (o) { eval(expr) }'.  Indirect eval is forbidden in strict mode code.

I don't know what I was thinking when I wrote this. Indirect calls to eval use a global execution context, but are permitted in both lenient and strict mode code. If the code being evaluated is strict mode code, then does get its own declarative environment, rather than instantiating variables in the containing environment. But that's different.

Comment 6

[Igor Bukanov]

(In reply to comment #4)
> Igor: comment 0 mentions indirect eval as a way to emulate eval(s, o), but we
> are conforming to ES5 in Firefox 3.6 by making indirect eval == global eval,
> per bug 495325.

That bug has not landed on 1.9.2 branch and its status for 1.9.2 is not wanted. Still with the indirect eval and the trick of using with (obj) { eval(); } it should be possible to cover all useful cases.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

This is what I just sent to the try server.

I didn't look too closely at the code, just ripped it out.

Apparently we only have two tests that use two-argument eval, and they're both regression tests for the same fuzzbug. I see no reason to keep those.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: v2

Full disks took out Tinderbox right in the middle of my Try Server run, but what results I got look good to me.

I took the bug summary literally and just made eval ignore the 2nd argument entirely. Hope that's the right thing.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Is it worth warning for the near future to allow developers who use it to see that we've explicitly disabled it? This is right in the territory where things will mostly "work" until some indeterminate point in the future when a random property or other won't exist and will have moved to the global object.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 438872 [details] [diff] [review]
v2

I authorize mrbkap -- he is the brave hacker who took on our over-extended obj_eval many years ago, while still a student.

/be

Comment 11

[Brendan Eich [:brendan]]

A warning if it can be done well (one-shot, don't want warning storms) would probably be helpful to developers using the old two-arg eval extension.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 438872 [details] [diff] [review]
v2

I would like to see the one-shot warning, but this is fine. We have been beaten back on this before, though, so watch out!

Comment 13

[Jeff Walden [:Waldo]]

Stealing with permission...

Comment 14

[Jeff Walden [:Waldo]]

Attachment: v3 (sort of)

v2 is fairly bitrotted (if still easily fixably), and because I'd happened to write a patch for this before checking the bug and seeing one had *already* been done (mine hits an extra nit or two, but otherwise it's identical), this is all me.  The different bits (warning, no more unnecessary WithGuard, some comment tweaks) make it different enough I'm throwing it up for a review.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 460364 [details] [diff] [review]
v3 (sort of)

r=me with the !caller->script handling cleaned up (caller->script must be non-null).

Comment 16

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/ff6cf05b19f1

Made the disputable faux pas of not citing r=brendan in the push comment (he didn't review *that* patch, whose lineage is distinct from the one he reviewed), hope he doesn't mind too much... ;-)

Comment 17

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/ff6cf05b19f1