496790 - Cannot access optimized closure in mootools

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Dave Townsend [:mossop]]

Visit the url given. Click on the 2009 drop down. On Firefox 3.0.x a list of years appears. On current tracemonkey, trunk and branch the page just reloads. As the page is loading I see the following in the error console:

Error: cannot access optimized closure
Source File: http://www.formula1.com/js/mootools.js
Line: 3

Bisect says the regression was caused by bug 494235 (http://hg.mozilla.org/releases/mozilla-1.9.1/rev/abd967e2173b)

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Comment 2

[Igor Bukanov]

Comment on attachment 382045 [details] [diff] [review]
fix

>-    if ((cg->flags & TCF_COMPILE_N_GO) && cg->compiler->callerFrame)
>-        script->flags |= JSSF_ESCAPE_HAZARD;
>+    if (cg->flags & TCF_COMPILE_N_GO) {
>+        JSStackFrame *caller = cg->compiler->callerFrame;
>+
>+        /* See jsobj.cpp:obj_eval, the indirectCall flag computation. */
>+        if (caller && caller->regs && *caller->regs->pc != JSOP_EVAL)

What about eval inside escape hazard code, like eval inside indirect eval?

Comment 3

[Ria Klaassen (not reading all bugmail)]

Bug 496750 seems the same.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #2)
> (From update of attachment 382045 [details] [diff] [review])
> >-    if ((cg->flags & TCF_COMPILE_N_GO) && cg->compiler->callerFrame)
> >-        script->flags |= JSSF_ESCAPE_HAZARD;
> >+    if (cg->flags & TCF_COMPILE_N_GO) {
> >+        JSStackFrame *caller = cg->compiler->callerFrame;
> >+
> >+        /* See jsobj.cpp:obj_eval, the indirectCall flag computation. */
> >+        if (caller && caller->regs && *caller->regs->pc != JSOP_EVAL)
> 
> What about eval inside escape hazard code, like eval inside indirect eval?

Great comment, I had intended to make JSSF_ESCAPE_HAZARD contagious, and it is via wrappers, but not via eval chaining. New patch soon, reduced testcase first.

/be

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 382045 [details] [diff] [review] [details])
> > >-    if ((cg->flags & TCF_COMPILE_N_GO) && cg->compiler->callerFrame)
> > >-        script->flags |= JSSF_ESCAPE_HAZARD;
> > >+    if (cg->flags & TCF_COMPILE_N_GO) {
> > >+        JSStackFrame *caller = cg->compiler->callerFrame;
> > >+
> > >+        /* See jsobj.cpp:obj_eval, the indirectCall flag computation. */
> > >+        if (caller && caller->regs && *caller->regs->pc != JSOP_EVAL)
> > 
> > What about eval inside escape hazard code, like eval inside indirect eval?
> 
> Great comment, I had intended to make JSSF_ESCAPE_HAZARD contagious, and it is
> via wrappers, but not via eval chaining.

But eval chaining can't help, because you can't pass unwrapped escaping closure refs to an inner eval -- you can pass only strings. So the inner eval still has to dig the would-be escapee out of a Call object, and that path is monitored for auto-wrapping of escape attempts.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: reduced js shell testcase, throws if bug, doesn't throw otherwise

Comment 8

[Brendan Eich [:brendan]]

Attachment: variation on last test, should *not* throw due to indirect eval

Comment 9

[Brendan Eich [:brendan]]

Attachment: variation on last test, indirect eval of source that evals

This test does not throw, because the direct eval embedded in the indirect eval's source string cannot leak optimized closures, per the assumption that the compile-time analysis is correct. So the indirect eval wrapping here is immaterial. Still trying to break this per Igor's idea.

/be

Comment 10

[Igor Bukanov]

(In reply to comment #7)
> Created an attachment (id=382056) [details]
> reduced js shell testcase, throws if bug, doesn't throw otherwise

Doesn't this test indicates that the check FUN_NEEDS_WRAPPER(fun) in CheckForEscapingClosure is not right and marks a flat, perfectly-ok-to-escape closure ?

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (In reply to comment #7)
> > Created an attachment (id=382056) [details] [details]
> > reduced js shell testcase, throws if bug, doesn't throw otherwise
> 
> Doesn't this test indicates that the check FUN_NEEDS_WRAPPER(fun) in
> CheckForEscapingClosure is not right and marks a flat, perfectly-ok-to-escape
> closure ?

Yes, it does -- skipmin is 1 even though the closure has no nested procedures that reach up across it. And as you have noted, if they did reach across, then the analysis would have made the flat closure an escaping heavyweight. So there is no need to check for escaping flat closures! Thanks,

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> Created an attachment (id=382066) [details]
> fix, v2

I still do not see an example when an escaping closure can be safely leaked via a Call or Arguments objects. My assumption is that it cannot and for for this reason I suggest to drop the (cx->fp->script->flags & JSSF_ESCAPE_HAZARD) check in CheckForEscapingClosure and remove JSSF_ESCAPE_HAZARD.

But if there are cases when the assumption is wrong, then the patch deserves r+. I has not yet convinced myself that it closes all the holes, but I file a new bug if any.

Comment 14

[Brendan Eich [:brendan]]

Attachment: fix, v3 -- optimize js_GetCallVar back to its former status

Leaving r? on v2 patch, which is smaller. This patch has the advantage of not flirting with a speed hit on call object variable gets, which could be hot.

The reason for JSSF_ESCAPE_HAZARD is twofold:

(1) To avoid deoptimizing gets of null closures from Call objects (these can happen because a heavyweight function may contain null closures as well as heavyweight-making functions that use eval, with, etc. -- IOW, heavyweight propagates only upward in a nest of functions, not up and then down all paths in the tree).

and

(2) To avoid breaking compatibility due to wrappers being leaky abstractions, or non-transparent proxies if you will. If we always wrapped any null closure accessed via js_GetCallVar, we could wrap during non-debugger, no-indirect-eval execution, and break JS semantics and backward compatibility.

(2) is more important than (1) of course. Both seems worth pursuing at this late stage in the endgame.

From these it follows that JSSF_ESCAPE_HAZARD must be contagious as it is in this patch, as well as via wrapping.

/be

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> The reason for JSSF_ESCAPE_HAZARD is twofold:
> 
> (1) To avoid deoptimizing gets of null closures from Call objects

The code in question contains:

    if (cx->fp && cx->fp->script && (cx->fp->script->flags & JSSF_ESCAPE_HAZARD)) {
        jsval v = *vp;

        if (VALUE_IS_FUNCTION(cx, v)) {
            JSObject *funobj = JSVAL_TO_OBJECT(v);
            JSFunction *fun = GET_FUNCTION_PRIVATE(cx, funobj);
            if (FUN_NEEDS_WRAPPER(fun)) {
 
Removing the initial check for cx->fp && cx->fp->script && (cx->fp->script->flags & JSSF_ESCAPE_HAZARD would lead to something like:

    jsval v = *vp;

    if (VALUE_IS_FUNCTION(cx, v)) {
        JSObject *funobj = JSVAL_TO_OBJECT(v);
        JSFunction *fun = GET_FUNCTION_PRIVATE(cx, funobj);
        if (FUN_NEEDS_WRAPPER(fun)) {

That gives smaller code and roughly the same amount of ifs for the case of non-hazard frames and functions. 

 (these can
> happen because a heavyweight function may contain null closures as well as
> heavyweight-making functions that use eval, with, etc. -- IOW, heavyweight
> propagates only upward in a nest of functions, not up and then down all paths
> in the tree).

Could you give an example of that? I checked the disassembly of the following example and there no closures in site.

function f(a)
{
    function g() { eval(str); }

    function h() { return a; }

    return g() + h() + function() { return a; }();
}

> (2) To avoid breaking compatibility due to wrappers being leaky abstractions,
> or non-transparent proxies if you will. If we always wrapped any null closure
> accessed via js_GetCallVar, we could wrap during non-debugger, no-indirect-eval
> execution, and break JS semantics and backward compatibility.

That is what I would like to see - an example how a direct eval or a script using function.caller can safely access a closure that must not escape. Without this I just cannot give a sound review for the patch.

Comment 16

[Igor Bukanov]

(In reply to comment #15)
> That is what I would like to see - an example how a direct eval or a script
> using function.caller can safely access a closure that must not escape. Without
> this I just cannot give a sound review for the patch.

To be precise - I would like to see an example where the compiler generates JSOP_CALLNAME and not JSOP_CALL(ARG|LOCAL|UPSLOT) when calling an upvar-referencing closure.

Comment 17

[Igor Bukanov]

(In reply to comment #14)
> Created an attachment (id=382089) [details]
> fix, v3 -- optimize js_GetCallVar back to its former status

I observed that an upvar-referencing closures currently can only be either a function expression that is immediately called or a top-level function declarations. For example, in a case like the following the function g is not a null closure and uses JSOP_NAME to access the argument a.

function f(a)
{
    ++a;
    var g = function g() {
        return a;
    }
    return g();
}

dis("-r", f);

If this observation is indeed the general case, then there is a simple way to deoptimize the Call getter only for upvar-referencing closures. The observation is that top-level function declarations are executed before the rest of the code in the function. As such call_resolve can only be called for the declared name after the corresponding var is initialized. Thus it is already possible to check for escaping closure at the resolve time so call_resolve can set the getter to the checking or non-checking one bases on slot's value. This way rt->getCallVar can be avoided.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #15)
> (In reply to comment #14)
> > The reason for JSSF_ESCAPE_HAZARD is twofold:
> > 
> > (1) To avoid deoptimizing gets of null closures from Call objects
> 
> The code in question contains:
> 
>     if (cx->fp && cx->fp->script && (cx->fp->script->flags &
> JSSF_ESCAPE_HAZARD)) {
>         jsval v = *vp;
> 
>         if (VALUE_IS_FUNCTION(cx, v)) {
>             JSObject *funobj = JSVAL_TO_OBJECT(v);
>             JSFunction *fun = GET_FUNCTION_PRIVATE(cx, funobj);
>             if (FUN_NEEDS_WRAPPER(fun)) {

Counting loads before the fast path branch (JSSF_ESCAPE_HAZARD not set): 3.

> Removing the initial check for cx->fp && cx->fp->script &&
> (cx->fp->script->flags & JSSF_ESCAPE_HAZARD would lead to something like:
> 
>     jsval v = *vp;
> 
>     if (VALUE_IS_FUNCTION(cx, v)) {

This has two loads, one for &js_FunctionClass, but neither depends on the other -- they both feed an equality test. But this first has to test !JSVAL_IS_PRIMITIVE(v) and while that does not load, it costs a branch (minor cycle penalty even if mispredicted since the code is in the pipe).

>         JSObject *funobj = JSVAL_TO_OBJECT(v);
>         JSFunction *fun = GET_FUNCTION_PRIVATE(cx, funobj);
>         if (FUN_NEEDS_WRAPPER(fun)) {

This is where more loads come in. With function values being accessed most often there would be a strictly more loads and ALU ops and branch tests than with JSSF_ESCAPE_HAZARD.

> Could you give an example of that?

Non-minimal example, to show variations that do and do not result in safe null closure gets from a Call object:

function m(a, b, c) {
    function f(a, b) {
        function g() { return eval(str); }

        function h() { return a + i() + j(c); }
        function i() { return b; }
        function j(c) {
            function k() { return c; }
            return k();
        }

        return g() + h();
    }
    return f();
}

str = "4";
m(1, 2, 3);

Your fabulous dis('-r', m) discloses:

flags: NULL_CLOSURE
main:
00000:  deflocalfun 0 function k() {return c;}
00005:  nop
00006:  calllocal 0
00009:  call 0
00012:  return
00013:  stop

Source notes:
  0:     0 [   0] newline 
  1:     5 [   5] funcdef  function 0 (function k() {return c;})
  3:     6 [   1] newline 
  4:     9 [   3] pcbase   offset 3

flags: NULL_CLOSURE
main:
00000:  getupvar 0
00003:  return
00004:  stop

Source notes:

These are functions j and k, respectively.

A breakpoint in js_GetCallVar is hit twice:

Breakpoint 1, js_GetCallVar (cx=0x30bca0, obj=0x2c1100, id=5, vp=0xbffff0e8) at ../jsfun.cpp:1131
1131	    return CallPropertyOp(cx, obj, id, vp, JSCPK_VAR, JS_FALSE);
(gdb) c
Continuing.

Breakpoint 1, js_GetCallVar (cx=0x30bca0, obj=0x2c1100, id=7, vp=0xbffff0e8) at ../jsfun.cpp:1131
1131	    return CallPropertyOp(cx, obj, id, vp, JSCPK_VAR, JS_FALSE);
(gdb) s
CallPropertyOp (cx=0x30bca0, obj=0x2c1100, id=7, vp=0xbffff0e8, kind=JSCPK_VAR, setter=0) at ../jsfun.cpp:1047
1047	    if (STOBJ_GET_CLASS(obj) != &js_CallClass)
(gdb) n
1050	    fun = GetCallObjectFunction(obj);
(gdb) 
1051	    fp = (JSStackFrame *) JS_GetPrivate(cx, obj);
(gdb) 
1053	    if (kind == JSCPK_ARGUMENTS) {
(gdb) 
1073	    JS_ASSERT((int16) JSVAL_TO_INT(id) == JSVAL_TO_INT(id));
(gdb) 
1074	    i = (uint16) JSVAL_TO_INT(id);
(gdb) 
1075	    JS_ASSERT_IF(kind == JSCPK_ARG, i < fun->nargs);
(gdb) 
1076	    JS_ASSERT_IF(kind == JSCPK_VAR, i < fun->u.i.nvars);
(gdb) 
1078	    if (!fp) {
(gdb) 
1089	    if (kind == JSCPK_ARG) {
(gdb) 
1092	        JS_ASSERT(kind == JSCPK_VAR);
(gdb) 
1093	        array = fp->slots;
(gdb) 
1095	    if (setter) {
(gdb) 
1099	        *vp = array[i];
(gdb) 
1101	    return JS_TRUE;
(gdb) p/x *vp
$1 = 0x2c1180
(gdb) call js_DumpObject($)
object 0x2c1180
class 0x1d17e0 Function
no own properties - see proto (Function at 0x2c0000)
slots:
   0 (proto) = <unnamed function at 0x2c0000 (JSFunction at 0x2c0000)>
   1 (parent) = <Call object at 0x2c1100>
   2 (private) = 0x2c7888
   3 (reserved) = undefined
   4 (reserved) = undefined

(gdb) p/x *(JSFunction *)0x2c7888
$2 = {
  object = {
    map = 0x30c8a0, 
    classword = 0x1d17e0, 
    fslots = {0x2c0000, 0x2c1000, 0x2c7889, 0x16, 0x16}, 
    dslots = 0x0
  }, 
  nargs = 0x1, 
  flags = 0xc000, 
  u = {
    n = {
      extra = 0x1, 
      spare = 0x0, 
      native = 0xda000000, 
      clasp = 0x30d970, 
      trcinfo = 0x30d820
    }, 
    i = {
      nvars = 0x1, 
      nupvars = 0x0, 
      skipmin = 0x0, 
      wrapper = 0x0, 
      script = 0x30d970, 
      names = {
        taggedAtom = 0x30d820, 
        array = 0x30d820, 
        map = 0x30d820
      }
    }
  }, 
  atom = 0x2c35fc
}
(gdb) call js_DumpId($.atom)
jsid 0x2c35fc = "j"

/be

Comment 19

[Brendan Eich [:brendan]]

In that example, function j has no upvars, but it is a null closure. If it had any upvars that reached up to the static level of f's body, or above, then it would have been deoptimized (since the eval in g can leak any containing function and all direct child declared functions nested within any containing function).

So the example does not show an unsafe leak, only a degenerate case. But it does show the the optimization benefit of JSSF_ESCAPE_HAZARD.

Indeed FindFunArgs is too pessimistic about HEAVYWEIGHT implying eval or a with that names an ancestor declared function, or uncle, great-aunt, etc. functions. But that is for another bug and release to optimize. If we did optimize so as to separate HEAVYWEIGHT from "could leak ancestors and their immediate kids" then again there could be Call objects accessed safely to get null closures, in this case including ones that do have upvars.

Comment 17 makes a good point about avoiding rt->getCallVar -- thanks.

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: fix, v4 -- avoid rt-wide deoptimization at a first-use cost in call_resolve

Leaving other r? requests outstanding, but v3 is going down. This v4 patch also avoids adding ugly macros, always a win.

/be

Comment 21

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/2d8dd0494e64

/be

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #21)
> http://hg.mozilla.org/tracemonkey/rev/2d8dd0494e64

Whoops, wrong bug.

/be

Comment 23

[Igor Bukanov]

Comment on attachment 382161 [details] [diff] [review]
fix, v4 -- avoid rt-wide deoptimization at a first-use cost in call_resolve

>--- a/js/src/jsscript.cpp
>+++ b/js/src/jsscript.cpp
>@@ -1530,8 +1530,22 @@ js_NewScriptFromCG(JSContext *cx, JSCode
>-    if ((cg->flags & TCF_COMPILE_N_GO) && cg->compiler->callerFrame)
>-        script->flags |= JSSF_ESCAPE_HAZARD;
>+    if (cg->flags & TCF_COMPILE_N_GO) {
>+        JSStackFrame *caller = cg->compiler->callerFrame;

With the patch JSSF_ESCAPE_HAZARD is only checked in js_GetCallVarChecked which is set only when a slot contains a closure that must not escape. That check is truly useless unless one want to save few cycles for indirect eval that overwrites such escaping closure with some other value.

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #23)
> (From update of attachment 382161 [details] [diff] [review])
> >--- a/js/src/jsscript.cpp
> >+++ b/js/src/jsscript.cpp
> >@@ -1530,8 +1530,22 @@ js_NewScriptFromCG(JSContext *cx, JSCode
> >-    if ((cg->flags & TCF_COMPILE_N_GO) && cg->compiler->callerFrame)
> >-        script->flags |= JSSF_ESCAPE_HAZARD;
> >+    if (cg->flags & TCF_COMPILE_N_GO) {
> >+        JSStackFrame *caller = cg->compiler->callerFrame;
> 
> With the patch JSSF_ESCAPE_HAZARD is only checked in js_GetCallVarChecked which
> is set only when a slot contains a closure that must not escape. That check is
> truly useless unless one want to save few cycles for indirect eval

Or debugger workalike.

> that
> overwrites such escaping closure with some other value.

True, it's not an issue now that the checked getter is split out. You may have mentioned replacing the getter last week -- my apologies for not following up on that. I will remove JSSF_ESCAPE_HAZARD, it is overkill.

/be

/be

Comment 25

[Igor Bukanov]

(In reply to comment #19)
> In that example, function j has no upvars, but it is a null closure. If it had
> any upvars that reached up to the static level of f's body, or above, then it
> would have been deoptimized (since the eval in g can leak any containing
> function and all direct child declared functions nested within any containing
> function).

That what I have been thinking - I just could not find a code path in the compiler that would not deoptimize an upvar-referencing closure. But thanks for the example, I have missed null closures without upvars. It was rather revealing to see its compilation in the debugger.

> 
> So the example does not show an unsafe leak, only a degenerate case. But it
> does show the the optimization benefit of JSSF_ESCAPE_HAZARD.

This is true. But this case is already slow by the fact that it goes via the Call object. On the other hand JSSF_ESCAPE_HAZARD adds complexity. This complexity requires careful considerations and extra mental efforts to analyze the code to make sure that the optimization is sound. And this bug clearly shows how much time was wasted due to the bug in the initial implementation of the optimization. Instead this time could be spent on improving the code in other areas.

Given this I claim that this optimization was not only premature but in fact in long term slows down the browser.  

> But that is for another bug and release to optimize.

Before farther optimizations I prefer to have a reasonable confirmation that the current hacks is enough to restore the compatibility of the upvar optimization with the web.

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #25)
> (In reply to comment #19)
> Given this I claim that this optimization was not only premature but in fact in
> long term slows down the browser.  

I agree; in my defense I was concerned that it is late in the release cycle to be regressing performance and possibly breaking compatibility by over-wrapping.

> > But that is for another bug and release to optimize.
> 
> Before farther optimizations I prefer to have a reasonable confirmation that
> the current hacks is enough to restore the compatibility of the upvar
> optimization with the web.

You must mean indirect eval, because that's the only possible issue on "the web" (more likely, in our add-ons and XUL apps -- possibly even our own app code!).

/be

Comment 27

[Brendan Eich [:brendan]]

Attachment: fix, v5 -- no more JSSF_ESCAPE_HAZARD

One more look would be appreciated.

/be

Comment 28

[Brendan Eich [:brendan]]

Igor: it seems that now call_resolve could do the wrapping of any needsWrapper() function-valued slot being resolved. I don't see why it should not. Then we don't need a de-optimized js_GetCallVarChecked. Right?

/be

Comment 29

[Igor Bukanov]

(In reply to comment #28)
> it seems that now call_resolve could do the wrapping of any
> needsWrapper() function-valued slot being resolved. I don't see why it should
> not. Then we don't need a de-optimized js_GetCallVarChecked.

The problem is where to store the wrapped value when the frame is still active and the Call object has no slots? If the slots to store the put args and vars would be preallocated as in the bug 495061, then the wrapper could be stored in that slot. But even that would require special getter to check that if the value is still the upvar-referencing closure, the the wrapper should be returned.

Another possibility is to have a non-shared special getter. That getter would get own slot in the Call object to store the wrapper. Then, when the frame is active, the getter would again check if the frame's slot is still the original closure and return the wrapper if so.

This would have rather non-trivial benefit of returning the same wrapper each time solving the identity problem as the code that does function_a === function_b would only see the single instance of wrapper. This, of cause, would also require that the function.caller would check the same storage for the wrapper.

 




This would have an added benefit of making sure that the wrapper is created only once

Comment 32

[Brendan Eich [:brendan]]

(In reply to comment #29)
> The problem is where to store the wrapped value when the frame is still active
> and the Call object has no slots?

The frame has a slot. Why wouldn't we store the wrapper over top of the wrapped function? The wrapper retains a ref to the wrapped object via its proto slot.

/be

Comment 33

[Igor Bukanov]

(In reply to comment #32)
> (In reply to comment #29)
> > The problem is where to store the wrapped value when the frame is still active
> > and the Call object has no slots?
> 
> The frame has a slot. Why wouldn't we store the wrapper over top of the wrapped
> function? 

Can the implementation of function.caller easily find that frame's slot that stores the closure? If yes, then indeed frame's slot is a good place and would avoid a special getter. If not, then storing the wrapper as a property of the closure seems like a good option.






The wrapper retains a ref to the wrapped object via its proto slot.
> 
> /be

Comment 34

[Igor Bukanov]

(In reply to comment #32)
> The frame has a slot. Why wouldn't we store the wrapper over top of the wrapped
> function? The wrapper retains a ref to the wrapped object via its proto slot.

It could break more things. Currently the wrapper does not behave exactly as the closure due to the use of JSOP_NAME that may refer to wrong things due to optimized away Call objects. So replacing the closure with the wrapper, say, after f.caller may affect the semantic in unexpected ways.

Comment 35

[Brendan Eich [:brendan]]

(In reply to comment #34)
> (In reply to comment #32)
> > The frame has a slot. Why wouldn't we store the wrapper over top of the wrapped
> > function? The wrapper retains a ref to the wrapped object via its proto slot.
> 
> It could break more things. Currently the wrapper does not behave exactly as
> the closure due to the use of JSOP_NAME that may refer to wrong things due to
> optimized away Call objects.

Yeah, my scope chain reification was scary so I dropped it.

But again, this is only for escapes that the compiler didn't see. That means only for .caller and (indirect|debugger)-eval escapes.

> So replacing the closure with the wrapper, say,
> after f.caller may affect the semantic in unexpected ways.

Not sure what you mean here -- we are wrapping .caller leaks. Bill Edney tested TIBET and it still backtraces (which is great news, much appreciated). I hope that is enough.

But if it's not, we would have to work on wrapper transparency more, not on having two slots, one for the unwrapped function and one for the wrapper. It would never be safe to expose the unwrapped function where the compiler did not think it could flow.

/be

Comment 36

[Igor Bukanov]

(In reply to comment #35)
> > So replacing the closure with the wrapper, say,
> > after f.caller may affect the semantic in unexpected ways.
> 
> Not sure what you mean here -- we are wrapping .caller leaks. Bill Edney tested
> TIBET and it still backtraces (which is great news, much appreciated). 

If a wrapper replaces the closure in the frame.slots, the replacement will be used not only for backtracing, but also in any place where the compiler calls the closure. It requires that in all those cases the wrapper properly emulates the closure. Since this is not the case currently, it increases chances that an innocent usage of f.caller just to report, say, a call stack, would break things.

Comment 37

[Brendan Eich [:brendan]]

(In reply to comment #36)
 > If a wrapper replaces the closure in the frame.slots, the replacement will be
> used not only for backtracing, but also in any place where the compiler calls
> the closure. It requires that in all those cases the wrapper properly emulates
> the closure. Since this is not the case currently, it increases chances that an
> innocent usage of f.caller just to report, say, a call stack, would break
> things.

But you convinced me that it *is* the case currently that the compiler never emits code to access a null closure with non-zero skipmin by emitting a NAME op getting a value from a Call object's property. This conclusion led to getting rid of JSSF_ESCAPE_HAZARD.

We're always going to wrap caller if it is a null closure with non-zero skipmin.

I don't see why we need two getters unless we suspect more bugs where access from a debugger- or indirect-eval-generated script or function should be distinguished from access via compiler-emitted NAME ops. If we believe that such bugs exist, then we should want JSSF_ESCAPE_HAZARD, which is set only when indirect-eval'ing or debugging.

I will land the r+'ed patch in a bit, we can followup in another bug.

/be

Comment 38

[Igor Bukanov]

(In reply to comment #37)
> (In reply to comment #36)
> But you convinced me that it *is* the case currently that the compiler never
> emits code to access a null closure with non-zero skipmin by emitting a NAME op
> getting a value from a Call object's property. This conclusion led to getting
> rid of JSSF_ESCAPE_HAZARD.

But the compiler still emits JSOP_CALLLOCAL etc.! If the closure will be replaced in the frame.slots, it will be the wrapper, not the closure, that will pushed on the stack using JSOP_CALLLOCAL for latter invocation.

Comment 39

[Brendan Eich [:brendan]]

No, any HEAVYWEIGHT across which a null closure might reach with upvars (or a flat closure might reach) causes that optimized closure to be deoptimized -- the null closure is marked escaping (fn->setFunArg() in FindFunArgs) and the flat closure optimization attempt is skipped, look for FUN_METER(badfunarg).

Again if you are concerned about a bug in the analysis, then the belt-and-braces way to go is JSSF_ESCAPE_HAZARD, so long as it is independent of the needsWrapper calculation.

/be

Comment 40

[Igor Bukanov]

Consider the following case:

function f(a)
{
    ++a;
    function g() {
        return a;
    }
    indirect_eval("leak = g");
    return g();
}

Here the call g() in the function f uses JSOP_CALLLOCAL as witnessed by dis(). If during indirect_eval the wrapper for g will be replaced in the frame.slots, then it will be the wrapper, not the closure, that is invoked during return g();

Comment 41

[Igor Bukanov]

(In reply to comment #40)
> Consider the following case:
> 
> function f(a)
> {
>     ++a;
>     function g() {
>         return a;
>     }
>     indirect_eval("leak = g");
>     return g();
> }

Similar example using f.caller:

function f(a)
{
    ++a;
    function g() {
         x();
         return a;
    }
    return g();
}

function x() { print(x.caller); }

Comment 42

[Brendan Eich [:brendan]]

Sorry, I'm not arguing for changing a frame's slot to reference a wrapper -- I should have said so. I'm going with the last r+'ed patch, absent a better idea.

/be

Comment 43

[Brendan Eich [:brendan]]

For .caller we have no choice. I hope we can wrap and get away with it. Seems like we are going in circles! Again, apologies for being unclear.

/be

Comment 44

[Igor Bukanov]

(In reply to comment #43)
> For .caller we have no choice. I hope we can wrap and get away with it.

It is interesting to know how widely f.caller is used to call the function as opposed to getting a stack trace or annotate the function with properties. Maybe it would be possible to get away with unconditional throw on any invocation of f().

Comment 45

[Brendan Eich [:brendan]]

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

/be

Comment 46

[Christopher Blizzard (:blizzard)]

Seeing this same error when trying to edit the slug for a post in WordPress 2.7.1.  I'll re-test once this lands on 1.9.1.

Comment 47

[Robert Sayre]

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

Comment 48

[Christopher Blizzard (:blizzard)]

TM build with this fix in it does not exhibit the problem in WordPress.

Comment 49

[John Resig]

It looks like this has already been resolved but we got a report of a similar problem with jQuery in FF 3.5b99:
http://groups.google.com/group/jquery-en/browse_thread/thread/ab6aeb3a41603021

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #49)
> It looks like this has already been resolved but we got a report of a similar
> problem with jQuery in FF 3.5b99:
> http://groups.google.com/group/jquery-en/browse_thread/thread/ab6aeb3a41603021

We need a check-in on 1.9.1. It should be fixed on trunk. I will verify when the todays nightly build will be available.

Comment 52

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on trunk with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090610 Minefield/3.6a1pre ID:20090610032606

When will this be checked into 1.9.1?

Comment 53

[aja+bugzilla]

FWIW, as per http://forums.mozillazine.org/viewtopic.php?p=6675365#p6675365

at www.movieline.com

Error: cannot access optimized closure
Source file: http://www.movieline.com/_/s/jcarousellite_1.0.1.pack.js
Line: 1

Google search says that Carousel Lite is a query plugin

Comment 54

[aja+bugzilla]

s/query/jquery/

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #53)
> FWIW, as per http://forums.mozillazine.org/viewtopic.php?p=6675365#p6675365

It's not checked into 1.9.1 yet. Robert when will the next TM merge happen on 1.9.1?

Comment 57

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d3b6d64ee18b

Comment 58

[mdew]

using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b5pre) Gecko/20090614 Shiretoko/3.5pre, I'm now occasionally getting

A script on this page may be busy, or it may have stopped responding. You can stop the script now, open the script in the debugger, or let the script continue.

Script: http://www.nzherald.co.nz/cssimagesjs/scripts/jquery/jquery.ui.all.packed.js:1

from sample site: http://www.nzherald.co.nz/entertainment/news/article.cfm?c_id=1501119&objectid=10578398

Comment 59

[Brendan Eich [:brendan]]

(In reply to comment #58)
> using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b5pre) Gecko/20090614
> Shiretoko/3.5pre, I'm now occasionally getting
> 
> A script on this page may be busy, or it may have stopped responding. You can
> stop the script now, open the script in the debugger, or let the script
> continue.
> 
> Script:
> http://www.nzherald.co.nz/cssimagesjs/scripts/jquery/jquery.ui.all.packed.js:1
> 
> from sample site:
> http://www.nzherald.co.nz/entertainment/news/article.cfm?c_id=1501119&objectid=10578398

Please file a new bug -- that has nothing to do with this bug, which is fixed.

/be

Comment 60

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on 1.9.1 with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090614 Shiretoko/3.5pre ID:20090614030748

Comment 65

[Henrik Skupin [:whimboo][⌚️UTC+2]]

For everyone who is interested we have another issue with "Cannot access optimized closure" with Mozmill. I have filed bug 505001.