1446693 - Extension process crash in js::gcstats::Statistics::lookupChildPhase with nursery strings

Status: VERIFIED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

[Ekanan Ketunuti]

This bug was filed from the Socorro interface and is
report bp-c316ef90-c50d-4900-8906-be37a0180317.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll js::gcstats::Statistics::lookupChildPhase js/src/gc/Statistics.cpp:176
1 xul.dll js::gcstats::Statistics::beginPhase js/src/gc/Statistics.cpp:1196
2 xul.dll JS::UnmarkGrayGCThingRecursively js/src/gc/Marking.cpp:3625
3 xul.dll JS::Zone::discardJitCode js/src/gc/Zone.cpp:213
4 xul.dll js::Nursery::collect js/src/gc/Nursery.cpp:763
5 xul.dll js::gc::GCRuntime::minorGC js/src/gc/GC.cpp:7743
6 xul.dll js::gc::GCRuntime::gcIfRequested js/src/gc/GC.cpp:7790
7 xul.dll static bool InvokeInterruptCallback js/src/vm/Runtime.cpp:524
8 xul.dll js::jit::InterruptCheck js/src/jit/VMFunctions.cpp:567
9 xul.dll js::OrderedHashMap<js::HashableValue, js::HeapPtr<JS::Value>, js::HashableValue::Hasher, js::ZoneAllocPolicy>::Entry::~Entry 

=============================================================

Running for a while w/ nursery strings enabled and see all of my extensions stopped working.

Comment 1

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

(In reply to Ekanan Ketunuti from comment #0)
> Running for a while w/ nursery strings enabled and see all of my extensions
> stopped working.

Sorry, can you clarify that? Are you saying that enabling nursery strings broke all of your extensions?

Comment 2

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

This stack is familiar. I thought I'd fixed it. I'll look again.

Comment 3

[Ekanan Ketunuti]

(In reply to Steve Fink [:sfink] [:s:] from comment #1)
> (In reply to Ekanan Ketunuti from comment #0)
> > Running for a while w/ nursery strings enabled and see all of my extensions
> > stopped working.
> 
> Sorry, can you clarify that? Are you saying that enabling nursery strings
> broke all of your extensions?

Yes, _all_ of my extensions stop working. Clicking UblockOrigin toolbar crashes extension process.

Comment 4

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

Attachment: Include discardJitCode in AutoTraceSession for minor GC

Whoa, sorry, very bad turnaround on my part on what is a pretty simple problem.

I'm not sure what Jon will think of this fix, though.

The problem is that when we decide to pretenure strings in a zone (ie, disable nursery strings for that zone), we do discardJitCode, which iterates over cells. And the iteration sees that we're not currently in an AutoTraceSession and so decides we need to unmark gray. This patch extends the scope of AutoTraceSession to include the discardJitCode call.

Comment 5

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

Attachment: Use whole cell buffer to post-barrier dependent strings

Currently, JSString::flattenInternal uses the cell edge buffer for postbarriering ropes and dependent strings, which is overkill. And buggy, because we're transmuting the strings during the flattening, and apparently it's possible to end up tracing a string with data that doesn't match its specific string subtype. But this also simplifies the post-barriering substantially.

Note that this does not solve all of the current problems. There appears to still be some issues with the pre-barriering. I haven't tracked that down yet, but again it seems like a pre-barrier can end up marking a string with mismatched flags and data. And I think this may even be happening when nursery strings are disabled?

Comment 6

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

Attachment: Remove invariant that no longer is

With strings in the nursery, it is no longer true that ropes, temporary or otherwise, are always tenured.

Comment 7

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

Attachment: Make nursery and tenured versions of representative test strings

I haven't actually found any bugs as a result, but it seems like if we're going to generate a bunch of representative strings, we ought to have both tenured and nursery versions of them.

Comment 8

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

Comment on attachment 8965912 [details] [diff] [review]
Use whole cell buffer to post-barrier dependent strings

Doh! I just uploaded these 3 patches to the wrong bug.

Comment 9

[Jon Coppeard (:jonco)]

Comment on attachment 8965910 [details] [diff] [review]
Include discardJitCode in AutoTraceSession for minor GC

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

This is not ideal, but it's OK I guess.  We should add an assertion that session.isSome() at the top of the block containing the discard call.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebfd78786947
Include discardJitCode in AutoTraceSession for minor GC, r=jonco

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/ebfd78786947

Comment 12

[Ekanan Ketunuti]

run fine for days, verfify++