676343 - Lexical scoping bug in Firefox 6

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Danny Yoo]

Attachment: lexical-scope-ff6.tar.gz

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/534.30 (KHTML, like Gecko) Chrome/12.0.742.124 Safari/534.30

Steps to reproduce:

I'm writing a Racket-to-JavaScript compiler as a part of my research work (http://hashcollision.org/whalesong).

I ran across what looks suspiciously like a bug in Firefox 6's JavaScript engine in some research code I've been writing.  Unfortunately, I have not yet been able to reduce the problem to a small case.

I wrote a function like this within one of my projects:

var adaptWorldFunction = function(worldFunction) {
    return function() {
        // Consumes any number of arguments.
        var successFunction = arguments[arguments.length - 1];
        plt.baselib.functions.internalCallDuringPause.apply(
            null,
            [MACHINE,
             worldFunction,
             function(v) {
                 successFunction(v);
             },
             function(err) {
                 // FIXME: do error trapping
                 throw(err);
             }].concat([].slice.call(arguments, 0, arguments.length - 1)));
    }
};



Actual results:


I received an unexpected error message about successFunction not being defined.  This made me suspicious, so I modified the code to be:


var adaptWorldFunction = function(worldFunction) {
    return function() {
        // Consumes any number of arguments.
        var successFunction = arguments[arguments.length - 1];
        var testScope = 42;
        plt.baselib.functions.internalCallDuringPause.apply(
            null,
            [MACHINE,
             worldFunction,
             function(v) {
                 alert(testScope);
                 successFunction(v);
             },
             function(err) {
                 // FIXME: do error trapping
                 throw(err);
             }].concat([].slice.call(arguments, 0, arguments.length - 1)));
    }
};


I received the same kind of error message, but this time with regards to testScope.
Within the context of my large project, lexical scope seems to be breaking.


When I finally relented and changed my definition to:

var adaptWorldFunction = function(worldFunction) {
   var successFunction;
   return function() {
       // Consumes any number of arguments.
       successFunction = arguments[arguments.length - 1];
       plt.baselib.functions.internalCallDuringPause.apply(
           null,
           [MACHINE,
            worldFunction,
            function(v) {
                successFunction(v);
            },
            function(err) {
                // FIXME: do error trapping
                throw(err);
            }].concat([].slice.call(arguments, 0, arguments.length - 1)));
   }
};


that apparently worked and avoided whatever lexical scoping bug I'm running into.  


This behavior does not affect Firefox 3 or 4, to the best of my knowledge; it's something that came up when I tested against Firefox 6.

I'm including my project files to this bug report.  The function 'adaptWorldFunction' is within talk-firefox6-bad.js.  If you change that function and lift the definition of successFunction up one level, the web page in talk-firefox-6-bad.html should run successfully.

Comment 1

[Jason Orendorff [:jorendorff]]

Confirmed.

Comment 2

[Jason Orendorff [:jorendorff]]

I wonder if this involves UPVAR_LEVEL_LIMIT. I have a partly reduced test case, currently 8000+ lines, and it ends with

						RUNTIME.trampoline(MACHINE, _start902);
					    })(MACHINE, function() { plt.runtime.setReadyTrue();SUCCESS(); }, FAIL, PARAMS);
					}, FAIL, PARAMS);
				    }, FAIL, PARAMS);
				}, FAIL, PARAMS);
			    }, FAIL, PARAMS);
			}, FAIL, PARAMS);
		    }, FAIL, PARAMS);
		}, FAIL, PARAMS);
	    }, FAIL, PARAMS);
	});
    });
    return invoke;
})()(plt.runtime.currentMachine, function() {}, function() {}, {});

plt.runtime.invokeMains(plt.runtime.currentMachine,
                        function() { alert('done');},
                        function(MACHINE, err) { alert(plt.baselib.format.format("~a", [err])); });
console.log("got this far");

Comment 3

[Jason Orendorff [:jorendorff]]

The number of functions enclosing the point where the name lookup fails is 17.

UPVAR_LEVEL_LIMIT is 16.

