Closed Bug 1390087 Opened 7 years ago Closed 7 years ago

Assertion failure: zone->isGCScheduled(), at js/src/jsgc.cpp:6785

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: gkw, Assigned: jonco)

References

Details

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

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 3bfcbdf5c6c3 (build with --enable-debug, run with --fuzzing-safe --ion-offthread-compile=off --no-baseline --no-ion):

// jsfunfuzz-generated
setGCCallback({
    action: "majorGC"
});
// Adapted from randomly chosen test: js/src/jit-test/tests/gc/bug-1374797.js
gcparam("allocationThreshold", 1);
offThreadCompileScript("");
for (let i = 0; i < 99999; i++) {
    Symbol.for(i);
}
eval(0);


Backtrace:

#0  js::gc::GCRuntime::budgetIncrementalGC (this=0x7f8ca4b4d670, nonincrementalByAPI=<optimized out>, reason=<optimized out>, budget=..., lock=...) at js/src/jsgc.cpp:6791
#1  0x00000000009f626e in js::gc::GCRuntime::gcCycle (this=this@entry=0x7f8ca4b4d670, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::ALLOC_TRIGGER) at js/src/jsgc.cpp:6901
#2  0x00000000009f6a85 in js::gc::GCRuntime::collect (this=this@entry=0x7f8ca4b4d670, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::ALLOC_TRIGGER) at js/src/jsgc.cpp:7060
#3  0x00000000009f6ef2 in js::gc::GCRuntime::gcSlice (this=0x7f8ca4b4d670, reason=JS::gcreason::ALLOC_TRIGGER, millis=0) at js/src/jsgc.cpp:7146
#4  0x00000000009f70f1 in js::gc::GCRuntime::gcIfRequested (this=0x7f8ca4b4d670) at js/src/jsgc.cpp:7348
/snip

For detailed crash information, see attachment.

Setting s-s as a start because this seems to involve GC.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/e33e38d61829
user:        Jon Coppeard
date:        Tue Jul 25 11:28:41 2017 +0100
summary:     Bug 1374797 - Fix logic around triggering atoms GCs r=sfink

Jon, is bug 1374797 a likely regressor?
Blocks: 1374797
Flags: needinfo?(jcoppeard)
The problem here is that when we're scheduling zones for GC and checking the GC budget we look at all zones, even those used by helper threads that we can't collect anyway.  So let's ignore these.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attachment #8896961 - Flags: review?(sphink)
This is not a security issue.
Group: javascript-core-security
Attachment #8896961 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e5d28a1a277
Ignore zones that we can't collect when scheduling zones for GC r=sfink
https://hg.mozilla.org/mozilla-central/rev/2e5d28a1a277
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Please request Beta approval on this when you get a chance.
Flags: needinfo?(jcoppeard)
Comment on attachment 8896961 [details] [diff] [review]
bug1390087-zone-scheduling

Approval Request Comment
[Feature/Bug causing the regression]: This has been like this since forever basically.
[User impact if declined]: We could occasionally end up doing GCs non-incrementally for no good reason.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a very simple change that ignores zones that we aren't going to collect anyway when budgeting GC.
[String changes made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8896961 - Flags: approval-mozilla-beta?
Comment on attachment 8896961 [details] [diff] [review]
bug1390087-zone-scheduling

Fix an assertion. Looks low risk. Beta56+.
Attachment #8896961 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Jon Coppeard (:jonco) from comment #8)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Jon's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.