Lexical scoping bug in Firefox 6

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Danny Yoo, Assigned: jorendorff)

Tracking

6 Branch
mozilla9
x86
Linux
Points:
---

Firefox Tracking Flags

(firefox6 affected, firefox7 affected, firefox8 affected)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
(Assignee)

Comment 1

6 years ago
Confirmed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

6 years ago
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");
(Assignee)

Comment 3

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

UPVAR_LEVEL_LIMIT is 16.

Could be a coincidence.
(Assignee)

Comment 4

6 years ago
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);
(Assignee)

Comment 5

6 years ago
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);
(Assignee)

Comment 6

6 years ago
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.
(Reporter)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
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.)
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+
(Assignee)

Comment 10

6 years ago
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...
status-firefox6: --- → affected
status-firefox7: --- → affected
status-firefox8: --- → affected
Whiteboard: [inbound]
(Assignee)

Comment 11

6 years ago
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?
(Reporter)

Comment 12

6 years ago
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

6 years ago
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-

Updated

6 years ago
Duplicate of this bug: 640880
(Assignee)

Comment 15

6 years ago
(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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9

Updated

6 years ago
Duplicate of this bug: 686472
You need to log in before you can comment on or make changes to this bug.