Closed Bug 1285186 Opened 4 years ago Closed 3 years ago

Assertion failure: !waitingOnGC[i]->runtimeMatches(rt), at js/src/vm/HelperThreads.cpp:313

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- unaffected
firefox-esr45 --- unaffected
firefox50 --- fixed

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [post-critsmash-triage][jsbugmon:])

Attachments

(2 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision 95ffbc4ff635 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager --gc-zeal=10):

// jsfunfuzz-generated
function f() {};
// Adapted from randomly chosen testcase: js/src/jit-test/tests/auto-regress/bug1268034.js
offThreadCompileScript("");
// jsfunfuzz-generated
abortgc();


Backtrace:

0   js-dbg-64-dm-clang-darwin-95ffbc4ff635	0x000000010afdd2cb js::CancelOffThreadParses(JSRuntime*) + 1163 (HelperThreads.cpp:313)
1   js-dbg-64-dm-clang-darwin-95ffbc4ff635	0x000000010b063f5b JSRuntime::destroyRuntime() + 363 (GCRuntime.h:1380)
2   js-dbg-64-dm-clang-darwin-95ffbc4ff635	0x000000010ade5560 JSContext::~JSContext() + 32 (jscntxt.cpp:897)
3   js-dbg-64-dm-clang-darwin-95ffbc4ff635	0x000000010ade2a6a js::DestroyContext(JSContext*) + 282 (Utility.h:256)
4   js-dbg-64-dm-clang-darwin-95ffbc4ff635	0x000000010a7c5a30 main + 13392 (js.cpp:7461)
/snip

For detailed crash information, see attachment.

GC assert, setting s-s first as a start.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/eef71b01a821
user:        Jan de Mooij
date:        Mon Jul 04 19:44:26 2016 +0200
summary:     Bug 1283169 - Finish incremental GC before cancelling off-threaad parse tasks. r=jonco

Jan, is bug 1283169 a likely regressor?
Blocks: 1283169
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Attached patch Patch (obsolete) — Splinter Review
Another issue with CancelOffThreadParses. When we abort an incremental gc, we were not emptying the parseWaitingOnGC list.

Good catch, this is also a pre-existing issue.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8769070 - Flags: review?(jcoppeard)
Comment on attachment 8769070 [details] [diff] [review]
Patch

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

Nice, thanks for fixing this.
Attachment #8769070 - Flags: review?(jcoppeard) → review+
Try uncovered some issues, I'll look into that today or tomorrow...
Attached patch Patch v2Splinter Review
Updated patch. This one is green on Try.

The AutoEnqueuePendingParseTasksAfterGC destructor checked !gc_.isIncrementalGCInProgress(), but I think it's better and nicer to call !OffThreadParsingMustWaitForGC, as that one also checks some other things.
Attachment #8769070 - Attachment is obsolete: true
Attachment #8770615 - Flags: review?(jcoppeard)
Attachment #8770615 - Flags: review?(jcoppeard) → review+
This should have had a security rating (and possibly sec-approval) before landing, as per comment 4 it sounds like this is not a Nightly-only regression. How bad of a security issue is this? Thanks.
Flags: needinfo?(jdemooij)
For example, do we need to get this fix on ESR-45? Jump the trains to ship with 48?
(In reply to Andrew McCreight [:mccr8] from comment #9)
> This should have had a security rating (and possibly sec-approval) before
> landing, as per comment 4 it sounds like this is not a Nightly-only
> regression. How bad of a security issue is this? Thanks.

It's not a security issue on anything other than Nightly.

The "pre-existing issue" I mentioned in comment 4 is unrelated and not s-s: we didn't empty the list of "parse tasks waiting on GC" when we aborted an incremental GC, so tasks could stay in that list longer than strictly necessary.
Flags: needinfo?(jdemooij)
Keywords: sec-high
Ah, thanks for the explanation.
Keywords: regression
https://hg.mozilla.org/mozilla-central/rev/0fe5429f71a7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [jsbugmon:] → [post-critsmash-triage][jsbugmon:]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.