496605 - Need to prevent optimized closure escape via foo.caller still

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Attachment: js shell testcase

See attached testcase. Easy fix, the assertion did its job.

/be

Comment 1

[Igor Bukanov]

(In reply to comment #0)
> Created an attachment (id=381807) [details]
> js shell testcase

Yep - the helper can see unwrapped caller...

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix, with macros to uphold DRY

Comment 3

[Brendan Eich [:brendan]]

Attachment: with fixed comment

Comment 4

[Igor Bukanov]

This hole is unrelated to the indirect eval as .caller leaks the closure without any eval involvement as the following example that crashes js shell demonstrates:

var escaped;

function helper()
{
    if (!escaped)
        escaped = helper.caller;
}

function f() {
    var x = 0;
    ++x;
    return function() {
        helper();
        return x;
    }();
}

f();
escaped();

Comment 5

[Brendan Eich [:brendan]]

Yes, good point. This came to me almost in a dream last night, and I just adapted a test you wrote with minimal change ;-).

/be

Comment 6

[Igor Bukanov]

Comment on attachment 381811 [details] [diff] [review]
with fixed comment

>       case FUN_CALLER:
>+            /* See the equivalent check in args_getProperty for ARGS_CALLEE. */
>+            if (FUN_ESCAPE_HAZARD(fp->down->fun)) {
>+                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                     JSMSG_OPTIMIZED_CLOSURE_LEAK);
>+                return JS_FALSE;
>+            }

Due to apparently widespread use of fun.caller this throwing of an exception may potentially break some sites. If this happens, the remedy would be to add the wrapping code instead of throwing. But if we get away with this simple throw, then better.

Comment 7

[Brendan Eich [:brendan]]

Attachment: Igor's more minimal (in dependencies) testcase

Comment 8

[Brendan Eich [:brendan]]

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

/be

Comment 9

[Brendan Eich [:brendan]]

Talked to sayrer, we're inclined not to risk breaking anything using foo.caller. Followup patch in this bug coming.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: followup fix for ultimate web compat

Comment 11

[Brendan Eich [:brendan]]

Attachment: variation on Igor's test with expected 'global' result printed, with patch

Without patch, bad times.

/be

Comment 12

[Brendan Eich [:brendan]]

Double-review just cuz igor dropped off IRC and I'd like to land this on tm ASAP.

/be

Comment 13

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/9a28ad13363e

Need both patches, of course, for m-c and 1.9.1. Sorry to double-patch. Thanks,

/be

Comment 14

[Igor Bukanov]

