Closed Bug 1395744 Opened 3 years ago Closed 3 years ago

Assertion failure: atomsZone->isGCMarking(), at js/src/jsgc.cpp:4122

Categories

(Core :: JavaScript Engine, defect, P3, critical)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: gkw, Assigned: jonco)

References

Details

(Keywords: assertion, bugmon, testcase, Whiteboard: [jsbugmon:])

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 04b6be50a252 (build with --enable-debug, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// jsfunfuzz-generated
function f(g) {
    for (var j = 0; j < 2072; ++j) {
        try {
            g();
        } catch (e) {
            e.toString();
        }
    }
}
startgc(6135, 'shrinking');
Function("f()")();
Function("f()")();
f(function () {
    x;
});
setGCCallback({
    action: "majorGC",
    phases: "begin"
});
// Adapted from randomly chosen test: js/src/jit-test/tests/parser/bug-1263881-1.js
s = "";
for (let i = 0; i < 30790; i++) {
    s += "import*as s" + i + " from';"
};
parseModule(s);


Backtrace:

#0  0x00000000009c240c in js::gc::GCRuntime::prepareZonesForCollection (this=this@entry=0x7f77d9b66738, reason=reason@entry=JS::gcreason::DELAYED_ATOMS_GC, isFullOut=<optimized out>, lock=...) at js/src/jsgc.cpp:4122
#1  0x00000000009f1fa2 in js::gc::GCRuntime::beginMarkPhase (this=0x7f77d9b66738, reason=JS::gcreason::DELAYED_ATOMS_GC, lock=...) at js/src/jsgc.cpp:4190
#2  0x00000000009f22ea in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7f77d9b66738, budget=..., reason=reason@entry=JS::gcreason::DELAYED_ATOMS_GC, lock=...) at js/src/jsgc.cpp:6801
#3  0x00000000009f3877 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7f77d9b66738, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DELAYED_ATOMS_GC) at js/src/jsgc.cpp:7141
#4  0x00000000009f3fc2 in js::gc::GCRuntime::collect (this=this@entry=0x7f77d9b66738, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DELAYED_ATOMS_GC) at js/src/jsgc.cpp:7290
/snip

For detailed crash information, see attachment.

Setting s-s because GC seems to be involved.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2e4748827cda
user:        Jon Coppeard
date:        Wed Aug 09 18:05:15 2017 +0100
summary:     Bug 1374239 - Store and re-throw module instantiation and evaluation errors r=shu

This bisection result above came from an earlier unreduced form of the same testcase here.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8f1c6864d359
user:        Masatoshi Kimura
date:        Thu Aug 24 22:45:53 2017 +0900
summary:     Bug 1098412 - Remove and update tests that use the legacy Iterator constructor. r=luke

This bisection result came from this reduced testcase.

Jon, is bug 1098412 (or bug 1374239) a likely regressor?
Blocks: 1098412
Flags: needinfo?(jcoppeard)
FWIW, this reduced testcase is a little fragile but still reproducible, so it will be great to fix this as soon as possible. The number of iterations in the for loops as well as the startgc value seem to be related to one another.
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
The problem is that the testcase runs another GC from the begin GC callback and that unschedules all the zones.  I don't think this is a problem in practice.  I'm not sure what the best way to fix this is though.
Related to bug 1395366?
Set to P3 based on https://bugzilla.mozilla.org/show_bug.cgi?id=1395744#c5. Jon please verify that priority is correct.
Priority: -- → P3
Group: javascript-core-security
I thought I'd uploading this patch a while ago... turns out I hadn't.

This saves the set of scheduled zones at the start of a GC in case the begin callback triggers a GC itself (which can happen if the CC needs to fix gray marking information).

This sounds similar to what's happening in bug 1405980... but I can't quite see how it would cause that exact problem.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attachment #8917419 - Flags: review?(sphink)
Comment on attachment 8917419 [details] [diff] [review]
bug1395744-save-scheduled-zones

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

Oh, ugh. Is there any other state that GC depends on that might be mutated during an inner GC? Like gcPreserveCode_ or keepShapeTables_?
Attachment #8917419 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6210553ddc6c
Save scheduled zones at the start of GC in case begin callback changes them r=sfink
(In reply to Steve Fink [:sfink] [:s:] from comment #9)
> Is there any other state that GC depends on that might be mutated
> during an inner GC? Like gcPreserveCode_ or keepShapeTables_?

That's really good question.  I'll look into that.
https://hg.mozilla.org/mozilla-central/rev/6210553ddc6c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Steve Fink [:sfink] [:s:] from comment #9)
> Is there any other state that GC depends on that might be mutated
> during an inner GC? Like gcPreserveCode_ or keepShapeTables_?

I checked these and they are fine: gcPreserveCode_ is set at the start of every GC, and keepShapeTables_ is set/unset by AutoKeepShapeTables and not changed by the GC.  I didn't find anything else that would need to be preserved.
Is there a user impact here that justifies uplift consideration or can this ride the 58 train?
Flags: needinfo?(jcoppeard)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
No, I don't think there is any user impact so let's let this ride the trains.
Flags: needinfo?(jcoppeard)
You need to log in before you can comment on or make changes to this bug.