473271 - TM: nested-closure array indexing problem when setting object properties in loop

Status: VERIFIED DUPLICATE of bug 452498

(Core :: JavaScript Engine, defect, P3)

Description

[Ben Newman (:bnewman) (:benjamn)]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090112 Minefield/3.2a1pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090112 Minefield/3.2a1pre

See attachment for minimized testcase.

Any of these modifications make the problem disappear:

 1. referring to $methodName (or even arguments[0]) in the hello function before returning the howdy closure
 2. unrolling the for-loop
 3. returning an anonymous function instead of declaring/returning the function named howdy

Reproducible: Always

Steps to Reproduce:
1. Load attached page
2. Note contents of alert message
Actual Results:  
Contents are "FAIL"

Expected Results:  
Contents should be "HIGH PASS"

Comment 1

[Ben Newman (:bnewman) (:benjamn)]

Attachment: minimized testcase

Comment 2

[Ben Newman (:bnewman) (:benjamn)]

Attachment: clearing up some confusion held over from the original test case

Changed the array to [0, 1, 2, 3] because ['EPIC FAIL', 'FAIL', 'PASS', 'HIGH PASS'] was misleading ('PASS' was a failing result, for instance).  The expected result is now 3, whereas the latest nightly returns 1.

Comment 3

[Jesse Young]

Attachment: even simpler version

Thanks Ben!

I played around a little more and made an even simpler test case. The bug happens even if the code isn't inside the prototype, and even if there is no array (just a for loop that iterates more than twice).

Comment 4

[Andreas Gal :gal]

"Iterates more than twice" smells like jit bug.

Comment 5

[Andreas Gal :gal]

Confirmed for TM tip. Great testcase. Thanks.

Comment 6

[Andreas Gal :gal]

We have to abort on DEFLOCALFUN if anything is left to capture as we walk past it. The upvar patch (452498) will copy the value into a flat closure for howdy every time. We can't fix this until 452498 is in the bag, so adding a dependency and we will re-visit.

Comment 7

[Andreas Gal :gal]

Blocking final. I don't think we need a beta for this though. Test case is straight forward.

Comment 8

[Brendan Eich [:brendan]]

Shell testcase:

var f = {}, max = 3;

var hello = function(n) {
    function howdy() { return n }
    f.test = howdy;
};

for (var i = 0; i <= max; i++)
    hello(i);

print(f.test() + " (expected " + max + ")");

I have no idea how bad this burns beta testers, but I'm gonna fix for b3.

/be

Comment 9

[Damon Sicore (:damons)]

Who owns this?

Comment 10

[Andreas Gal :gal]

I will grab it and re-test when brendan lands the upvar patch.

Comment 11

[Robert Sayre]

This wfm with the latest upvar patch applied

Comment 12

[Robert Sayre]

assigning to brendan since upvar fixes this

Comment 13

[Brendan Eich [:brendan]]

Added test to tm's trace-test.js:

http://hg.mozilla.org/tracemonkey/rev/0e3f7afa0f0e

/be

Comment 14

[Bob Clary [:bc] (inactive)]

js1_8_1/trace/trace-test.js	
http://hg.mozilla.org/tracemonkey/rev/61892f57b46a

Comment 15

[Bob Clary [:bc] (inactive)]

cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/js/tests/shell.js,v  <--  shell.js

Comment 16

[Bob Clary [:bc] (inactive)]

testMoreClosures v1.9.3