886102 - Crash [@ js::detail::HashTable] or Assertion failure: outermostScript->hasParallelIonScript(), at ion/ParallelFunctions.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stacks

x = Iterator((function() {}), true);
y = Iterator((function() {}), true);
y = ArrayBuffer();
ParallelArray([99999], function(z) {
    if (z % 1000 == 99) {
        x.n
    }
})

crashes js debug shell on m-c changeset 4c4f75c20e9b without any CLI arguments at js::detail::HashTable, and sometimes asserts at Assertion failure: outermostScript->hasParallelIonScript(), at ion/ParallelFunctions.cpp

s-s because this testcase keeps alternating between a crash, an assert, and exiting normally, locking to be safe.

Comment 1

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

A threadsafe shell is needed to reproduce.

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/63b006573c65
user:        Brian Hackett
date:        Wed May 22 11:36:29 2013 -0600
summary:     Bug 870052 - Various tweaks to reduce recompilation on asm.js style apps, r=jandem.

Brian, is bug 870052 a likely regressor?

Comment 3

[Brian Hackett [Laid off!]]

No, this is a parallel array bug.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: fix

Needed to mark all caches as idempotent in parallel mode.

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 768107 [details] [diff] [review]
fix

Patch is wrong.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: fix v2

So it apparently is perfectly legitimate for a script's ionScript/parallelIonScript to be set to NULL while that script is still on the stack. NULLing is to prevent re-entry into invalidated code, and the IonScript is usually recovered from the frame information.

In light of this, it seems like the correct fix would be to not assert that the outermost script has a parallel IonScript.

Comment 7

[Shu-yu Guo [:shu]]

Gary, I can't reproduce the HashTable crash at all. I left the crash case running for about half an hour and never tripped it.

Comment 8

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

(In reply to Shu-yu Guo [:shu] from comment #7)
> Gary, I can't reproduce the HashTable crash at all. I left the crash case
> running for about half an hour and never tripped it.

It's okay, we'll report the crash should it occur again, after this patch lands. It was intermittently changing between crashing, asserting and exiting normally for me too, as per comment 0.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 768618 [details] [diff] [review]
fix v2

This patch is also wrong -- original patch is probably correct from conversation on IRC.

12:38 < shu> nmatsakis: the reason we monitor for non-idempotent caches is that we can't repeat the operation, so we have to propagate along the constraints, even though there's *already* a type barrier that 
             will fail in the JIT code
12:39 < shu> nmatsakis: so... the original patch is correct: mark all ICs in parallel as idempotent
12:40 < shu> nmatsakis: i thought for some reason that we might need to propage type information even if we won't trip a type barrier
12:40 < shu> nmatsakis: but that's wrong

Comment 10

[Niko Matsakis [:nmatsakis]]

Comment on attachment 768107 [details] [diff] [review]
fix

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

My only comment is that it would be nice to include a comment in the IDEMPOTENT_OP case describing WHY such ops are idempotent. That is, summarizing the IRC conversation that we had.

Comment 11

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

Attachment: stack

a2 = [];;
p0 = new ParallelArray([3857], objectEmulatingUndefined);;
p1 = new ParallelArray(a2);;
p0.__proto__ = p1;
Object.defineProperty(this, "p2", {
    get: function() {
        return p0.map(function(e) {});
    }
});;
p2.toSource = XPCNativeWrapper;

With the fix, this testcase asserts at Assertion failure: idempotent(), at ion/IonCaches.cpp on rev 7469440b076b. It does not assert without the fix.

Comment 12

[Shu-yu Guo [:shu]]

Attachment: fix v2

The old fix wasn't quite sufficient. Idempotency should just be ignored for parallel caches, but to mark caches as idempotent is too restrictive. In parallel execution what happens is that *we can treat non-idempotent caches as idempotent*, since we can simply bail out without doing anything effectful and wait for the restarted ForkJoin computation, even if something effectful (like monitoring types) needed to have been done.

Comment 13

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c576cf51a80

try: https://tbpl.mozilla.org/?tree=Try&rev=dcc924a0d06e

Comment 14

[Andrew McCreight [:mccr8]]

Marking high mostly because this sounds vaguely scary.  I don't entirely understand it.

Comment 15

[Shu-yu Guo [:shu]]

(In reply to Andrew McCreight [:mccr8] from comment #14)
> Marking high mostly because this sounds vaguely scary.  I don't entirely
> understand it.

I never saw the HashTable crash, but the |outermostScript->hasParallelIonScript()| assert isn't even s-s AFAICT, it's just a sanity check that we assert.

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0c576cf51a80

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Since this was checked in without sec-approval, I assume it only affected 24 (trunk at the time) and not 24? I'm trying to determine whether ESR24 was affected by this.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Shu told me that PA is ifdef'd off in everything but current trunk.