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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Testcase

Comment 3

[Fuzzing Team]

JSBugMon: Cannot process bug: Error: Failed to isolate test from comment

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.)

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1516409-stats-phase

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.

Comment 6

[Jon Coppeard (:jonco)]

This is not s-s.

Comment 7

[Jon Coppeard (:jonco)]

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.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug1516409-delayed-gray-marking

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.

Comment 9

[Steve Fink [:sfink] [:s:]]

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.

Comment 10

[Jon Coppeard (:jonco)]

(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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d20164aedbf
Separate out flags for gray and black delayed marking r=sfink

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8d20164aedbf