1057580 - Assertion failure: resumePointsEmpty(), at jit/MIRGraph.cpp:897

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision cd2acc7ab2f8 (run with --no-threads --fuzzing-safe --ion-eager):


function testApplyCallHelper(f) {
    for (var i = 0; i < 10; ++i)
        f.call(this);
}
function testApplyCall() {
    var r = testApplyCallHelper(
        function (a0,a1,a2,a3,a4,a5,a6,a7) { 
	    x = [a0,a1,a2,a3,a4,a5,a6,a7]; 
        }
    );
    r += testApplyCallHelper(x);
}
testApplyCall();

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9e5f711e25f7
user:        Nicolas B. Pierron
date:        Thu Aug 21 20:47:04 2014 +0200
summary:     Bug 1042729 part 4 - Keep a reference to outer resume point on basic blocks, and make discarding resume point precise. r=jandem

This iteration took 607.376 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Needinfo from nbp based on comment 2 :)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: another assertion stack

I hit this assertion following a link to imgur.com

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: another assertion stack

I crashed on https://tbpl.mozilla.org/

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jan, is it possible for you to take a look at this since :nbp is away? This is hit by fuzzers, and also real world sites as shown by :dbaron's comments above.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(FWIW, I commented the assert out of my local build so I wouldn't crash every half hour running debug builds, so you won't see any more reports from me.)

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] GMT-5 Aug 25-29 from comment #6)
> Jan, is it possible for you to take a look at this since :nbp is away? This
> is hit by fuzzers, and also real world sites as shown by :dbaron's comments
> above.

I looked a bit into this. The resume point it's complaining about is the one created in annotateGetPropertyCache, in the "if (inlinePropTable->numEntries() > 0)" branch. We should consider rewriting the polymorphic inlining code, it's the most complicated part of IonBuilder IMO. I'll think about it but I don't see an easy fix.

npp will be back in a few days, I think it's best to wait for him.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure that no prior resume point survive.

Comment 10

[Nicolas B. Pierron [:nbp]]

From jandem on IRC #ionmonkey:
> why do we need to call the function in processThrow and processReturn? it's not clear to me why or when the calls are inserted

The calls are added in processThrow and processReturn to clear the remaining prior resume points which have not been used in a a call.  So these are just to ensure that we discard the remaining resume points before finishing the MIR Graph of one JSScript.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> The calls are added in processThrow and processReturn to clear the remaining
> prior resume points which have not been used in a a call.  So these are just
> to ensure that we discard the remaining resume points before finishing the
> MIR Graph of one JSScript.

Can we call the discard function in IonBuilder::build? Not all scripts end with a processReturn or processThrow call, for example when there's an infinite loop at the end it also terminates the CFG.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8482877 [details] [diff] [review]
Ensure that no prior resume point survive.

Canceling review for now, see comment 4.

Comment 13

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

Comment 14

[Hannes Verschore [:h4writer]]

We also hit this on octane-typescript in the shell running in a debug (optimized) build

Comment 15

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #12)
> Comment on attachment 8482877 [details] [diff] [review]
> Ensure that no prior resume point survive.
> 
> Canceling review for now, see comment 4.

I do not see any other explanation for comment 4, except that this might be a non-verbose assertion caught by the signal handler.  I suggest we go with the current patch as it fix the issue reported by the fuzzer in comment 0.  And as fuzzers are running debug builds, they will also find the second issue if there is any.

Comment 16

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Ensure that no prior resume point survive.

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8487172 [details] [diff] [review]
IonMonkey: Ensure that no prior resume point survive.

Review of attachment 8487172 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonBuilder.cpp
@@ +1330,5 @@
>      }
>  
> +  break_outer_loop:
> +    // Discard unreferenced & pre-allocated resume points.
> +    replaceMaybeFallbackFunctionGetter(nullptr);

Instead of adding gotos to an already-very-complicated function, just add this call after the traverseBytecode() call in IonBuilder::build() and maybe buildInline. r=me with that.

Comment 18

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Ensure that no prior resume point survive.

(With suggestion applied)

Comment 19

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=12c7abfa088e

Comment 20

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8998fb69e28f

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8998fb69e28f