Closed Bug 1384550 Opened 7 years ago Closed 7 years ago

Intermittent dom/html/test/test_bug1264157.html | application crashed [@ js::gc::GCRuntime::prepareZonesForCollection] after Assertion failure: atomsZone->isGCMarking(), at /home/worker/workspace/build/src/js/src/jsgc.cpp:3962

Categories

(Core :: JavaScript Engine, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: jonco)

References

Details

(Keywords: crash, intermittent-failure)

Crash Data

Attachments

(1 file)

The is the assertion added in bug 1374797:

    /*
     * Check that we do collect the atoms zone if we triggered a GC for that
     * purpose.
     */
    MOZ_ASSERT_IF(reason == JS::gcreason::DELAYED_ATOMS_GC, atomsZone->isGCMarking());
Assignee: nobody → jcoppeard
Blocks: 1374797
I think this is happening because when we trigger a GC it doesn't happen immediately, and when it does happen the current state may have changed and we're now unable to collect the atoms zone.

Since this doesn't happen very often I added a check in gcIfRequested() and made it skip the collection.  In practice I expect the atoms GC will get triggered the next time an atom is allocated.  The patch clears majorGCTriggerReason so that it doesn't prevent another GC of any kind from being triggered.
Attachment #8890468 - Flags: review?(sphink)
Comment on attachment 8890468 [details] [diff] [review]
bug1384550-atoms-assertion

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

I was happy to see that assert go in, because it seemed like very nonlocal state that would be nice to depend upon but the proof of why it should hold was non-obvious. So I think this is a good result -- without it, our telemetry ends up with GCs triggered by atoms that don't collect atoms, not to mention that's probably not very useful behavior.
Attachment #8890468 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f13dfb12254e
Skip atoms collection if it's no longer possible r=sfink
https://hg.mozilla.org/mozilla-central/rev/f13dfb12254e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.