Closed Bug 676343 Opened 8 years ago Closed 8 years ago

Lexical scoping bug in Firefox 6

Categories

(Core :: JavaScript Engine, defect)

6 Branch
x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9
Tracking Status
firefox6 --- affected
firefox7 --- affected
firefox8 --- affected

People

(Reporter: dyoo, Assigned: jorendorff)

References

Details

Attachments

(2 files)

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.
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Confirmed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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");
The number of functions enclosing the point where the name lookup fails is 17.

UPVAR_LEVEL_LIMIT is 16.

Could be a coincidence.
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);
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);
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.
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.
Attached patch v1Splinter Review
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.)
Assignee: general → jorendorff
Attachment #557227 - Flags: review?(cdleary)
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
Attachment #557227 - Flags: review?(cdleary) → review+
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...
Whiteboard: [inbound]
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.
Attachment #557227 - Flags: approval-mozilla-beta?
Attachment #557227 - Flags: approval-mozilla-aurora?
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 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.
Attachment #557227 - Flags: approval-mozilla-beta?
Attachment #557227 - Flags: approval-mozilla-beta-
Attachment #557227 - Flags: approval-mozilla-aurora?
Attachment #557227 - Flags: approval-mozilla-aurora-
Duplicate of this bug: 640880
(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.
http://hg.mozilla.org/mozilla-central/rev/1a29139e6baf
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Duplicate of this bug: 686472
You need to log in before you can comment on or make changes to this bug.