Last Comment Bug 383682 - eval is too dynamic (js1_5/Regress/regress-68498-003.js)
: eval is too dynamic (js1_5/Regress/regress-68498-003.js)
: regression, testcase, verified1.8.1.13
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: Jason Orendorff [:jorendorff]
: 385898 391923 (view as bug list)
Depends on: 399587
Blocks: 382509 397188
  Show dependency treegraph
Reported: 2007-06-07 19:13 PDT by Blake Kaplan (:mrbkap)
Modified: 2016-05-11 15:45 PDT (History)
13 users (show)
dveditz: blocking1.8.1.13+
dveditz: wanted1.8.1.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (87 bytes, text/plain)
2007-06-07 19:13 PDT, Blake Kaplan (:mrbkap)
no flags Details
Fix (6.29 KB, patch)
2007-10-08 16:42 PDT, Blake Kaplan (:mrbkap)
brendan: review+
igor: review+
brendan: approval1.9+
Details | Diff | Splinter Review

Description User image Blake Kaplan (:mrbkap) 2007-06-07 19:13:39 PDT
Created attachment 267672 [details]

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.
Comment 1 User image Blake Kaplan (:mrbkap) 2007-06-08 12:25:36 PDT
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.
Comment 2 User image Bob Clary [:bc:] 2007-06-13 11:34:15 PDT
test case section 1 of js1_5/Regress/regress-68498-003.js
Comment 3 User image Blake Kaplan (:mrbkap) 2007-07-22 18:44:40 PDT
*** Bug 385898 has been marked as a duplicate of this bug. ***
Comment 4 User image Blake Kaplan (:mrbkap) 2007-10-08 16:42:31 PDT
Created attachment 284059 [details] [diff] [review]

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 5 User image Blake Kaplan (:mrbkap) 2007-10-08 16:55:08 PDT
Comment on attachment 284059 [details] [diff] [review]

I think brendan mentioned he was going to be busy this week, Igor would you review this?
Comment 6 User image Igor Bukanov 2007-10-08 18:00:35 PDT
Comment on attachment 284059 [details] [diff] [review]

>             /* 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?
Comment 7 User image Blake Kaplan (:mrbkap) 2007-10-08 18:04:41 PDT
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 8 User image Blake Kaplan (:mrbkap) 2007-10-08 18:20:16 PDT
Comment on attachment 284059 [details] [diff] [review]

This patch restores needed compatibility that was lost with overzealous code removal from a previous fix.
Comment 9 User image Brendan Eich [:brendan] 2007-10-09 11:59:33 PDT
Comment on attachment 284059 [details] [diff] [review]

Thanks, too bad cross-browser editors and such have forked their JS to make use of this old SpiderMonkey eval extension.

Comment 10 User image Blake Kaplan (:mrbkap) 2007-10-09 15:43:24 PDT
Fix checked into trunk.
Comment 11 User image Bob Clary [:bc:] 2007-10-31 10:23:08 PDT
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
Comment 12 User image Blake Kaplan (:mrbkap) 2007-10-31 10:29:35 PDT
(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.
Comment 13 User image Blake Kaplan (:mrbkap) 2007-11-02 19:40:50 PDT
*** Bug 391923 has been marked as a duplicate of this bug. ***
Comment 14 User image Bob Clary [:bc:] 2007-12-10 08:11:40 PST
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.
Comment 15 User image Samuel Sidler (old account; do not CC) 2008-03-07 10:27:04 PST
Fixed on the branch in bug 382509.
Comment 16 User image Bob Clary [:bc:] 2008-03-17 05:05:08 PDT
v 1.8.1 js1_5/Regress/regress-68498-003.js, js1_5/Regress/regress-383682.js linux|mac|win32

Note You need to log in before you can comment on or make changes to this bug.