Bug 183172 - emitCodeToGetArgumentsArrayLength should not crash on PhantomNewArrayWithSpread
Summary: emitCodeToGetArgumentsArrayLength should not crash on PhantomNewArrayWithSpread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
: 183770 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-02-27 12:35 PST by André Bargull
Modified: 2018-05-22 00:37 PDT (History)
10 users (show)

See Also:


Attachments
Patch (7.42 KB, patch)
2018-04-26 12:14 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (7.76 KB, patch)
2018-04-26 13:12 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (7.55 KB, patch)
2018-05-01 06:56 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.80 MB, application/zip)
2018-05-01 08:41 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description André Bargull 2018-02-27 12:35:53 PST
SVN: rev228977
Build with: perl Tools/Scripts/build-jsc --jsc-only --debug


Executing the following test case leads to this assertion:
---
DFG ASSERTION FAILED: arguments->op() == CreateDirectArguments || arguments->op() == CreateScopedArguments || arguments->op() == CreateClonedArguments || arguments->op() == CreateRest || arguments->op() =
= NewArrayBuffer || arguments->op() == PhantomDirectArguments || arguments->op() == PhantomClonedArguments || arguments->op() == PhantomCreateRest || arguments->op() == PhantomNewArrayBuffer
../../Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp(71) : JSC::DFG::Node* JSC::DFG::emitCodeToGetArgumentsArrayLength(JSC::DFG::InsertionSet&, JSC::DFG::Node*, unsigned int, JSC::DFG::NodeOrigin)
---


Test case:
---
function f() {
    var a = [1];
    for (var i = 0; i < 1000000; ++i) {
        [...a].length;
    }
}
f();
---
Comment 1 Robin Morisset 2018-03-30 08:17:27 PDT
I was able to reproduce this. I am looking into it.
Comment 2 Robin Morisset 2018-04-26 12:10:53 PDT
I found the problem and discussed it with Saam.

DFGArgumentsEliminationPhase.cpp currently believes that allocations of NewArrayWithSpread can be deleted if they are only used by GetArrayLength, but when it then calls emitCodeToGetArgumentsArrayLength, the latter has no idea what to do with GetArrayLength.

