Closed Bug 1446693 Opened 3 years ago Closed 3 years ago

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

Categories

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

x86
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- disabled
firefox61 --- verified

People

(Reporter: ananuti, Assigned: sfink)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 3 obsolete files)

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.
(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?
Flags: needinfo?(ananuti)
This stack is familiar. I thought I'd fixed it. I'll look again.
(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.
Flags: needinfo?(ananuti)
Flags: needinfo?(sphink)
Priority: -- → P1
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.
Assignee: nobody → sphink
Status: NEW → ASSIGNED
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?
With strings in the nursery, it is no longer true that ropes, temporary or otherwise, are always tenured.
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 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.
Attachment #8965912 - Attachment is obsolete: true
Flags: needinfo?(sphink)
Attachment #8965913 - Attachment is obsolete: true
Attachment #8965914 - Attachment is obsolete: true
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.
Attachment #8965910 - Flags: review+
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebfd78786947
Include discardJitCode in AutoTraceSession for minor GC, r=jonco
https://hg.mozilla.org/mozilla-central/rev/ebfd78786947
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
run fine for days, verfify++
Status: RESOLVED → VERIFIED
Depends on: 1455599
You need to log in before you can comment on or make changes to this bug.