1579303 - [hazards] functions appear to GC due to impossible refcount release

Status: NEW

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

Description

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

This comes from the following hazard:

Function '_ZNK7mozilla23CycleCollectedJSRuntime15DescribeGCThingEbN2JS9GCCellPtrER34nsCycleCollectionTraversalCallback$void mozilla::CycleCollectedJSRuntime::DescribeGCThing(uint8, JS::GCCellPtr, nsCycleCollectionTraversalCallback*) const' has unrooted 'obj' of type 'JSObject*' live across GC call 'mozilla::CycleCollectedJSRuntime.DescribeCustomObjects:0' at /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSRuntime.cpp:591
    CycleCollectedJSRuntime.cpp:586: Call(7,8, obj := aThing.as())
    CycleCollectedJSRuntime.cpp:587: Call(8,9, __temp_3 := GetObjectCompartment(obj*))
    CycleCollectedJSRuntime.cpp:587: Assign(9,10, compartmentAddress := __temp_3*)
    CycleCollectedJSRuntime.cpp:588: Call(10,11, clasp := GetObjectClass(obj*))
    CycleCollectedJSRuntime.cpp:591: Call(11,12, __temp_4 := this*.DescribeCustomObjects*(obj*,clasp*,name)) [[GC call]]
    CycleCollectedJSRuntime.cpp:591: Assume(12,13, __temp_4*, false)
    CycleCollectedJSRuntime.cpp:593: Call(13,14, __temp_5 := IsFunctionObject(obj*))
GC Function: mozilla::CycleCollectedJSRuntime.DescribeCustomObjects:0
    XPCJSRuntime.DescribeCustomObjects:0
    uint8 XPCJSRuntime::DescribeCustomObjects(JSObject*, js::Class*, int8[72]*)[72]) const
    nsCOMPtr<T>::~nsCOMPtr() [with T = nsIXPCScriptable]
    nsCOMPtr<T>::~nsCOMPtr() [with T = nsIXPCScriptable] [[base_dtor]]
    _ZN8nsCOMPtrI16nsIXPCScriptableED4Ev
    nsIXPCScriptable.Release:0
    BackstagePass.Release:0
    uint32 BackstagePass::Release()
    BackstagePass.__deleting_dtor :0
    void BackstagePass::~BackstagePass() [[deleting_dtor]]
    void BackstagePass::~BackstagePass()
    void BackstagePass::~BackstagePass() [[base_dtor]]
    void BackstagePass::~BackstagePass(int32)
    void nsIGlobalObject::~nsIGlobalObject() [[base_dtor]]
    void nsIGlobalObject::~nsIGlobalObject(int32)
    void nsIGlobalObject::DisconnectEventTargetObjects()
    void nsIGlobalObject::ForEachEventTargetObject(std::function<void(mozilla::DOMEventTargetHelper*, bool*)>*)>&) const
    _Res std::function<_Res(_ArgTypes ...)>::operator()(_ArgTypes ...) const [with _Res = void; _ArgTypes = {mozilla::DOMEventTargetHelper*, bool*}]
    const std::function<void(mozilla::DOMEventTargetHelper*, bool*)>._M_invoker
    arbitrary function pointer const std::function<void(mozilla::DOMEventTargetHelper*, bool*)>._M_invoker

One problem here is that ForEachEventTargetObject takes a functor and calls it repeatedly, and the analysis treats that as an arbitrary function pointer. But that's not actually the problem in this case; the function in question is a lambda that can GC.

The problem is that this nsCOMPtr<nsIXPCScriptable*> object is never going to drop its refcount to zero:

  XPCWrappedNativeProto* p =
      static_cast<XPCWrappedNativeProto*>(xpc_GetJSPrivate(obj));
  nsCOMPtr<nsIXPCScriptable> scr = p->GetScriptable();
  if (!scr) {
    return false;
  }

  SprintfLiteral(name, "JS Object (%s - %s)", clasp->name,
                 scr->GetJSClass()->name);
  return true;

scr gets its value from p->GetScriptable, which presumably has at least one reference to what it's returning. Then it runs a bunch of innocuous code that can't possibly release any of those references. Finally, it decrements its refcount in the destructor.

I believe it is guaranteed to not call Release here. And the analysis could even figure this out, by looking at all calls within the nsCOMPtr's scope (using existing RAII mechanisms.) I have quite a few hazard false positives that stem from this same conservative "...but what if it's dead??!" stance.

Comment 1

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

I have an implementation of a very hacky approach to this. It just looks at all calls within the scope of the nsCOMPtr in question, using mostly existing RAII machinery, and checks them against a whitelist. If all calls are deemed safe, it pretends like the destructor was never called at all. (It would be better to have the callgraph contain a variant of the destructor that never calls Release() instead, but that's a couple levels deep and this is much simpler.)

In practice, it's really just a mechanism to annotate specific nsCOMPtr callsites by whitelisting stuff. A much simpler alternative would be GC-whitelisting the entire function (XPCJSRuntime::DescribeCustomObjects in this case, plus several others in my current set of hazards.) That bothers me, though -- it just seems very plausible that someone might add a call to something that does GC.

Comment 2

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

A more principled implementation would be to prune out these edges based on global callgraph information: any call that cannot reach ~nsCOMPtr<T> could be considered safe (and function pointers would be assumed to be able to reach it.) That's a little awkward to implement, though, since it's something of a recursive definition and right now the calls are being generated locally to each function (I process a function and dump out all of the calls it makes; the callgraph is not needed, which is good since it hasn't been built yet.) So I would have to generate the pessimistic callgraph, recording potentially removable calls, then re-process those, and possibly iterate to a fixed point -- and all this for a very small number of cases that need it. (For the GC analysis, at least; there might be more for more DOM-centric analyses.)

Comment 3

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

In practice, I am using bug 1746090 when these situations get particularly annoying.