I fix the problem by teaching emitCodeToGetArgumentsArrayLength how to deal with GetArrayLength. Because this requires emitting an Add that can overflow and thus exit, we also tell DFGArgumentsEliminationPhase to give up on eliminating a NewArrayWithSpread when it is used by a GetArrayLength that is not allowed to exit.
Comment 3 Robin Morisset 2018-04-26 12:14:44 PDT
Created attachment 338903 [details]
Patch
Comment 4 Saam Barati 2018-04-26 12:20:22 PDT
Comment on attachment 338903 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:326
> +                    if ((node->child1()->op() == NewArrayWithSpread || node->child1()->op() == PhantomNewArrayWithSpread)

PhantomNewArrayWithSpread is not possible at this point in the transformation

> Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:76
> +        unsigned countNonSpreadArguments = 0;

style nit: I would name this "numberOfNonSpreadArguments"

> JSTests/stress/length-of-new-array-with-spread.js:9
> +function f() {
> +    var a = [1, 2];
> +    for (var i = 0; i < 10000; ++i) {
> +        var l = [...a, 42, ...a].length;
> +        if (l != 5)
> +            throw "Wrong length: " + l;
> +    }
> +}
> +f();

Let's add some more tests:
where a may be a rest parameter, etc.
Comment 5 Robin Morisset 2018-04-26 13:12:02 PDT
Created attachment 338907 [details]
Patch
Comment 6 Robin Morisset 2018-04-26 13:13:13 PDT
(In reply to Saam Barati from comment #4)
> Comment on attachment 338903 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338903&action=review
> 
> > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:326
> > +                    if ((node->child1()->op() == NewArrayWithSpread || node->child1()->op() == PhantomNewArrayWithSpread)
> 
> PhantomNewArrayWithSpread is not possible at this point in the transformation

Right, I removed it from the test.

> > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:76
> > +        unsigned countNonSpreadArguments = 0;
> 
> style nit: I would name this "numberOfNonSpreadArguments"

Done.
 
> > JSTests/stress/length-of-new-array-with-spread.js:9
> > +function f() {
> > +    var a = [1, 2];
> > +    for (var i = 0; i < 10000; ++i) {
> > +        var l = [...a, 42, ...a].length;
> > +        if (l != 5)
> > +            throw "Wrong length: " + l;
> > +    }
> > +}
> > +f();
> 
> Let's add some more tests:
> where a may be a rest parameter, etc.

I've added two more tests, tell me if more are needed.
Comment 7 Saam Barati 2018-04-26 13:44:34 PDT
Comment on attachment 338907 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:75
> +    if (arguments->op() == NewArrayWithSpread || arguments->op() == PhantomNewArrayWithSpread) {

I don't think we want to actually support NewArrayWithSpread here, just PhantomNewArrayWithSpread, reasons below

> Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:88
> +                DFG_ASSERT(graph, child, child->op() == Spread || child->op() == PhantomSpread, child->op());
> +                DFG_ASSERT(graph, child->child1().node(),
> +                    child->child1()->op() == CreateRest
> +                    || child->child1()->op() == NewArrayBuffer
> +                    || child->child1()->op() == PhantomCreateRest
> +                    || child->child1()->op() == PhantomNewArrayBuffer,
> +                    child->child1()->op());

These assertions only holds if it's PhantomNewArrayWithSpread. And if it's PhantomNewArrayWithSpread, then its children must also be Phantoms.

> Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:104
> +        return insertionSet.insertConstant(nodeIndex, origin, jsNumber(numberOfNonSpreadArguments));

It would be really weird if this code were reached...
Comment 8 Robin Morisset 2018-05-01 06:56:45 PDT
Created attachment 339195 [details]
Patch
Comment 9 Robin Morisset 2018-05-01 06:58:39 PDT
(In reply to Saam Barati from comment #7)
> Comment on attachment 338907 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338907&action=review
> 
> > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:75
> > +    if (arguments->op() == NewArrayWithSpread || arguments->op() == PhantomNewArrayWithSpread) {
> 
> I don't think we want to actually support NewArrayWithSpread here, just
> PhantomNewArrayWithSpread, reasons below

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:88
> > +                DFG_ASSERT(graph, child, child->op() == Spread || child->op() == PhantomSpread, child->op());
> > +                DFG_ASSERT(graph, child->child1().node(),
> > +                    child->child1()->op() == CreateRest
> > +                    || child->child1()->op() == NewArrayBuffer
> > +                    || child->child1()->op() == PhantomCreateRest
> > +                    || child->child1()->op() == PhantomNewArrayBuffer,
> > +                    child->child1()->op());
> 
> These assertions only holds if it's PhantomNewArrayWithSpread. And if it's
> PhantomNewArrayWithSpread, then its children must also be Phantoms.

Fixed.

> 
> > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:104
> > +        return insertionSet.insertConstant(nodeIndex, origin, jsNumber(numberOfNonSpreadArguments));
> 
> It would be really weird if this code were reached...

I agree it does not seem likely with the code as it is currently, but I like it in case the Spread nodes were eliminated (for example through some kind of constant folding) happening earlier.
Comment 10 EWS Watchlist 2018-05-01 08:41:00 PDT
Comment on attachment 339195 [details]
Patch

Attachment 339195 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7524394

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 11 EWS Watchlist 2018-05-01 08:41:12 PDT
Created attachment 339198 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 12 Robin Morisset 2018-05-02 03:54:46 PDT
Comment on attachment 339195 [details]
Patch

The windows test failure seem unrelated.
Comment 13 WebKit Commit Bot 2018-05-02 04:21:34 PDT
Comment on attachment 339195 [details]
Patch

Clearing flags on attachment: 339195

Committed r231229: <https://trac.webkit.org/changeset/231229>
Comment 14 WebKit Commit Bot 2018-05-02 04:21:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2018-05-02 04:23:26 PDT
<rdar://problem/39899574>
Comment 16 dwfault 2018-05-22 00:37:33 PDT
*** Bug 183770 has been marked as a duplicate of this bug. ***