Assertion failure: !auxNextLink && !hasDelayedMarking, at js/src/gc/Heap.h:397

VERIFIED FIXED in Firefox 66

Status

()

defect
--
critical
VERIFIED FIXED
5 months ago
2 months ago

People

(Reporter: gkw, Assigned: jonco)

Tracking

(Blocks 2 bugs, 6 keywords)

Trunk
mozilla66
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 unaffected, firefox65 unaffected, firefox66 verified)

Details

(Whiteboard: [fuzzblocker][jsbugmon:update])

Attachments

(2 attachments)

Reporter

Description

5 months ago
The following testcase crashes on mozilla-central revision edf1f05e9d00 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/jit-test/tests/gc/bug-1464872.js
var x = newGlobal();
x.evaluate("grayRoot()");
x = 0;
// jsfunfuzz-generated
gcparam("markStackLimit", 4);
gc();

Backtrace:

#0  js::gc::Arena::setNextDelayedMarking (this=<optimized out>, arena=<optimized out>) at js/src/gc/Heap.h:397
#1  0x000056015d10dffb in js::GCMarker::processDelayedMarkingList (this=0x7f4cd1c1d648, outputList=0x7ffcd31f81b0, color=js::gc::MarkColor::Gray, shouldYield=false, budget=...) at js/src/gc/Marking.cpp:2591
#2  0x000056015d10be45 in js::GCMarker::markAllDelayedChildren (this=0x7f4cd1c1d648, budget=...) at js/src/gc/Marking.cpp:2626
#3  0x000056015d0f46ae in js::GCMarker::markUntilBudgetExhausted (this=0x7f4cd1c1d648, budget=...) at js/src/gc/Marking.cpp:1636
#4  0x000056015d0f4195 in js::gc::GCRuntime::markGrayReferencesInCurrentGroup (this=0x7f4cd1c1c680, fop=<optimized out>, budget=...) at js/src/gc/GC.cpp:5285
#5  0x000056015d1624fb in sweepaction::SweepActionSequence<js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&>::run (this=0x7f4cd1c173d0, args=..., args=..., args=...) at js/src/gc/GC.cpp:6293
#6  0x000056015d163666 in sweepaction::SweepActionRepeatFor<js::gc::SweepGroupsIter, JSRuntime*, js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&>::run (this=0x7f4cd1c2d040, args=..., args=..., args=...) at js/src/gc/GC.cpp:6353
/snip

For detailed crash information, see attachment.

Setting s-s because gc is involved.
Reporter

Comment 2

5 months ago
autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/3e7a4e085ead
user:        Jon Coppeard
date:        Thu Dec 06 16:27:22 2018 -0500
summary:     Bug 1463462 - Parition the mark stack into black and gray entries r=sfink

Jon, is bug 1463462 a likely regressor?
Blocks: 1463462
Flags: needinfo?(jcoppeard)
Reporter

Comment 3

5 months ago
Or might this also be related to bug 1514421, bug 1514520 or bug 1514704?
Reporter

Comment 4

5 months ago
Should this be set as dupe, please land the testcase in this bug, and also this one:

offThreadCompileScript("");
x = new WeakMap;
x.set(Array, "");
gc();

This is the fuzzing testcase for bug 1513465 (a regression from bug 1509923).

(still going through backlog)
Reporter

Comment 5

5 months ago
Elevating to [fuzzblocker]. This seems to be crashing all over the place.
Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]
Assignee

Updated

5 months ago
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Assignee

Comment 6

5 months ago
Attachment #9032449 - Flags: review?(sphink)
Assignee

Comment 7

5 months ago
Oops, posted without explanation.

The problem is the way we handled delayed marking of arenas (which happens when we run out of mark stack) doesn't account for the fact that we can delay marking of an arena that we've already processed if the current delayed arena has a cell with pointers into the that arena.  (I did mean it to take account of this but it doesn't work because GCMarker::delayMarkingArena sees the hasDelayedMarking flag is already set in this case, because we're using auxNextLink to link the arenas into outputList in GCMarker::processDelayedMarkingList).

This patch rewrites the way this works and tidies up a bunch of stuff.  I added separate flags for onDelayedMarkingList and hasDelayedMarking and make delayMarkingArena just re-set the hasDelayedMarking flag if we need to delay marking for an arena that's already been processed.  The main fix is that we have to run the loop repeatedly in processDelayedMarkingList if there is more work to do (but we only have to mark the new arenas which have the flag set).

You commented that we could have separate flags for black and gray marking, and that's still true and would be an improvement.  I'm not sure whether its worth it though.
Comment on attachment 9032449 [details] [diff] [review]
bug1514927-delayed-marking

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

Makes sense.
Attachment #9032449 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/d61c96936904
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Updated

5 months ago
Status: RESOLVED → VERIFIED

Comment 11

5 months ago
JSBugMon: This bug has been automatically verified fixed.
Reporter

Updated

5 months ago
Depends on: 1515824
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.