Bug 183172 – emitCodeToGetArgumentsArrayLength should not crash on PhantomNewArrayWithSpread

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[André Bargull]

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();
---

[Robin Morisset]

I was able to reproduce this. I am looking into it.

[Robin Morisset]

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.

[Robin Morisset]

Created attachment 338903 [details]
Patch

[Saam Barati]

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.

[Robin Morisset]

Created attachment 338907 [details]
Patch

[Robin Morisset]

(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.

[Saam Barati]

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...

[Robin Morisset]

Created attachment 339195 [details]
Patch

[Robin Morisset]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Robin Morisset]

Comment on attachment 339195 [details]
Patch

The windows test failure seem unrelated.

[WebKit Commit Bot]

Comment on attachment 339195 [details]
Patch

Clearing flags on attachment: 339195

Committed r231229: <https://trac.webkit.org/changeset/231229>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39899574>

*** Bug 183770 has been marked as a duplicate of this bug. ***