Could be a coincidence.

Comment 4

[Jason Orendorff [:jorendorff]]

Shell testcase:

function f() {
    return function () { return function () { return function () {
    return function () { return function () { return function () {
    return function () { return function () { return function () {
    return function () { return function () { return function () {
    return function () { return function () { return function () {
	return function (a) {
            var v = a;
	    assertEq(v, 42);
	    return function() { return v; };
	};
    }; }; }; }; }; }; }; }; }; }; }; }; }; }; };
};

assertEq(f()()()()()()()()()()()()()()()()(42)(), 42);

Comment 5

[Jason Orendorff [:jorendorff]]

You can trim one of those nested functions and still get the error. But only one. I set a breakpoint and confirmed that this is the minimum amount of nesting that will cause this if-statement in BindNameToSlot to hit:

        if (level >= UpvarCookie::UPVAR_LEVEL_LIMIT)
            return JS_TRUE;

Here's the slightly shorter testcase, fwiw:

function f() {
    return function () { return function () { return function () {
    return function () { return function () { return function () {
    return function () { return function () { return function () {
    return function () { return function () { return function () {
    return function () { return function () { return function (a) {
        var v = a;
	assertEq(v, 42);
	return function() { return v; };
    }; }; }; }; }; }; }; }; }; }; }; }; }; }; };
};

assertEq(f()()()()()()()()()()()()()()()(42)(), 42);

Comment 6

[Jason Orendorff [:jorendorff]]

The code for `function() { return v; }` is:

  00019:  lambda_fc (function () {return v;})
  00022:  nullblockchain

but the code for `v` in that function is:

  00000:  name "v"

Cc-ing Brendan and going to bed.

Comment 7

[Danny Yoo]

Darn.  I'm running into the same category of lexical-scoping bug in Firefox 3.5.1.6 as well.  See: http://hashcollision.org/whalesong/examples/where-am-i/where-am-i.xhtml.  As soon as you allow location updates, you'll see a "proc is not defined" error, but the code in question:

    var wrapFunction = function(proc) {
        var f = function(MACHINE) {
            var p = proc;         // this line raises a "proc is not defined" error
            ...
        }
        ...
    }

is in a context where proc should be lexically bound.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: v1

This seems to fix it.

I claim the assertion I'm deleting here is left over from JSContext::display days and is now nugatory.

(That being the case, I don't think this patch would be safe for branches that still have cx->display.)

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 557227 [details] [diff] [review]
v1

I had a dream within the last few months about checking the absolute instead of the delta against the delta-max. Thanks for fixing.

Which supported releases still have the display?

/be

Comment 10

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1a29139e6baf

The display was removed in July 2010, so the current release (ff6) does not have it, much less beta or aurora.

That means this fix should apply pretty cleanly to any of those. Requesting approval...

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 557227 [details] [diff] [review]
v1

This bug causes the JS engine to give the wrong answer for certain complex (usually compiler-generated) JS code. It's not a security bug, but it is a very subtle correctness bug with a straightforward fix. I think we should take the fix on aurora and beta.

Comment 12

[Danny Yoo]

I'm glad that the bug will be fixed in future versions of Firefox.

In the meantime, is there a particular way I can work around this bug?  One of the constraints I'm trying to maintain is that my compiler will produce output that works across a multitude of browsers, including legacy ones like Firefox 3.6.  One of the domains for the compiler are school computers, where it's very hard for me to demand that schools upgrade their browsers.

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 557227 [details] [diff] [review]
v1

Hard to see why this is something that needs to bypass the trains. If you've got further reasoning, please re-nomininate and we'll evaluate.

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to Danny Yoo from comment #12)
> In the meantime, is there a particular way I can work around this bug?

I think you can probably work around this bug by putting
  eval();
in each JS function nested 16-deep or greater.

Maybe your compiler already has an optimization pass to reduce the nesting depth of functions in the output. If not, doing that could also help. I think FF3.6 had a hard limit of something like 200 nested functions, so minimizing nesting depth was necessary.

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1a29139e6baf