Bug 163372

Summary: Fix Array.prototype.splice ES6 compliance.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: REOPENED ---    
Severity: Normal CC: buildbot, fpizlo, ggaren, jfbastien, joepeck, keith_miller, Mostafa.Shahdadi, msaboff, rniwa, ryanhaddad, sbarati, ysuzuki
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
ysuzuki: review+
proposed patch (after more offline feedback from Yusuke).
mark.lam: review-, buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-yosemite
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews112 for mac-yosemite
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
proposed patch. ggaren: review+

Description Mark Lam 2016-10-12 17:23:25 PDT
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.
Comment 1 Mark Lam 2016-10-12 17:27:03 PDT
Created attachment 291430 [details]
proposed patch.
Comment 2 Yusuke Suzuki 2016-10-12 17:33:31 PDT
Comment on attachment 291430 [details]
proposed patch.

r=me
Comment 3 Geoffrey Garen 2016-10-12 17:34:17 PDT
> 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
Comment 4 Mark Lam 2016-10-12 17:37:58 PDT
(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".
Comment 5 Mark Lam 2016-10-12 18:06:57 PDT
Created attachment 291431 [details]
proposed patch (after more offline feedback from Yusuke).
Comment 6 Build Bot 2016-10-12 18:49:02 PDT
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.
Comment 7 Build Bot 2016-10-12 18:49:05 PDT
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
Comment 8 Build Bot 2016-10-12 18:54:42 PDT
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.
Comment 9 Build Bot 2016-10-12 18:54:45 PDT
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
Comment 10 Build Bot 2016-10-12 18:58:42 PDT
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.
Comment 11 Build Bot 2016-10-12 18:58:45 PDT
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
Comment 12 Yusuke Suzuki 2016-10-12 18:59:11 PDT
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().
Comment 13 Build Bot 2016-10-12 19:08:37 PDT
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.
Comment 14 Build Bot 2016-10-12 19:08:40 PDT
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
Comment 15 Mark Lam 2016-10-12 19:49:45 PDT
(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.
Comment 16 Mark Lam 2016-10-13 16:18:20 PDT
(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.
Comment 17 Mark Lam 2016-10-13 17:04:22 PDT
Created attachment 291544 [details]
proposed patch.
Comment 18 Geoffrey Garen 2016-10-13 17:07:31 PDT
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.
Comment 19 Mark Lam 2016-10-13 17:09:06 PDT
(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.
Comment 20 Yusuke Suzuki 2016-10-13 17:09:45 PDT
r=me too!
Comment 21 Mark Lam 2016-10-13 22:32:50 PDT
Thanks for the reviews.  The patch has passed the JSC and layout tests.  Landed in r207322: <http://trac.webkit.org/r207322>.
Comment 22 Joseph Pecoraro 2016-10-14 01:59:49 PDT
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: test@array-prototype-splice-making-typed-array.js:8:10
stress/array-prototype-splice-making-typed-array.js.no-llint: global code@array-prototype-splice-making-typed-array.js:11:5

Might be related to this change?
Comment 23 Joseph Pecoraro 2016-10-14 02:19:48 PDT
> 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.
Comment 24 Mark Lam 2016-10-14 08:33:21 PDT
(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.
Comment 25 Ryan Haddad 2016-10-14 09:26:25 PDT
Reverted r207322 for reason:

This change caused JSC test failures

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