(In reply to comment #11)
> Created an attachment (id=381826) [details]
> variation on Igor's test with expected 'global' result printed, with patch

IMO just throwing for now is a better option. We do not know if there are web sites that need it and just throwing gives very strong hint about deprecated .caller.

Comment 15

[Igor Bukanov]

Comment on attachment 381825 [details] [diff] [review]
followup fix for ultimate web compat

>@@ -1343,11 +1343,19 @@ fun_getProperty(JSContext *cx, JSObject 
> 
>       case FUN_CALLER:
...
>+            if (FUN_ESCAPE_HAZARD(caller)) {
>+                JSObject *wrapper = WrapEscapingClosure(cx, fp, FUN_OBJECT(caller), caller);

Hm,  shouldn't it be fp->down, not fp?

Comment 16

[Brendan Eich [:brendan]]

I fixed the fp->down, sorry about flailing (again):

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

We know of frameworks such as TIBET, which use foo.caller. It's really way too late to give strong hints by breaking old de-facto standard APIs. I should have cottoned to this right away.

/be

Comment 17

[Igor Bukanov]

(In reply to comment #14)
> IMO just throwing for now is a better option. We do not know if there are web
> sites that need it and just throwing gives very strong hint about deprecated
> .caller.

Plus note that that WrapEscapingClosure does not provide the full compatibility. There are well known issues with it as in many corner cases it does not preserve full compatibility with FF 3.0. That is why I suggest to wait for real bug reports before doing that.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)
> (In reply to comment #14)
> > IMO just throwing for now is a better option. We do not know if there are web
> > sites that need it and just throwing gives very strong hint about deprecated
> > .caller.
> 
> Plus note that that WrapEscapingClosure does not provide the full
> compatibility. There are well known issues with it as in many corner cases it
> does not preserve full compatibility with FF 3.0. That is why I suggest to wait
> for real bug reports before doing that.

This is true but the risk is less if we wrap instead of throw -- non-recursive backtracing with function name reflection still works. IIRC this is how TIBET uses caller.

/be

Comment 19

[Igor Bukanov]

(In reply to comment #18)
> This is true but the risk is less if we wrap instead of throw -- non-recursive
> backtracing with function name reflection still works. IIRC this is how TIBET
> uses caller.

Right - I forgot that there is no problem with escaping closures, the problem is in calling them.  Ideally we should throw at the point of the call and such throwing would not interfere with scripts like TIBET. 

On the other hand there is no known way to detect escaped closures during the call without serious performance degradation. The solution that works is to make sure that any leak is detected. But at that point we do not know how the closure will be used which prevents just throwing an exception. So wrapping for web compatibility is in due.

Comment 20

[William J. Edney]

Brendan -

Thanks for adding me to the bug.

It is absolutely true that we use '.caller', particularly for stack tracing
both during development and as a way of offering tech support reps reasonable
stack data.

Furthermore, we have a concept of 'contextual' functions where the originating
calling context can be determined by walking the stack.

By way of example, we have a call $byId(), a *copy* of which can be placed in a
separate frame/window and then a slot is placed on that Function instance that
defines where it was installed. So, when the author invokes $byId('fooElem'),
the window/document context does not have to be passed. $byId() is then free to
invoke TIBET functionality over in the 'code frame' and the originating context
can been determined by walking the stack back (as you may know, TIBET is loaded
in a completely separate frame which 'stays resident' throughout the entire
course of the application run).

Cheers,

- Bill

Comment 21

[Brendan Eich [:brendan]]

Bill, I have no idea whether the optimized closure work, even with this bug as a compatibility partial fix, breaks you or not. Have you run TIBET in 3.5 beta 4? Could you try the next tracemonkey automated build after the commit of this bug's third patch (which was at 12:24 PDT)?

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/

Please report anything broken. If in doubt, post here. Thanks,

/be

Comment 22

[William J. Edney]

Brendan -

TIBET runs wonderfully in 3.5b4, which now gives me an opportunity to say 'thank you!' to you and all of the folks who worked so **** Tracemonkey :-).

I will grab a build later on today and report any issues immediately.

Thanks!

Cheers,

- Bill

Comment 23

[Igor Bukanov]

Food for thoughts: an alternative to creating a wrapper with patched code is to have a wrapper that captures all the variables from the display that the closure may access. Then, when called, such wrapper would reconstruct the display populating it with pseudo-frames and then it will call the original closure.

Comment 24

[Igor Bukanov]

(In reply to comment #22)
> TIBET runs wonderfully in 3.5b4

The problem is that it turned out that one particular optimization that is present 3.5b4 had a hole. Fixes to repair it are rather non-trivial. Moreover, the landed fixes (like this bug) to make the optimization sound is not compatible with FF 3.0 or even 3.5b4 behavior with some corner cases of functionName.caller usage. Thus it is important to know if those cases happens in practice or not.

Comment 25

[William J. Edney]

Guys -

Still have positive function on TIBET's use of stack walking as of this build:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090605 Minefield/3.6a1pre

which I pulled from here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/tracemonkey-macosx/1244235502/

those builds have a timestamp of 14:19.

Please let me know if I got the right one ;-).

Cheers,

- Bill

FYI, here's the salient TIBET code:

    stackArray = [];

    try
    {
        //  NOTE:   this will DIE on certain top-level functions unless
        //          you're running a version of Mozilla which has fixes for
        //          Mozilla Bug #119529. NOTE also that it does so silently
        //          and that try/catch *won't* trigger, even a finally
        //          clause fails to be executed (probably a bug itself).
        //          Other Mozilla bugs related to Function.caller (*not*
        //          arguments.caller), include Mozilla Bug #168081 and
        //          Mozilla Bug #222029.
        startCaller = arguments.callee.caller;

        //  Since we started by already going one level up to the
        //  caller, we start our count at 1 to get the correct number of
        //  frames.
        for (i = 1; i < skip; i++)
        {
            if (isValid(startCaller.caller))
            {
                startCaller = startCaller.caller;
            };
        };

        theCaller = startCaller;
        while (isValid(theCaller) &&
                (stackArray.length <= $cfg('stack.backtrace_max')))
        {
            stackArray.push(theCaller);
            if (theCaller.caller == theCaller)
            {
                break;
            };

            theCaller = theCaller.caller;
        };
    }
    catch (e)
    {
    };

Comment 26

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1138b052566f

Comment 27

[Mike Shaver (:shaver emeritus)]

This patch has been implicated in bug 497355, which is a problem for Thunderbird if not other apps.

Comment 28

[Igor Bukanov]

Comment on attachment 381825 [details] [diff] [review]
followup fix for ultimate web compat

forgotten r+