Fix Array.prototype.splice ES6 compliance.

• Component: JavaScriptCore
• Status: REOPENED ---

[Mark Lam]

Our Array.prototype.splice implementation neglected to set length on the result array (step 12 of https://tc39.github.io/ecma262/#sec-array.prototype.splice) in a certain code path.  Will fix.

[Mark Lam]

Created attachment 291430 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 291430 [details]
proposed patch.

r=me

[Geoffrey Garen]

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:939
> +    // should never exceed 1 ^ 53 - 1. However,

2^53-1

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:940
> +    // 1. our runtime does not allow the arrays lengths to exceed UINT_MAX. Hence, length <= UINT_MAX.

arrays'

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:944
> +    // Hence, it is impossible for length + insertCount - actualDeleteCount to get anywhere near 1^53-1.

2^53-1

[Mark Lam]

(In reply to comment #3)
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:939
> > +    // should never exceed 1 ^ 53 - 1. However,
> 
> 2^53-1

Thanks for catching these.

> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:940
> > +    // 1. our runtime does not allow the arrays lengths to exceed UINT_MAX. Hence, length <= UINT_MAX.
> 
> arrays'

I'll change this to "allow array lengths".

[Mark Lam]

Created attachment 291431 [details]
proposed patch (after more offline feedback from Yusuke).

[Build Bot]

Comment on attachment 291431 [details]
proposed patch (after more offline feedback from Yusuke).

Attachment 291431 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2273922

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 291434 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 291431 [details]
proposed patch (after more offline feedback from Yusuke).

Attachment 291431 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2273933

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 291435 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 291431 [details]
proposed patch (after more offline feedback from Yusuke).

Attachment 291431 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2273932

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 291436 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Yusuke Suzuki]

Comment on attachment 291431 [details]
proposed patch (after more offline feedback from Yusuke).

View in context: https://bugs.webkit.org/attachment.cgi?id=291431&action=review

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:940
> +    if (UNLIKELY(static_cast<uintptr_t>(length) + insertCount - actualDeleteCount < ((1ull << 53) - 1))) {

`< ((1ull << 53) - 1))` is always true, right?
And I think `length` should be uint64_t OR double to preserve its value when it is larger than UINT_MAX.
So we need to change getLength() implementation not to call toUInt32().

[Build Bot]

Comment on attachment 291431 [details]
proposed patch (after more offline feedback from Yusuke).

Attachment 291431 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2273945

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 291438 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Mark Lam]

(In reply to comment #12)
> Comment on attachment 291431 [details]
> proposed patch (after more offline feedback from Yusuke).
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291431&action=review
> 
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:940
> > +    if (UNLIKELY(static_cast<uintptr_t>(length) + insertCount - actualDeleteCount < ((1ull << 53) - 1))) {
> 
> `< ((1ull << 53) - 1))` is always true, right?

This s wrong.  Should be >, not <.

> And I think `length` should be uint64_t OR double to preserve its value when
> it is larger than UINT_MAX.
> So we need to change getLength() implementation not to call toUInt32().

You are right.  This is trickier than I thought.  I'll look into it.

[Mark Lam]

(In reply to comment #15)
> > And I think `length` should be uint64_t OR double to preserve its value when
> > it is larger than UINT_MAX.
> > So we need to change getLength() implementation not to call toUInt32().
> 
> You are right.  This is trickier than I thought.  I'll look into it.

On further consideration, this part is not worth fixing at present.  It only manifests as an ES6 compliance issue if we make a contrived example, e.g.

    var maxLength = (2 ** 53) - 1;
    Array.prototype.splice.call({ length: maxLength }, maxLength, 0, 1, 2, 3);

In real world code with arrays being spliced together, we would have gotten an OutOfMemoryError.  FYI, Chrome also does not presently implement this as spec'ed correctly.

In the interest of focusing on more important bugs, I will defer fixing step 8.  I filed https://bugs.webkit.org/show_bug.cgi?id=163417 to track this for later.

Let's get the rest of this patch landed.  I will clean it up and re-upload shortly.

[Mark Lam]

Created attachment 291544 [details]
proposed patch.

[Geoffrey Garen]

Comment on attachment 291544 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=291544&action=review

r=me

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:982
> +    unsigned itemCount = insertCount;

Why two variables for the same value? I would pick either itemCount or insertCount as the name, and then declare once above.

[Mark Lam]

(In reply to comment #18)
> Comment on attachment 291544 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291544&action=review
> 
> r=me

Thanks.

> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:982
> > +    unsigned itemCount = insertCount;
> 
> Why two variables for the same value? I would pick either itemCount or
> insertCount as the name, and then declare once above.

The spec uses 2 variables.  I'm keeping it consistent with the spec so that it's easier to compare the code to review whether we're missing something.

[Yusuke Suzuki]

r=me too!

[Mark Lam]

Thanks for the reviews.  The patch has passed the JSC and layout tests.  Landed in r207322: <http://trac.webkit.org/r207322>.

[Joseph Pecoraro]

I'm seeing a JSC stress test failure in trunk r207327:

stress/array-prototype-splice-making-typed-array.js.no-llint: Exception: TypeError: Attempted to assign to readonly property.
stress/array-prototype-splice-making-typed-array.js.no-llint: splice@[native code]
stress/array-prototype-splice-making-typed-array.js.no-llint: array-prototype-splice-making-typed-array.js:20:27
stress/array-prototype-splice-making-typed-array.js.no-llint: [e-mail]:8:10
stress/array-prototype-splice-making-typed-array.js.no-llint: global [e-mail]:11:5

Might be related to this change?

[Joseph Pecoraro]

> Might be related to this change?

Yep this seems to be the cause. I didn't investigate further, but it looks like the test expected this could change with a better splice implementation and references bug 159645.

[Mark Lam]

(In reply to comment #23)
> > Might be related to this change?
> 
> Yep this seems to be the cause. I didn't investigate further, but it looks
> like the test expected this could change with a better splice implementation
> and references bug 159645.

I'll look into it.

[Ryan Haddad]

Reverted r207322 for reason:

This change caused JSC test failures

Committed r207344: <http://trac.webkit.org/changeset/207344>