Last Comment Bug 676343 - Lexical scoping bug in Firefox 6
: Lexical scoping bug in Firefox 6
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 6 Branch
: x86 Linux
: -- normal (vote)
: mozilla9
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
: 640880 686472 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-03 12:02 PDT by Danny Yoo
Modified: 2011-09-14 01:45 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
affected


Attachments
lexical-scope-ff6.tar.gz (174.85 KB, application/octet-stream)
2011-08-03 12:02 PDT, Danny Yoo
no flags Details
v1 (2.94 KB, patch)
2011-08-31 10:16 PDT, Jason Orendorff [:jorendorff]
brendan: review+
asa: approval‑mozilla‑aurora-
asa: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Danny Yoo 2011-08-03 12:02:43 PDT
Created attachment 550457 [details]
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] 2011-08-03 12:26:45 PDT
Confirmed.
Comment 2 Jason Orendorff [:jorendorff] 2011-08-03 19:15:18 PDT
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] 2011-08-03 19:26:42 PDT
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] 2011-08-03 19:50:04 PDT
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] 2011-08-03 19:57:57 PDT
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] 2011-08-03 20:01:51 PDT
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 2011-08-30 15:04:26 PDT
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] 2011-08-31 10:16:28 PDT
Created attachment 557227 [details] [diff] [review]
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] 2011-08-31 14:05:57 PDT
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] 2011-09-01 10:06:13 PDT
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] 2011-09-01 10:10:19 PDT
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 2011-09-01 13:36:21 PDT
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] 2011-09-01 14:25:25 PDT
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 14 Jan de Mooij [:jandem] 2011-09-01 14:40:43 PDT
*** Bug 640880 has been marked as a duplicate of this bug. ***
Comment 15 Jason Orendorff [:jorendorff] 2011-09-02 07:16:42 PDT
(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 17 Jan de Mooij [:jandem] 2011-09-14 01:45:08 PDT
*** Bug 686472 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.