Created attachment 267672 [details] testcase In bug 382509, we removed indirect eval entirely. In doing so, I ripped out the code that made |foo.eval(...)| equivalent to |with (foo) eval(...)|. At the time, I intended to do this, however, this broke (at the very least) bclary's spider, which relied on |this.eval(...)| to add all function statements to 'this', and not the activation record of the current function. As a temporary workaround, I think that Bob could simply do with (this) eval(...) but I wanted to raise this as a potential compatibility problem and at least have a bug on file.
Of course, that workaround doesn't work. I didn't notice before that obj.eval was not truly syntactic sugar for with (obj) eval, it also set the caller's scopechain and variables object.
test case section 1 of js1_5/Regress/regress-68498-003.js
*** Bug 385898 has been marked as a duplicate of this bug. ***
Created attachment 284059 [details] [diff] [review] Fix This patch brings back the indirect call variables object jazz. I suspect this will fix a bunch of random bustage that I've seen on a bunch of sites.
Comment on attachment 284059 [details] [diff] [review] Fix I think brendan mentioned he was going to be busy this week, Igor would you review this?
Comment on attachment 284059 [details] [diff] [review] Fix > /* Pick JSOP_EVAL and flag tc as heavyweight if eval(...). */ > pn2->pn_op = JSOP_CALL; >- if (pn->pn_arity == PN_NAME && >+ if (pn->pn_op == JSOP_NAME && > pn->pn_atom == cx->runtime->atomState.evalAtom) { > pn2->pn_op = JSOP_EVAL; > tc->flags |= TCF_FUN_HEAVYWEIGHT; > } Question: why this change is necessary here?
Originally, when we were going to outlaw all "indirect" eval statements, we needed to be more particular about when we emitted JSOP_CALL vs. JSOP_EVAL. Without this patch applied, we generate JSOP_EVAL for |foo.eval()| as well as |eval()|. With the patch, the former becomes a JSOP_CALLPROP, so obj_eval will know to muddle with the scope chain. Note that this is a partial backout of the patch in bug 382509.
Comment on attachment 284059 [details] [diff] [review] Fix This patch restores needed compatibility that was lost with overzealous code removal from a previous fix.
Comment on attachment 284059 [details] [diff] [review] Fix Thanks, too bad cross-browser editors and such have forked their JS to make use of this old SpiderMonkey eval extension. /be
Fix checked into trunk.
Note js1_5/Regress/regress-68498-003.js still fails in the browser on trunk (I assume this is expected) with: Testing calling obj.eval(str); currently at expect[3] within test - expected: false actual: true reason: Expected value 'false', Actual value 'true' but attachment 267672 [details] does pass. Checking in regress-383682.js; /cvsroot/mozilla/js/tests/js1_5/Regress/regress-383682.js,v <-- regress-383682.js initial revision: 1.1
(In reply to comment #11) > Note js1_5/Regress/regress-68498-003.js still fails in the browser on trunk (I > assume this is expected) with: This is a XOW problem, but I think there's already a bug filed on it.
*** Bug 391923 has been marked as a duplicate of this bug. ***
js1_5/Regress/regress-383682.js verified fixed 2007-12-09 1.9.0 linux|mac|win js1_5/Regress/regress-68498-003.js still fails however.
Fixed on the branch in bug 382509.
v 1.8.1 js1_5/Regress/regress-68498-003.js, js1_5/Regress/regress-383682.js linux|mac|win32
or
