eval is too dynamic (js1_5/Regress/regress-68498-003.js)

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
10 years ago
a year ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

({regression, testcase, verified1.8.1.13})

unspecified
regression, testcase, verified1.8.1.13
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.13 +
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
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

10 years ago
test case section 1 of js1_5/Regress/regress-68498-003.js
Flags: in-testsuite+
Summary: eval is too dynamic → eval is too dynamic (js1_5/Regress/regress-68498-003.js)
(Assignee)

Updated

10 years ago
Duplicate of this bug: 385898
(Assignee)

Updated

10 years ago
Assignee: general → mrbkap
Flags: blocking1.9?
(Assignee)

Comment 4

10 years ago
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.
Attachment #284059 - Flags: review?(brendan)
(Assignee)

Comment 5

10 years ago
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?
Attachment #284059 - Flags: review?(igor)

Updated

10 years ago
Blocks: 397188

Comment 6

10 years ago
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?
(Assignee)

Comment 7

10 years ago
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.

Updated

10 years ago
Attachment #284059 - Flags: review?(igor) → review+
(Assignee)

Comment 8

10 years ago
Comment on attachment 284059 [details] [diff] [review]
Fix

This patch restores needed compatibility that was lost with overzealous code removal from a previous fix.
Attachment #284059 - Flags: approval1.9?
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
Attachment #284059 - Flags: review?(brendan)
Attachment #284059 - Flags: review+
Attachment #284059 - Flags: approval1.9?
Attachment #284059 - Flags: approval1.9+
(Assignee)

Comment 10

10 years ago
Fix checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 399587

Comment 11

10 years ago
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
(Assignee)

Comment 12

10 years ago
(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.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 391923

Comment 14

10 years ago
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.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

10 years ago
Flags: blocking1.8.1.12?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.12?
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Fixed on the branch in bug 382509.
Keywords: fixed1.8.1.13

Comment 16

10 years ago
v 1.8.1 js1_5/Regress/regress-68498-003.js, js1_5/Regress/regress-383682.js linux|mac|win32
Keywords: fixed1.8.1.13 → verified1.8.1.13
(Assignee)

Updated

a year ago
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.