1433001 - Zone::reportAllocationOverflow is a no-op

Status: NEW

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

Description

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

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.

Comment 1

[Jon Coppeard (:jonco)]

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

Comment 2

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

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.