Closed Bug 583757 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta4+

People

(Reporter: dan, Assigned: dvander)

References

()

Details

(Keywords: regression, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files)

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.
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.
My initial guess is bug 495331, but checking now.
Yep.  Local backout says the patch for bug 495331 is responsible
Blocks: 495331
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Keywords: regression
Summary: Javascript closures : closed-over values become unexpectedly undefined. → TM: Javascript closures : closed-over values become unexpectedly undefined.
blocking2.0: ? → beta4+
Oh no, not again.
Assignee: general → 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);
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.
Assignee: jorendorff → dvander
Attached patch fixSplinter Review
spot fix until sept 1st
Attachment #466173 - Flags: review?(jorendorff)
Comment on attachment 466173 [details] [diff] [review]
fix

Add the trace-test. r=me with that.
Attachment #466173 - Flags: review?(jorendorff) → review+
http://hg.mozilla.org/tracemonkey/rev/870371756071
Status: NEW → ASSIGNED
Whiteboard: fixed-in-tracemonkey
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.
This patch is wafer thin and could be cherry-picked, IINM.

/be
http://hg.mozilla.org/mozilla-central/rev/7802c6c281e2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.