Bug 1579303 Comment 0 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

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 is question is a lambda that *can* GC.

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

```c++
  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.
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:

```c++
  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.

Back to Bug 1579303 Comment 0