Closed
Bug 495325
Opened 15 years ago
Closed 15 years ago
ES5 indirect eval == global eval NOW!
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: brendan, Assigned: mrbkap)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(2 files, 5 obsolete files)
691 bytes,
text/plain
|
Details | |
10.86 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
'nuff said, except: NOW! means mozilla-central and tracemonkey repos, but not 1.9.1 branch. We needed this last year. /be
Reporter | ||
Updated•15 years ago
|
Summary: ES5 indirect eval NOW! → ES5 indirect eval == global eval NOW!
Reporter | ||
Comment 1•15 years ago
|
||
This needs an owner and a patch, pronto. /be
Assignee | ||
Comment 3•15 years ago
|
||
So, in writing this patch, I noticed that we broke the no-scripted-caller case again. This patch just throws in this case. Given that it's been broken for a while and nobody has been clamoring, I think this should be OK. Otherwise, I think this patch does what's needed here.
Attachment #394973 -
Flags: review?(brendan)
Assignee | ||
Comment 4•15 years ago
|
||
Reporter | ||
Comment 5•15 years ago
|
||
Comment on attachment 394973 [details] [diff] [review] Proposed fix No-scripted-caller is setTimeout(eval, 0, str) or the like, right? How did we break it? I don't see how caller is dereferenced unless non-null (indirectCall will be false if !caller). > #if JS_HAS_EVAL_THIS_SCOPE >- /* If obj.eval(str), emulate 'with (obj) eval(str)' in the caller. */ >+ /* >+ * If we see an indirect call, then run eval in the global scope. We do >+ * this so the compiler can make assumptions about what bindings may or >+ * may not exist in the current frame if it doesn't see 'eval'. >+ */ > if (indirectCall) { > callerScopeChain = js_GetScopeChain(cx, caller); Need GetGlobalForObject here, no? Also, which global? The eval function object's global, or the caller's global? I hope ES5 says (it may not if it doesn't include enough multiple-globals magic). /be
Assignee | ||
Comment 6•15 years ago
|
||
(In reply to comment #5) > (From update of attachment 394973 [details] [diff] [review]) > No-scripted-caller is setTimeout(eval, 0, str) or the like, right? How did we > break it? I don't see how caller is dereferenced unless non-null (indirectCall > will be false if !caller). We're both guilty here: caller->fun is dereferenced when we check if this script is in the cache, and I call it if (scopeobj). Is it an important enough usecase to keep around? > > if (indirectCall) { > > callerScopeChain = js_GetScopeChain(cx, caller); > > Need GetGlobalForObject here, no? I add the GetGlobalForObject below. If I added it here, then we'd restore the wrong object when fixing up the caller's scope chain. > Also, which global? The eval function object's global, or the caller's global? > I hope ES5 says (it may not if it doesn't include enough multiple-globals > magic). I think that for compatibilities with older SpiderMonkies, we need to use the global of the 'this' object (to support otherWindow.eval()). That will translate into the caller's global if this was a evil() indirect call as opposed to a window.evil() indirect call.
Reporter | ||
Comment 7•15 years ago
|
||
Talked to mrbkap, he is gonna update the patch not to use an implicit with. /be
Reporter | ||
Comment 8•15 years ago
|
||
Ping? /be
Reporter | ||
Comment 9•15 years ago
|
||
Comment on attachment 394973 [details] [diff] [review] Proposed fix Review ping2. /be
Assignee | ||
Comment 10•15 years ago
|
||
Sorry -- the last couple of days have been busy. I'll get to this tomorrow.
Assignee | ||
Comment 11•15 years ago
|
||
Attachment #394973 -
Attachment is obsolete: true
Attachment #396564 -
Flags: review?(brendan)
Attachment #394973 -
Flags: review?(brendan)
Assignee | ||
Comment 12•15 years ago
|
||
Comment on attachment 396564 [details] [diff] [review] patch v2 Er, this isn't right.
Attachment #396564 -
Attachment is obsolete: true
Attachment #396564 -
Flags: review?(brendan)
Assignee | ||
Comment 13•15 years ago
|
||
Attachment #394974 -
Attachment is obsolete: true
Assignee | ||
Comment 14•15 years ago
|
||
I talked this over with brendan. If you want to augment the scope chain, you now have to type 'with (obj) eval(...)'. foo.eval() means that foo must be a global object. eval(..., foo); means that foo can be anything and we ignore the scope chain entirely.
Attachment #396625 -
Flags: review?(igor)
Attachment #396625 -
Flags: review?(brendan)
Reporter | ||
Comment 15•15 years ago
|
||
Comment on attachment 396625 [details] [diff] [review] patch v3 I'm going to let Igor go first ;-). /be
Comment 16•15 years ago
|
||
Comment on attachment 396625 [details] [diff] [review] patch v3 >diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp >--- a/js/src/jsobj.cpp >+++ b/js/src/jsobj.cpp >@@ -1290,131 +1296,124 @@ obj_eval(JSContext *cx, JSObject *obj, u >+ > OBJ_TO_INNER_OBJECT(cx, obj); ... >+ >+ ok = js_CheckPrincipalsAccess(cx, obj, >+ JS_StackFramePrincipals(cx, caller), >+ cx->runtime->atomState.evalAtom); ... >+ >+ /* NB: We know obj is a global object here. */ >+ JS_ASSERT(!OBJ_GET_PARENT(cx, obj)); Nit: Move the assert right after OBJ_TO_INNER_OBJECT to ensure that any OBJ_TO_INNER_OBJECT returns a global object.
Attachment #396625 -
Flags: review?(igor) → review+
Assignee | ||
Comment 17•15 years ago
|
||
Brendan: review ping.
Reporter | ||
Comment 18•15 years ago
|
||
Blake, could you refresh? I conflicted you a bit :-(. /be
Assignee | ||
Comment 19•15 years ago
|
||
Updated to tip.
Attachment #396625 -
Attachment is obsolete: true
Attachment #401336 -
Flags: review?(brendan)
Attachment #396625 -
Flags: review?(brendan)
Reporter | ||
Comment 20•15 years ago
|
||
Comment on attachment 401336 [details] [diff] [review] patch v3.1 (gdb) r Starting program: /Users/brendaneich/Hacking/hg.mozilla.org/tracemonkey/js/src/Darwin_DBG.OBJ/js Reading symbols for shared libraries +++++....................................................................... done js> function f(s){return eval(s)} js> f("#2#") Assertion failure: script->nfixed == 0, at ../jsinterp.cpp:1021 Program received signal SIGTRAP, Trace/breakpoint trap. JS_Assert (s=0x1afec1 "script->nfixed == 0", file=0x1ae3c4 "../jsinterp.cpp", ln=1021) at ../jsutil.cpp:69 69 abort(); (gdb) r The program being debugged has been started already. Start it from the beginning? (y or n) y Starting program: /Users/brendaneich/Hacking/hg.mozilla.org/tracemonkey/js/src/Darwin_DBG.OBJ/js Reading symbols for shared libraries . done js> eval("#1=[#1#]") Assertion failure: script->nfixed == 0, at ../jsinterp.cpp:1021 Program received signal SIGTRAP, Trace/breakpoint trap. JS_Assert (s=0x1afec1 "script->nfixed == 0", file=0x1ae3c4 "../jsinterp.cpp", ln=1021) at ../jsutil.cpp:69 69 abort(); The assertions that nfixed == 0 are bogus. Not sure you can assert anything about nfixed, or need to. /be
Attachment #401336 -
Flags: review?(brendan) → review-
Assignee | ||
Comment 21•15 years ago
|
||
diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp --- a/js/src/jsinterp.cpp +++ b/js/src/jsinterp.cpp @@ -1497,22 +1497,17 @@ js_Execute(JSContext *cx, JSObject *chai hookData = mark = NULL; oldfp = js_GetTopStackFrame(cx); frame.script = script; if (down) { /* Propagate arg state for eval and the debugger API. */ frame.callobj = down->callobj; frame.argsobj = down->argsobj; frame.varobj = down->varobj; - if (script->staticLevel > 0) { - JS_ASSERT(script->nfixed == 0); - frame.fun = down->fun; - } else { - frame.fun = NULL; - } + frame.fun = (script->staticLevel > 0) ? down->fun : NULL; frame.thisv = down->thisv; if (down->flags & JSFRAME_COMPUTED_THIS) flags |= JSFRAME_COMPUTED_THIS; frame.argc = down->argc; frame.argv = down->argv; frame.annotation = down->annotation; } else { frame.callobj = NULL;
Attachment #401336 -
Attachment is obsolete: true
Attachment #401349 -
Flags: review?(brendan)
Reporter | ||
Comment 22•15 years ago
|
||
Comment on attachment 401349 [details] [diff] [review] patch v4 Whew! Let's go... /be
Attachment #401349 -
Flags: review?(brendan) → review+
Assignee | ||
Updated•15 years ago
|
Whiteboard: fi,
Assignee | ||
Comment 23•15 years ago
|
||
Oops... http://hg.mozilla.org/tracemonkey/rev/de72243414cd
Whiteboard: fi, → fixed-in-tracemonkey
Comment 24•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/de72243414cd
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Flags: wanted1.9.2-
Updated•14 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•