Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array#splice bug with large arrays in Safari 7 and 8 #295

Closed
Krinkle opened this issue Mar 3, 2015 · 3 comments
Closed

Array#splice bug with large arrays in Safari 7 and 8 #295

Krinkle opened this issue Mar 3, 2015 · 3 comments
Assignees

Comments

@Krinkle
Copy link
Contributor

Krinkle commented Mar 3, 2015

This is fixed in the latest version of JavaScriptCore in WebKit Nightly, and got fixed in V8 several years ago in Chromium. But the stable releases of Safari 7 and 8 still have this bug.

// This returns true in Safari 8
function spliceBrokenSafari() {
  var a = new Array(100000);
  a.splice(30, 0, 'x');
  a.splice(20, 1);
  return a.indexOf('x') !== 29;
}

In the versions of JavaScriptCore with this bug, the index is 30 instead of 29.

We had to workaround this for the time being in wikimedia/VisualEditor@d468b00

@catrope wrote:

For Safari I know the parameters pretty well (array has to be sparse, has to have length >=100k, the 'x' has to be at offset >= 8), for Opera 12 I don't know them at all other than that that specific example fails

@catrope
Copy link

catrope commented Mar 3, 2015

Clarification: the example above isn't the one that fails in Opera, that's a different one:

function spliceBrokenOpera() {
    var n = 256,
        a = [];
    a[n] = 'a';
    a.splice( n + 1, 0, 'b' );
    return a[n] !== 'a';
}

@ljharb
Copy link
Member

ljharb commented Jun 20, 2015

So, the Safari test is about brokenness with a.splice(20, 1) on a large sparse array.

Reading the steps in https://es5.github.io/#x15.4.4.12, specifically 12:

  • k is 20, itemCount is 0, actualDeleteCount is 1, len is 100001
  • (len - actualDeleteCount) will always be greater than k for the intents of this stepthrough
  • from is '21', and to is '20'
  • fromPresent is false since the only index present in the array is 30
  • thus, do delete a[to], ie, a[20] (which is already nonexistent)
  • This will continue to iterate, deleting until fromPresent is true, which will be when from is 30, which means k is 29, and to is 29
  • then, a[to] = a[from], or a[29] = a[30]
  • continuing to iterate, a[30] will be deleted, because a[31] is not present
  • This will stop when k is 100000, and then k is set to 100001
  • while k > (len – actualDeleteCount +itemCount) ie while k > (100001 - 1 + 0) or while k > 100000
    • delete a[k - 1] which deletes index 100000 (kind of useless in this case)
  • step 13, skipped. 14: k is 20
  • step 15, skipped
  • step 16, a.length = len - actualDeleteCount + itemCount ie a.length = 100000, and return a

So I've convinced myself that starting with an array of length 1e5, with index 30 set to some value, and then .splice(20, 1) should end up in an array of length 1e5 - 1, with that value at index 29. (I hate splice so much)

I'll test the Opera one similarly, while I figure out how the best way to fix this Safari bug.

@ljharb
Copy link
Member

ljharb commented Jun 20, 2015

Interestingly enough, the test detected the bug with length 1e5, 'x' index 8, splice start 1 - but when I dropped the index from 8 to 7, it failed to detect the bug. Probably some weird JIT behavior here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants