Closed Bug 1516409 Opened 11 months ago Closed 11 months ago

Crash [@ js::gcstats::Statistics::lookupChildPhase]

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- unaffected
firefox66 --- fixed

People

(Reporter: gkw, Assigned: jonco)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [jsbugmon:])

Crash Data

Attachments

(3 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision 1ff40219367b (build with no significant extra parameters, i.e. opt 64-bit non-deterministic build, run with --fuzzing-safe --no-threads --baseline-eager):

See attachment.

Backtrace:

#0  js::gcstats::Statistics::lookupChildPhase (this=<optimized out>, phaseKind=<optimized out>) at js/src/gc/Statistics.cpp:183
#1  js::gcstats::Statistics::beginPhase (this=0x7f73fc31b530, phaseKind=<optimized out>) at js/src/gc/Statistics.cpp:1266
#2  0x00005574d88e15eb in js::gcstats::AutoPhase::AutoPhase (stats=..., phaseKind=js::gcstats::PhaseKind::UNMARK_GRAY, this=<optimized out>) at js/src/gc/Statistics.h:470
#3  UnmarkGrayGCThing (rt=<optimized out>, thing=...) at js/src/gc/Marking.cpp:3661
#4  ShouldMarkCrossCompartment (marker=<optimized out>, src=<optimized out>, dstCell=<optimized out>) at js/src/gc/Marking.cpp:340
#5  ShouldTraceCrossCompartment (trc=<optimized out>, src=<optimized out>, dstCell=0x13b) at js/src/gc/Marking.cpp:372
/snip

For detailed crash information, see attachment.

A m-c tree should be present at /home/ubuntu/trees/mozilla-central or change the path at the top of the testcase.

Setting s-s as a start. The issue goes away with a --enable-debug build.
Attached file Testcase
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Error: Failed to isolate test from comment
autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/fd169181a978
user:        Jon Coppeard
date:        Tue Dec 18 07:13:59 2018 +0000
summary:     Bug 1514704 - Attribute initial gray marking time to correct stats phase r=sfink

Jon, is bug 1514704 a likely regressor?

(I can't get a better smaller testcase, the symptoms seem to go away on even slight reduction.)
Blocks: 1514704
Flags: needinfo?(jcoppeard)
Attached patch bug1516409-stats-phase (obsolete) — Splinter Review
I couldn't reproduce this but I'm pretty sure I know what's going on.

During marking of gray roots we run out of mark stack space and resort to delaying arenas and marking all their unmarked children.  This doesn't distinguish between black and gray marking and so can trigger black marking at this point.  This in turn can cause gray unmarking where marking crosses a zone boundary into a zone we're not collecting.

The assertion is because the stats phases don't allow this nesting.  The simple fix is just to allow it.

This also changes the assertion to give details of the stats phase kinds.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attachment #9033746 - Flags: review?(sphink)
This is not s-s.
Group: javascript-core-security
Comment on attachment 9033746 [details] [diff] [review]
bug1516409-stats-phase

On second thoughts, I'll do the proper fix which is to add a separate flag for delayed gray marking.
Attachment #9033746 - Attachment is obsolete: true
Attachment #9033746 - Flags: review?(sphink)
This turned out nicely and made the delayed marking much simpler.  The patch adds separate flags for delayed black and gray marking.  Since we can now remove processed arenas from the list during the gray marking stage I removed the stuff about not yielding during this stage to ensure we made progress.
Attachment #9033964 - Flags: review?(sphink)
Comment on attachment 9033964 [details] [diff] [review]
bug1516409-delayed-gray-marking

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

I like the simplification, and the removal of the forced progress stuff. Moar incremental good.

::: js/src/gc/Marking.cpp
@@ +2586,4 @@
>    // Marking delayed children may add more arenas to the list, including arenas
> +  // we are currently processing or have previously processed. Handle this by
> +  // clearing a flag each arenas before marking its children which is set again
> +  // if it is re-added. Iterate the list until no new arenas were added.

s/each arenas/on each arena/, but this sentence is getting a little out of hand. "which" refers to children?...no...the arena?...no...the flag!

Maybe "Handle this by clearing a flag on each arena before marking its children. This flag will be set again if it is re-added."?

@@ +2639,5 @@
> +
> +  return finished;
> +}
> +
> +void GCMarker::filterDelayedMarkingList() {

For me, this would read better in the caller if it were named rebuildDelayedMarkingList. Right now, it sounds like you process the delayed marking list, then you filter it -- but it should be empty if you processed it, so what is there to filter? You have read the definition of filterDelayedMarkingList to understand.
Attachment #9033964 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #9)
> s/each arenas/on each arena/, but this sentence is getting a little out of
> hand. "which" refers to children?...no...the arena?...no...the flag!
> 
> Maybe "Handle this by clearing a flag on each arena before marking its
> children. This flag will be set again if it is re-added."?

Ugh, yeah I'll clean this up, thanks.

> For me, this would read better in the caller if it were named
> rebuildDelayedMarkingList. Right now, it sounds like you process the delayed
> marking list, then you filter it -- but it should be empty if you processed
> it, so what is there to filter? You have read the definition of
> filterDelayedMarkingList to understand.

Yes, fair comment.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d20164aedbf
Separate out flags for gray and black delayed marking r=sfink
https://hg.mozilla.org/mozilla-central/rev/8d20164aedbf
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Duplicate of this bug: 1516759
You need to log in before you can comment on or make changes to this bug.