583757 - TM: Javascript closures : closed-over values become unexpectedly undefined.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dan Shoutis]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.8) Gecko/20100723 Ubuntu/10.04 (lucid) Firefox/3.6.8
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b3pre) Gecko/20100728 Ubuntu/10.04 (lucid) MozillaDeveloperPreview/4.0b3pre

Apologies for the non-minimized testcase. Errors are showing up in 'ntimes_apply', defined on line 290 of cfa.js; you can see some alerts designed to catch the error there.

Expected behaviour, and what I'm seeing on other browsers and FF 3.6.8: the 'result' function has appropriate access to the values passed into 'count', 'action', and 'adj' from its containing function.

With the FF4 nightly and mozilla-central checkout with JIT enabled: those variables become undefined at some point during execution. With JIT disabled, the results are as expected.

There's some extra trap code on line 695-703 which causes them to become undefined immediately. Commenting this code out allows progress, but during execution the problem still appears.

Adding an extra custom property to the 'result' function (line 320) makes the bug disappear entirely.

It is possible the issue is related to drawing to the canvas or using setTimeout: I have been unable to reproduce the bug using the standalone js shell from the mozilla-central build. ('cat cfa.js standalone-wrapper.js | path/to/shell/js')

The beginnings of my attempt to isolate a smaller test case are in ff4-isolate.js; however it has yet to trigger the bug on my machine.

A possibly related(identical?) bug, with variables in a closure becoming undefined during execution and specific to JIT: https://bugzilla.mozilla.org/show_bug.cgi?id=581223

Sidebar for code reading: http://dl.dropbox.com/u/6600185/cfa/cfademo.html has a writeup with some code architecture discussion at at the bottom in 'Under the Hood'. (This page also evidences the bug). My heavy-handed use of continuation-passing style and trampolining is why it happens to be so closure-heavy.

Reproducible: Always

Steps to Reproduce:
1. Unzip & load cfademo.html in a browser
2. Notice debugging alerts about things becoming undefined.

Actual Results:  
Variables 'count', 'action', 'adj' all become undefined during execution. The guard on l308 of cfa.js catches it and alerts.

Expected Results:  
A pretty picture of some trees drawn dynamically in a canvas. No alerts.

Comment 1

[Boris Zbarsky [:bzbarsky]]

m-c regression range dates back to end of January: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=49cc24d8bd6b&tochange=f57b95afb57e

This includes a TM merge, as expected.

Comment 2

[Boris Zbarsky [:bzbarsky]]

My initial guess is bug 495331, but checking now.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase zip file

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yep.  Local backout says the patch for bug 495331 is responsible

Comment 5

[Jason Orendorff [:jorendorff]]

Oh no, not again.

Comment 6

[Jason Orendorff [:jorendorff]]

var arr = [];

function f() {}

function g(n, h) {
    var a = f;
    if (n <= 0)
	return f;

    var t = g(n - 1, h);
    var r = function(x) {
	if (x)
	    return a;
	return a(h(function() { return t(); }));
    };
    arr.push(r); 
    return r;
}

g(80, f);
g(80, f);
g(80, f);
for (var i = 0; i < arr.length; i++)
    assertEq(arr[i](1), f);

Comment 7

[Jason Orendorff [:jorendorff]]

It's recursion. If you change

     if (n <= 0)
+    {
+        eval("0");
         return f;
+    }

Then it aborts recording the particular trace that causes the bug.

dvander says, turn off tracing recursion in heavyweight functions. Or just wait until Sept. 1 and this will go away.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

spot fix until sept 1st

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 466173 [details] [diff] [review]
fix

Add the trace-test. r=me with that.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Should we continue to wait for the merge here? Just trying to tell if this extends code freeze for beta 4 or if I can tell build to get rolling tomorrow morning.

Comment 12

[Brendan Eich [:brendan]]

This patch is wafer thin and could be cherry-picked, IINM.

/be

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7802c6c281e2