Open Bug 1433001 Opened 7 years ago Updated 2 years ago

Zone::reportAllocationOverflow is a no-op

Categories

(Core :: JavaScript: GC, enhancement, P3)

enhancement

Tracking

()

Tracking Status
firefox60 --- affected

People

(Reporter: sfink, Unassigned)

Details

This looks bad, but is there some reason it won't happen? #307808 = float64 js::Nursery::doCollection(uint32, js::gc::TenureCountCache*) #11414 = void js::gc::StoreBuffer::traceCells(js::TenuringTracer*) #11415 = void js::gc::StoreBuffer::MonoTypeBuffer<T>::trace(js::gc::StoreBuffer*, js::TenuringTracer&) [with T = js::gc::StoreBuffer::CellPtrEdge] #307668 = void js::gc::StoreBuffer::CellPtrEdge::trace(js::TenuringTracer*) const #307654 = void js::TenuringTracer::traverse(JSObject**) [with T = JSObject] #307656 = JSObject* js::TenuringTracer::moveToTenured(JSObject*) #307672 = uint64 js::TenuringTracer::moveObjectToTenured(JSObject*, JSObject*, int32) #259591 = uint64 js::UnboxedArrayObject::objectMovedDuringMinorGC(JSTracer*, JSObject*, JSObject*, int32) #35893 = T* js::MallocProvider<Client>::pod_malloc(size_t) [with T = unsigned char; Client = JS::Zone; size_t = long unsigned int] #18296 = void JS::Zone::reportAllocationOverflow() #17966 = void js::ReportAllocationOverflow(JSContext*) ... #34945 = JSString* JS_NewStringCopyZ(JSContext*, int8*) It isn't just UnboxedArrayObject. It can also happen with ArgumentsObject and TypedArrayObject and just plain TenuringTracer::moveSlotsToTenured. Or some very different ways. If nothing else, I think these things will assert with the current phase tree setup, because minorGC pushes onto the phase stack, and we don't expect major GC phases underneath MINOR_GC or EVICT_NURSERY or whatever.
(In reply to Steve Fink [:sfink] [:s:] from comment #0) Well, Zone::reportAllocationOverflow() calls js::ReportAllocationOverflow() passing nullptr as the context, and the latter returns immediately in this case. So that's bad because it won't actually ever report an overflow. I guess we don't have tests for this, but we should. If we had passed a context js::ReportAllocationOverflow() has an AutoSuppressGC before calling JS_ReportErrorNumberASCII() so that would have worked out, but it's still pretty nasty that we're doing this inside a minor GC.
Oh! Thanks. The hazard analysis would have picked up on that AutoSuppressGC, but I didn't. I generated this stack from my callgraph traverser, which actually *does* have the needed information, it's just in a cryptic format: #17966 = void js::ReportAllocationOverflow(JSContext*) (IN LIMITED 1) #29390 = void JS_ReportErrorNumberASCII(JSContext*, (JSErrorFormatString*)(void*,uint32)*, void*, uint32) #153327 = void JS_ReportErrorNumberASCIIVA(JSContext*, (JSErrorFormatString*)(void*,uint32) The (IN LIMITED 1) has a bitset of potential allocations, 1 here meaning GC is suppressed. :( I assume we really don't want to be allocating anything in either the nursery or the tenured heap during a minor GC? I mean, we could probably make it work in the tenured heap as long as you don't then store a nursery pointer into it or something, but it just seems very error-prone. I don't see any paths that don't go through ReportAllocationOverflow, so this bug is invalid. I'll repurpose it.
Summary: Minor GC can trigger major GC while tenuring? → Zone::reportAllocationOverflow is a no-op
Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.