Closed Bug 886102 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 --- unaffected
firefox25 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected

People

(Reporter: gkw, Assigned: shu)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [adv-main25+])

Crash Data

Attachments

(3 files, 2 obsolete files)

Attached file 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.
A threadsafe shell is needed to reproduce.
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?
Flags: needinfo?(bhackett1024)
No, this is a parallel array bug.
Flags: needinfo?(bhackett1024)
Flags: needinfo?(nmatsakis)
Attached patch fix (obsolete) — Splinter Review
Needed to mark all caches as idempotent in parallel mode.
Assignee: general → shu
Attachment #768107 - Flags: review?(nmatsakis)
Flags: needinfo?(nmatsakis)
Comment on attachment 768107 [details] [diff] [review]
fix

Patch is wrong.
Attachment #768107 - Attachment is obsolete: true
Attachment #768107 - Flags: review?(nmatsakis)
Attached patch fix v2 (obsolete) — Splinter Review
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.
Attachment #768618 - Flags: review?(nmatsakis)
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.
(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 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
Attachment #768618 - Attachment is obsolete: true
Attachment #768618 - Flags: review?(nmatsakis)
Attachment #768107 - Attachment is obsolete: false
Attachment #768107 - Flags: review?(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.
Attachment #768107 - Flags: review?(nmatsakis) → review+
Attached file 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.
Flags: needinfo?(shu)
Attached patch fix v2Splinter Review
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.
Attachment #768107 - Attachment is obsolete: true
Attachment #770752 - Flags: review?(nmatsakis)
Flags: needinfo?(shu)
Attachment #770752 - Flags: review?(nmatsakis) → review+
Marking high mostly because this sounds vaguely scary.  I don't entirely understand it.
Keywords: sec-high
(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.
https://hg.mozilla.org/mozilla-central/rev/0c576cf51a80
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 893679
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.
Shu told me that PA is ifdef'd off in everything but current trunk.
Whiteboard: [adv-main25+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.