Closed Bug 989586 Opened 6 years ago Closed 6 years ago

new Array(length) return wrong size

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 + fixed
firefox30 + fixed
firefox31 + fixed

People

(Reporter: swediomurital, Assigned: jandem)

References

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.152 Safari/537.36

Steps to reproduce:

Firefox Linux i686; rv:28.0

HTML:

Array.length: <input type="text" id="arw1" />
Demanded length:<input type="text" id="arw2" />

JS:

var arw1 = document.getElementById('arw1'),
      arw2 = document.getElementById('arw2');

function arrMany(w, show) {
    var a = new Array(w);
    if (show) {
        arw1.value = a.length;
        arw2.value = w;
    }
    /* optionally something like
    if (a.length !== w) {
         console.log(a.length, w);
    }
    */
}

function tick() {
    arrMany(3, false);
    arrMany(4, false);
    arrMany(5, false);
    arrMany(10, true);
}

setInterval(tick, 1000 / 300);


Actual results:

After some time, 20+ seconds, length of new Array(10) returns size of *previous* call. I.e. 5 instead of 10.


Expected results:

Length should be the one specified.

Notes:

1. No action in Browser Console or Web Console.
2. If Web Console is open the error never occurs.
3. If Web Console is opened when the error manifests, it corrects itself at once.
4. Given pt. 3. it is rather hard to debug in a sane way using the browser itself.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Still reproducible in latest trunk (tip 0e19655e93df). Not reproducible with ion turned off.

Expected: throws "ok"
Actual: typein:5:4 Error: Assertion failed: got 5, expected 3: iter: 4725

shell test case:
---
function t() {
  var iter = 0;
  function a(w) {
    var a = new Array(w);
    assertEq(a.length, w, "iter: " + ++iter);
    if (iter === 1000000) throw "ok";
  }
  function r() {
    a(3); a(4); a(5); a(10);
  }
  for(;;) r();
}
---
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jdemooij)
Curious. Note that additional invocations of t() in the shell testcase assert on iteration 1 or 2. Always with "got 3, expected 4".

Except if the shell is started with --ion-eager, then it's always "got 3, expected 4".

The iteration in which the assert happens first varies: Without --ion-eager I saw values between 5293 and 8053, with --ion-eager between 1098 and 1338.
Component: JavaScript Engine → JavaScript Engine: JIT
It starts being wrong after compilation of 'r', which inlines 'a' and uses MNewArray instead of a call to Array's constructor. Looking at visitNewArray in CodeGenerator + the fact that it shows up after different iterations counts lets me think it is GC related, if not a GC bug. Adding a call to gc() anywhere in r() seems to make the problem disappear.


function a(w) {
    var a = new Array(w);
    assertEq(a.length, w)
}
function r() {
    a(3); a(4);
}
for(;;) r();
Regression from bug 922270. Consider this line:

    var a = new Array(w);

The Baseline IC will create the array template object with length == 3, and we'll use this template object in Ion, even for the w == 4 case.
Blocks: 922270
Flags: needinfo?(jdemooij) → needinfo?(bhackett1024)
Attached patch PatchSplinter Review
Might as well fix this myself.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8399586 - Flags: review?(bhackett1024)
Thanks a lot for the bug report and shell testcase btw.
Flags: needinfo?(bhackett1024)
Very safe fix, we should backport this as it can break real-world websites.
Attachment #8399586 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/37d8a465a7b8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment on attachment 8399586 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 922270.
User impact if declined: Broken websites.
Testing completed (on m-c, etc.): On m-i and m-c.
Risk to taking this patch (and alternatives if risky): Very low.
String or IDL/UUID changes made by this patch: None.
Attachment #8399586 - Flags: approval-mozilla-beta?
Attachment #8399586 - Flags: approval-mozilla-aurora?
Attachment #8399586 - Flags: approval-mozilla-beta?
Attachment #8399586 - Flags: approval-mozilla-beta+
Attachment #8399586 - Flags: approval-mozilla-aurora?
Attachment #8399586 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.