1582326 - Suppressed rooting hazard from AddRef

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

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

I'm adding a JS::AutoSuppressGCAnalysis in ReflectorNode::edges because objects can be held live across this function, and the analysis thinks it can GC:

    UNWRAP_OBJECT(Node, &get(), node);

calls

   dom::UnwrapObject ->
   UnwrapObjectInternal ->
   nsCOMPtr<T>::operator=(T*) [with T = nsINode] ->
   nsCOMPtr<T>::assign_with_AddRef(nsISupports*) [with T = nsINode] ->
   virtual nsISupports::AddRef

and AddRef for various subclasses can GC. For example:

#265586 = uint32 xpcAccessibilityService::AddRef()
#1851275 = nsAccessibilityService* GetOrCreateAccService(uint32)
#1986585 = uint8 nsAccessibilityService::Init()
#421833 = nsIObserverService.NotifyObservers:0
#45526 = nsObserverService.NotifyObservers:0
#45527 = uint32 nsObserverService::NotifyObservers(nsISupports*, int8*, uint16*)
#739067 = void nsObserverList::NotifyObservers(nsISupports*, int8*, uint16*)
#420386 = nsIObserver.Observe:0
#288965 = nsJSEnvironmentObserver.Observe:0
#288966 = uint32 nsJSEnvironmentObserver::Observe(nsISupports*, int8*, uint16*)
#975634 = void nsJSContext::GarbageCollectNow(int32, uint32, uint32, int64)

or

#422742 = nsINode.AddRef:0
#41446 = mozilla::dom::Attr.AddRef:0
#41447 = uint32 mozilla::dom::Attr::AddRef()
#738574 = uint64 nsCycleCollectingAutoRefCnt::incr(nsISupports*) [with void (* suspect)(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*, bool*) = NS_CycleCollectorSuspect3; uintptr_t = long unsigned int]
#709615 = uint64 nsCycleCollectingAutoRefCnt::incr(void*, nsCycleCollectionParticipant*) [with void (* suspect)(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*, bool*) = NS_CycleCollectorSuspect3; uintptr_t = long unsigned int]
#728314 = NS_CycleCollectorSuspect3
#754883 = void nsCycleCollector::Suspect(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*)
#754506 = nsCycleCollector.cpp:nsISupports* CanonicalizeXPCOMParticipant(nsISupports*)
#418671 = nsISupports.QueryInterface:0
#126035 = WrappedJSHolder.QueryInterface:0
#126036 = uint32 WrappedJSHolder::QueryInterface(nsIID*, void**)
#117521 = uint32 nsXPCWrappedJS::QueryInterface(nsIID*, void**)
#955488 = uint32 nsXPCWrappedJS::DelegatedQueryInterface(nsID*, void**)
#955581 = XPCWrappedJSClass.cpp:nsTString<char> {anonymous}::GetFunctionName(JSContext*, JS::Handle<JSObject*>)
#478456 = uint8 JS_Enumerate(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::GCVector<JS::PropertyKey> >)
#499291 = uint8 js::GetPropertyKeys(JSContext*, JS::Handle<JSObject*>, uint32, JS::MutableHandle<JS::StackGCVector<JS::PropertyKey> >)
#610613 = Iteration.cpp:uint8 Snapshot(JSContext*, JS::Handle<JSObject*>, uint32, JS::MutableHandle<JS::StackGCVector<JS::PropertyKey> >)
#472852 = uint8 js::CheckForInterrupt(JSContext*)
#472853 = uint8 JSContext::handleInterrupt()
#621677 = Runtime.cpp:uint8 HandleInterrupt(JSContext*, uint8)
#544518 = uint8 js::gc::GCRuntime::gcIfRequested()

and

#422742 = nsINode.AddRef:0
#37650 = mozilla::dom::Document.AddRef:0
#37651 = uint32 mozilla::dom::Document::AddRef()
#738574 = uint64 nsCycleCollectingAutoRefCnt::incr(nsISupports*) [with void (* suspect)(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*, bool*) = NS_CycleCollectorSuspect3; uintptr_t = long unsigned int]
#709615 = uint64 nsCycleCollectingAutoRefCnt::incr(void*, nsCycleCollectionParticipant*) [with void (* suspect)(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*, bool*) = NS_CycleCollectorSuspect3; uintptr_t = long unsigned int]
#728314 = NS_CycleCollectorSuspect3
#754929 = nsCycleCollector.cpp:void SuspectAfterShutdown(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*, uint8*)
#420765 = nsCycleCollectionParticipant.DeleteCycleCollectable:0
#41507 = nsXBLBinding::cycleCollection.DeleteCycleCollectable:0
#41508 = void nsXBLBinding::cycleCollection::DeleteCycleCollectable(void*)
#722246 = void nsXBLBinding::DeleteCycleCollectable()
#722247 = void nsXBLBinding::~nsXBLBinding()
#1874528 = void nsXBLBinding::~nsXBLBinding() [[base_dtor]]
#1568798 = void nsXBLBinding::UnbindAnonymousContent(mozilla::dom::Document*, nsIContent*, uint8)
#716786 = void nsAutoScriptBlocker::~nsAutoScriptBlocker() [[complete_dtor]]
#716787 = void nsAutoScriptBlocker::~nsAutoScriptBlocker() [[base_dtor]]
#716784 = void nsContentUtils::RemoveScriptBlocker()
#420545 = nsIRunnable.Run:0
#33443 = mozilla::Runnable.Run:0
#343206 = WorkerThreadPrimaryRunnable.Run:0
#343207 = RuntimeService.cpp:uint32 mozilla::dom::workerinternals::{anonymous}::WorkerThreadPrimaryRunnable::Run()
#483238 = void JS_GC(JSContext*, int32)

among various other paths.

Perhaps all of these are impossible? I don't know. If they're impossible, then this annotation is good but I'd like to comment why and maybe teach the analysis why without needing this annotation. If it is possible for AddRef to GC, I guess I'll need to figure out how to prevent it from causing hazards.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So in order:

1. I think the right fix for ReflectorNode::edges is to change node to nsINode* and use UNWRAP_NON_WRAPPER_OBJECT instead of UNWRAP_OBJECT. That will avoid the call to AddRef and the complications. A comment about how &get() is not going to be a CCW, or about how UnwrapDOMObjectToISupports can only return non-null if its argument is an actual DOM object, not a CCW, might be worthwhile to explain why UNWRAP_NON_WRAPPER_OBJECT is OK here. So something like this:

  nsINode* node;
  if (NS_SUCCEEDED(UNWRAP_NON_WRAPPER_OBJECT(Node, &get(), node)) {
    // Do things with `node` here.
  }

2. The code as it exists right now most definitely calls AddRef on the node.

3. We could probably use RefPtr<nsINode> instead of nsCOMPtr<nsINode> (which works with nsISupports under the hood) to make it clear to the compiler that the AddRef is happening on an nsINode, not a random nsISupports* and that therefore things like xpcAccessibilityService::AddRef are not relevant. That said, the fact that xpcAccessibilityService::AddRef can in fact GC (and can in fact run arbitrary code or something!) is rather disturbing.

4. The path from nsINode::AddRef (which would still be what we're doing with a RefPtr<nsINode>) to nsCycleCollector::Suspect is in fact correct. The CanonicalizeXPCOMParticipant call there is debug-only (inside an assertion) but does in fact happen in a debug build, because the AddRef call passes a null nsCycleCollectionParticipant. However the QI to nsCycleCollectionISupports in that code cannot land in WrappedJSHolder::QueryInterface, because the thing we're calling QI on is in fact an nsINode if the caller did not screw up. So the rest of that stack can't happen in this case, and nsINode::QueryInterface overrides will check for nsCycleCollectionISupports up front and return without reaching the parts of Element::QueryInterface that jump out into XBL and can in fact GC or run script or whatnot.

So the current code is, I believe, safe, but see item 1 for probably the best way to make this clear to the analysis.

Comment 2

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)

That said, the fact that xpcAccessibilityService::AddRef can in fact GC (and can in fact run arbitrary code or something!) is rather disturbing.

Should we get a separate bug on file about this?

Comment 3

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

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)

So in order:

Thank you! I mean, I was kind of hoping the answer was "no, AddRef is fine, this is all noise", but these specifics are great.

1. I think the right fix for ReflectorNode::edges is to change node to nsINode* and use UNWRAP_NON_WRAPPER_OBJECT instead of UNWRAP_OBJECT. That will avoid the call to AddRef and the complications. A comment about how &get() is not going to be a CCW, or about how UnwrapDOMObjectToISupports can only return non-null if its argument is an actual DOM object, not a CCW, might be worthwhile to explain why UNWRAP_NON_WRAPPER_OBJECT is OK here. So something like this:

  nsINode* node;
  if (NS_SUCCEEDED(UNWRAP_NON_WRAPPER_OBJECT(Node, &get(), node)) {
    // Do things with `node` here.
  }

Ok, perfect. There aren't very many of these cases, so a fix like this should work.

2. The code as it exists right now most definitely calls AddRef on the node.

3. We could probably use RefPtr<nsINode> instead of nsCOMPtr<nsINode> (which works with nsISupports under the hood) to make it clear to the compiler that the AddRef is happening on an nsINode, not a random nsISupports* and that therefore things like xpcAccessibilityService::AddRef are not relevant. That said, the fact that xpcAccessibilityService::AddRef can in fact GC (and can in fact run arbitrary code or something!) is rather disturbing.

Ok. Until I land my current patch stack, the analysis is probably too dumb to properly take advantage of the difference. But post-bug 1531951, it will do the right thing.

4. The path from nsINode::AddRef (which would still be what we're doing with a RefPtr<nsINode>) to nsCycleCollector::Suspect is in fact correct. The CanonicalizeXPCOMParticipant call there is debug-only (inside an assertion) but does in fact happen in a debug build, because the AddRef call passes a null nsCycleCollectionParticipant. However the QI to nsCycleCollectionISupports in that code cannot land in WrappedJSHolder::QueryInterface, because the thing we're calling QI on is in fact an nsINode if the caller did not screw up. So the rest of that stack can't happen in this case, and nsINode::QueryInterface overrides will check for nsCycleCollectionISupports up front and return without reaching the parts of Element::QueryInterface that jump out into XBL and can in fact GC or run script or whatnot.

Ooh, thank you -- CanonicalizeXPCOMParticipant shows up a lot. I tend not to worry too much about debug-only stuff, even though the hazard analysis intentionally enables DEBUG. I maybe should go one way or the other. But from nsINode::AddRef, I can get 3 other paths to GC with DEBUG, all going through Suspect. They are for mozilla::dom::Attr, mozilla::dom::Document, and nsIContent:

#422742 = nsINode.AddRef:0
#41446 = mozilla::dom::Attr.AddRef:0
#41447 = uint32 mozilla::dom::Attr::AddRef()
#738574 = uint64 nsCycleCollectingAutoRefCnt::incr(nsISupports*) [with void (* suspect)(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*, bool*) = NS_CycleCollectorSuspect3; uintptr_t = long unsigned int]
#709615 = uint64 nsCycleCollectingAutoRefCnt::incr(void*, nsCycleCollectionParticipant*) [with void (* suspect)(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*, bool*) = NS_CycleCollectorSuspect3; uintptr_t = long unsigned int]
#728314 = NS_CycleCollectorSuspect3
#754883 = void nsCycleCollector::Suspect(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*)

and

#422742 = nsINode.AddRef:0
#39144 = nsIContent.AddRef:0
#39145 = uint32 nsIContent::AddRef()
#1569521 = uint64 nsCycleCollectingAutoRefCnt::incr(nsISupports*) [with void (* suspect)(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*, bool*) = NS_CycleCollectorSuspectUsingNursery; uintptr_t = long unsigned int]
#776184 = uint64 nsCycleCollectingAutoRefCnt::incr(void*, nsCycleCollectionParticipant*) [with void (* suspect)(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*, bool*) = NS_CycleCollectorSuspectUsingNursery; uintptr_t = long unsigned int]
#754930 = NS_CycleCollectorSuspectUsingNursery
#728314 = NS_CycleCollectorSuspect3
#754883 = void nsCycleCollector::Suspect(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*)

and

#422742 = nsINode.AddRef:0
#37650 = mozilla::dom::Document.AddRef:0
#37651 = uint32 mozilla::dom::Document::AddRef()
#738574 = uint64 nsCycleCollectingAutoRefCnt::incr(nsISupports*) [with void (* suspect)(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*, bool*) = NS_CycleCollectorSuspect3; uintptr_t = long unsigned int]
#709615 = uint64 nsCycleCollectingAutoRefCnt::incr(void*, nsCycleCollectionParticipant*) [with void (* suspect)(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*, bool*) = NS_CycleCollectorSuspect3; uintptr_t = long unsigned int]
#728314 = NS_CycleCollectorSuspect3
#754883 = void nsCycleCollector::Suspect(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*)

As for QueryInterface, I'm wondering about the following stacks:

#422740 = nsINode.QueryInterface:0
#39142 = nsIContent.QueryInterface:0
#41580 = mozilla::dom::FragmentOrElement.QueryInterface:0
#41764 = mozilla::dom::Element.QueryInterface:0
#41765 = uint32 mozilla::dom::Element::QueryInterface(nsIID*, void**)
#1568426 = uint32 nsBindingManager::GetBindingImplementation(nsIContent*, nsIID*, void**)
#762269 = void mozilla::dom::AutoJSAPI::~AutoJSAPI()
#834000 = _ZN7mozilla3dom9AutoJSAPID2Ev
#955416 = void mozilla::dom::AutoJSAPI::ReportException()
#483266 = uint8 js::ErrorReport::init(JSContext*, JS::Handle<JS::Value>, uint32)
#514943 = JSString* js::ConcatStrings(JSContext*, JS::Handle<JSString*>, JS::Handle<JSString*>) [with js::AllowGC allowGC = (js::AllowGC)1; typename js::MaybeRooted<JSString*, allowGC>::HandleType = JS::Handle<JSString*>]
#619071 = JSRope* JSRope::new_(JSContext*, JS::Handle<JSString*>, JS::Handle<JSString*>, uint64, uint8) [with js::AllowGC allowGC = (js::AllowGC)1; typename js::MaybeRooted<JSString*, allowGC>::HandleType = JS::Handle<JSString*>; size_t = long unsigned int]
#620236 = JSRope* js::AllocateString(JSContext*, uint8) [with StringT = JSRope; js::AllowGC allowGC = (js::AllowGC)1]
#506636 = JSString* js::AllocateStringImpl(JSContext*, uint8) [with StringAllocT = JSString; js::AllowGC allowGC = (js::AllowGC)1]

and

#422740 = nsINode.QueryInterface:0
#41444 = mozilla::dom::Attr.QueryInterface:0
#41445 = uint32 mozilla::dom::Attr::QueryInterface(nsIID*, void**)
#722262 = void nsNodeSupportsWeakRefTearoff::nsNodeSupportsWeakRefTearoff(nsINode*) [[complete_ctor]]
#722260 = void nsNodeSupportsWeakRefTearoff::nsNodeSupportsWeakRefTearoff(nsINode*)
#722261 = nsCOMPtr<T>::nsCOMPtr(T*) [with T = nsINode]
#731346 = _ZN8nsCOMPtrI7nsINodeEC4EPS0_
#729918 = void nsCOMPtr<T>::Assert_NoQueryNeeded() [with T = nsINode]
#417739 = mozilla::dom::EventTarget.Release:0
#422744 = nsINode.Release:0
#41448 = mozilla::dom::Attr.Release:0
#41449 = uint32 mozilla::dom::Attr::Release()
#1538065 = void nsNodeUtils::LastRelease(nsINode*)
#47606 = nsGenericHTMLFormElement.ClearForm:0
#47607 = void nsGenericHTMLFormElement::ClearForm(uint8, uint8)
#1647924 = uint32 mozilla::dom::HTMLFormElement::RemoveElement(nsGenericHTMLFormElement*, uint8)
#761508 = void nsContentUtils::AddScriptRunner(nsIRunnable*)
#1338968 = void nsContentUtils::AddScriptRunner(already_AddRefed<nsIRunnable>)
#420545 = nsIRunnable.Run:0
#33443 = mozilla::Runnable.Run:0
#343206 = WorkerThreadPrimaryRunnable.Run:0
#343207 = RuntimeService.cpp:uint32 mozilla::dom::workerinternals::{anonymous}::WorkerThreadPrimaryRunnable::Run()
#483238 = void JS_GC(JSContext*, int32)

but that one's using an Assert_NoQueryNeeded. If I take that away, it can only get to GC through Suspect. So that stack doesn't really add anything.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Should we get a separate bug on file about this?

Yeah, probably. I filed bug 1582581.

Comment 5

[Boris Zbarsky [:bzbarsky]]

CanonicalizeXPCOMParticipant shows up a lot.

I bet... I would hope that it can't GC in any real cases, but who knows when someone will write a really buggy QI impl. :(

But from nsINode::AddRef, I can get 3 other paths to GC with DEBUG, all going through Suspect.

OK, but how is Ssupect triggering GC? Or is this via CanonicalizeXPCOMParticipant?

As for QueryInterface, I'm wondering about the following stacks:

That first stack that goes through nsBindingManager::GetBindingImplementation can in fact GC. It will get reached if you QI an Element to an interface that it does not implement... I assume we can't very well whitelist callsites based on argument values, even if those are compile-time constants?

The second stack through nsNodeSupportsWeakRefTearoff::nsNodeSupportsWeakRefTearoff is bogus-ish. The nsNodeSupportsWeakRefTearoff ctor does in fact assign to an nsCOMPtr<nsINode> and that does in fact call Assert_NoQueryNeeded, and that does in fact QI and then release. All that is true. But this particular release cannot trigger nsNodeUtils::LastRelease, because we have already taken a ref to the node in question in the nsCOMPtr, so its refcount cannot go to zero. I'm not sure how best to express this to the analysis.

Comment 6

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

Attachment: Bug 1582326 - skip unnecessary unwrapping to resolve a false positive rooting hazard r=bzbarsky

Comment 7

[Andrew McCreight [:mccr8]]

I think I've discussed this with Steve a few times already, but CanonicalizeXPCOMParticipant --> nsXPCWrappedJS::QueryInterface will never end up in the JS engine. CanonicalizeXPCOMParticipant is specifically a QI to nsCycleCollectionISupports, and nsXPCWrappedJS::QueryInterface explicitly handles that case and returns before it gets into any of the weird JS engine stuff that it does. I have no idea how to convey that to the analysis.

From comment 3: AddRef/Release methods can always get into nsCycleCollector::Suspect(). The whole point of Suspect is to help track whether AddRef/Release have been called on a CCed object. Suspect itself should hopefully not GC in practice, even in debug builds, because we usually check nsCycleCollectionISupports before doing anything too scary. My impression is that "does this GC if the second argument = X" is way beyond the kind of analysis that the hazard analysis can do.

It might be possible to remove the nsCycleCollectionISupports hack and add a Canonicalize method to nsISupports (or some new nsICCSupports class), and then hopefully the analysis could figure out that Canonicalize can never GC, while letting QI remain weird, but that would likely be a lot of work.

Comment 8

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

Sorry, I can never keep straight what is what even after I figure out one piece of the puzzle. I should add the annotation with a full comment explaining this that I can refer back to. But that'll require me to actually understand it. Here's where I am now:

// CanonicalizeXPCOMParticipant appears to be able to GC because it calls
// QueryInterface, which has implementations that can GC, but it cannot GC
// because:
//
// 1. It only does a QI of nsCycleCollectionISupports.
// 2. ???The only QIs that will end up in the JS engine are for nsXPCWrappedJS. Or maybe the only things that participate in cycle
// collection are nsXPCWrappedJS instances? Or... this only happens when going through WrappedJSHolder::QueryInterface, maybe that helps?
// 3. nsXPCWrappedJS::QueryInterface special-cases nsCycleCollectionISupports:
// https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/js/xpconnect/src/XPCWrappedJS.cpp#192
// 4. We assume that nothing will do CanonicalizeXPCOMParticipant on an object
// that does a GC in a failed QueryInterface call.

(In reply to Andrew McCreight [:mccr8] from comment #7)

I think I've discussed this with Steve a few times already, but CanonicalizeXPCOMParticipant --> nsXPCWrappedJS::QueryInterface will never end up in the JS engine. CanonicalizeXPCOMParticipant is specifically a QI to nsCycleCollectionISupports, and nsXPCWrappedJS::QueryInterface explicitly handles that case and returns before it gets into any of the weird JS engine stuff that it does. I have no idea how to convey that to the analysis.

I can always just annotate CanonicalizeXPCOMParticipant.

From comment 3: AddRef/Release methods can always get into nsCycleCollector::Suspect(). The whole point of Suspect is to help track whether AddRef/Release have been called on a CCed object. Suspect itself should hopefully not GC in practice, even in debug builds, because we usually check nsCycleCollectionISupports before doing anything too scary. My impression is that "does this GC if the second argument = X" is way beyond the kind of analysis that the hazard analysis can do.

Yeah, it can really only do that if a call is templatized on a parameter. Though I've done similar special-cased things when the value of the argument is easy to trace through the CFG. (I'm not sure I've landed any of those, since I always seem to find a simpler way that makes it unnecessary. But it does seem to work.)

It might be possible to remove the nsCycleCollectionISupports hack and add a Canonicalize method to nsISupports (or some new nsICCSupports class), and then hopefully the analysis could figure out that Canonicalize can never GC, while letting QI remain weird, but that would likely be a lot of work.

Not worth it, unless it's useful for some other reason.

Comment 9

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

Now back to:

Suspect itself should hopefully not GC in practice, even in debug builds, because we usually check nsCycleCollectionISupports before doing anything too scary.

I guess I need to look for those checks. Here are some stacks that I can't immediately dismiss:

#754883 = void nsCycleCollector::Suspect(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*)
#754551 = void nsCycleCollector::CheckThreadSafety()
#720804 = uint8 nsIEventTarget::IsOnCurrentThread()
#420530 = nsIEventTarget.IsOnCurrentThreadInfallible:0
#71199 = mozilla::net::nsStreamTransportService.IsOnCurrentThreadInfallible:0
#71200 = uint8 mozilla::net::nsStreamTransportService::IsOnCurrentThreadInfallible()
#766627 = nsCOMPtr<T>::~nsCOMPtr() [with T = nsIThreadPool]
#767703 = nsCOMPtr<T>::~nsCOMPtr() [with T = nsIThreadPool] [[base_dtor]]
#417679 = nsIEventTarget.Release:0
#351363 = mozilla::dom::WebSocketImpl.Release:0
#351364 = uint32 mozilla::dom::WebSocketImpl::Release()
#1884439 = void mozilla::dom::WebSocketImpl::~WebSocketImpl() [[complete_dtor]]
#1884440 = void mozilla::dom::WebSocketImpl::~WebSocketImpl() [[base_dtor]]
#1884437 = void mozilla::dom::WebSocketImpl::Disconnect()
#1421440 = void mozilla::dom::WorkerMainThreadRunnable::Dispatch(uint32, mozilla::ErrorResult*)
#908128 = uint8 mozilla::dom::AutoSyncLoopHolder::Run()
#908129 = uint8 mozilla::dom::WorkerPrivate::RunCurrentSyncLoop()
#651256 = void JS_MaybeGC(JSContext*)

Ok, that requires a refcount going to zero and is during a safety check and just isn't very compelling.

#754883 = void nsCycleCollector::Suspect(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*)
#754882 = nsCycleCollector.cpp:uint8 HasParticipant(void*, nsCycleCollectionParticipant*)
#754554 = nsCycleCollector.cpp:void ToParticipant(nsISupports*, nsXPCOMCycleCollectionParticipant**)
#691470 = uint32 CallQueryInterface(nsISupports*, nsXPCOMCycleCollectionParticipant**) [with T = nsISupports; DestinationType = nsXPCOMCycleCollectionParticipant]
#418671 = nsISupports.QueryInterface:0
#419450 = mozilla::dom::SVGTests.QueryInterface:0
#233521 = mozilla::dom::SVGGraphicsElement.QueryInterface:0
#233522 = uint32 mozilla::dom::SVGGraphicsElement::QueryInterface(nsIID*, void**)
#41828 = uint32 nsStyledElement::QueryInterface(nsIID*, void**)
#41765 = uint32 mozilla::dom::Element::QueryInterface(nsIID*, void**)
#1568426 = uint32 nsBindingManager::GetBindingImplementation(nsIContent*, nsIID*, void**)
#483530 = uint8 JS_WrapObject(JSContext*, JS::MutableHandle<JSObject*>)
#498439 = uint8 JS::Compartment::wrap(JSContext*, JS::MutableHandle<JSObject*>)
#577636 = uint8 JS::Compartment::getNonWrapperObjectForCurrentCompartment(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JSObject*>)
#2093490 = IndirectCall: preWrap

Getting the binding implementation has a number of ways it can claim to GC. I'll skip the rest of those here.

#754883 = void nsCycleCollector::Suspect(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*)
#754882 = nsCycleCollector.cpp:uint8 HasParticipant(void*, nsCycleCollectionParticipant*)
#754554 = nsCycleCollector.cpp:void ToParticipant(nsISupports*, nsXPCOMCycleCollectionParticipant**)
#691470 = uint32 CallQueryInterface(nsISupports*, nsXPCOMCycleCollectionParticipant**) [with T = nsISupports; DestinationType = nsXPCOMCycleCollectionParticipant]
#418671 = nsISupports.QueryInterface:0
#372956 = mozilla::SVGFilterObserverList.QueryInterface:0
#372957 = uint32 mozilla::SVGFilterObserverList::QueryInterface(nsIID*, void**)
#416657 = nsISupports.AddRef:0
#417479 = nsIAccessibilityService.AddRef:0
#265585 = xpcAccessibilityService.AddRef:0
#265586 = uint32 xpcAccessibilityService::AddRef()
#1851275 = nsAccessibilityService* GetOrCreateAccService(uint32)
#1986585 = uint8 nsAccessibilityService::Init()
#421833 = nsIObserverService.NotifyObservers:0
#45526 = nsObserverService.NotifyObservers:0
#45527 = uint32 nsObserverService::NotifyObservers(nsISupports*, int8*, uint16*)
#739067 = void nsObserverList::NotifyObservers(nsISupports*, int8*, uint16*)
#420386 = nsIObserver.Observe:0
#358524 = nsFlatpakPrintPortal.Observe:0
#358525 = uint32 nsFlatpakPrintPortal::Observe(nsISupports*, int8*, uint16*)
#2097017 = IndirectCall: _ZZN20nsFlatpakPrintPortal7ObserveEP11nsISupportsPKcPKDsE23s_g_unix_fd_list_append$_ZN20nsFlatpakPrintPortal7ObserveEP11nsISupportsPKcPKDs$uint32 nsFlatpakPrintPortal::Observe(nsISupports*

There's the a11y stuff again. Ignoring.

#754883 = void nsCycleCollector::Suspect(void*, nsCycleCollectionParticipant*, nsCycleCollectingAutoRefCnt*)
#754882 = nsCycleCollector.cpp:uint8 HasParticipant(void*, nsCycleCollectionParticipant*)
#754554 = nsCycleCollector.cpp:void ToParticipant(nsISupports*, nsXPCOMCycleCollectionParticipant**)
#691470 = uint32 CallQueryInterface(nsISupports*, nsXPCOMCycleCollectionParticipant**) [with T = nsISupports; DestinationType = nsXPCOMCycleCollectionParticipant]
#418671 = nsISupports.QueryInterface:0
#418842 = nsIInputStreamLengthCallback.QueryInterface:0
#97255 = nsMIMEInputStream.QueryInterface:0
#97256 = uint32 nsMIMEInputStream::QueryInterface(nsIID*, void**)
#847184 = uint8 nsMIMEInputStream::IsSeekableInputStream() const
#742957 = nsCOMPtr<T>::nsCOMPtr(nsQueryInterface<U>) [with U = nsIInputStream; T = nsISeekableStream]
#743524 = _ZN8nsCOMPtrI17nsISeekableStreamEC4I14nsIInputStreamEE16nsQueryInterfaceIT_E
#741896 = void nsCOMPtr<T>::assign_from_qi(nsQueryInterface<U>, const nsIID&) [with U = nsIInputStream; T = nsISeekableStream; nsIID = nsID]
#742552 = void nsCOMPtr<T>::assign_assuming_AddRef(T*) [with T = nsISeekableStream]
#417830 = nsITellableStream.Release:0
#424542 = nsISeekableStream.Release:0
#98117 = mozilla::net::CacheFileOutputStream.Release:0
#98118 = uint32 mozilla::net::CacheFileOutputStream::Release()
#851542 = uint32 mozilla::net::CacheFile::RemoveOutput(mozilla::net::CacheFileOutputStream*, uint32)
#851543 = void mozilla::net::CacheFile::NotifyListenersAboutOutputRemoval()
#851562 = nsBaseHashtable<KeyClass, DataType, UserDataType>::Iterator::~Iterator() [with KeyClass = nsUint32HashKey; DataType = nsAutoPtr<mozilla::net::ChunkListeners>; UserDataType = mozilla::net::ChunkListeners*]
#853736 = nsBaseHashtable<KeyClass, DataType, UserDataType>::Iterator::~Iterator() [with KeyClass = nsUint32HashKey; DataType = nsAutoPtr<mozilla::net::ChunkListeners>; UserDataType = mozilla::net::ChunkListeners*] [[base_dtor]]
#728041 = void PLDHashTable::Iterator::~Iterator()
#738500 = void PLDHashTable::ShrinkIfAppropriate()
#738480 = uint8 PLDHashTable::ChangeTable(int32)
#738481 = uint8 PLDHashTable::ChangeTable(int32)::__lambda12*) [with F = PLDHashTable::ChangeTable(int32_t)::<lambda(const PLDHashTable::Slot&)>; uint32_t = unsigned int]
#738479 = PLDHashTable.cpp:void PLDHashTable::ChangeTable(PLDHashTable::Slot*)::<lambda(const PLDHashTable::Slot&)>

There are others similar to that one that don't require the PLDHashTable stuff, but are equally improbable. If I prevent any ::Release() from being called, then it doesn't find any more. And this is all within the HasParticipant MOZ_ASSERT anyway.

The last different-looking one I found was a rather terrifying one that triggers a GC by calling subsumes() inside of a read barrier. (But again, it's all within MOZ_ASSERT conditions.)

Looking at the implementation of Suspect(), it doesn't do anything other than mPurpleBuf.Put() if you're non-DEBUG, so it doesn't seem very worrisome.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/64c2cb6d207cc22983f80d4f794c1589eedced49
https://hg.mozilla.org/mozilla-central/rev/64c2cb6d207c

Comment 11

[Ryan VanderMeulen (PTO, back 6-April)]

Sounds like this can ride the trains if it's a false positive. Feel free to nominate for uplift if that's incorrect.

Comment 12

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

Sounds like this can ride the trains if it's a false positive. Feel free to nominate for uplift if that's incorrect.

Yes. I was leaving it as a security issue only because it seemed like some of the discussion might be sensitive. I meant to drop a note here that I was landing with sec-review for that reason.

Comment 13

[Boris Zbarsky [:bzbarsky]]

QueryInterface, which has implementations that can GC, but it cannot GC because:
...
// 1. It only does a QI of nsCycleCollectionISupports.

s/of/to/. That is, the interface pointer it asks for is nsCycleCollectionISupports.

The only QIs that will end up in the JS engine are for nsXPCWrappedJS.

Not quite. QIs will end up in the JS engine if the object has a "weird" QI impl that touches the JS engine.

Now in practice, most objects do not have weird QI impls. They just take the incoming interface ID, compare to a static C++ table of IDs they implement, and if they get a match, return a pointer to themselves. None of that GCs.

Some objects do have a weird QI impl. One such is nsXPCWrappedJS, which will in general call into JS to do the "do I implement this interface?" check, because it's the shim for implementing XPCOM things via JS objects. However it will not call into JS for the specific IID of nsCycleCollectionISupports, so this specific weirdness is safe.

Another weird QI impl is Element::QueryInterface, which calls into GetBindingImplementation, which touches JS. However, it first calls FragmentOrElement::QueryInterface, which always returns success for nsCycleCollectionISupports, so in practice, for this specific IID we never reach GetBindingImplementation. We could add a release assert or check to that effect to GetBindingImplementation if that would help.

JSWindowActor::QueryInterfaceActor has a weird QI, but ends up delegating to an nsXPCWrappedJS. However, before doing that it does some JS-engine-related work to maybe create that nsXPCWrappedJS. Needs auditing to see whether that stuff can GC...

There's various accessible and gfx and ipc and widget stuff that has "weird" (as in, not implemented via XPCOM QI macros) QI methods that I did not audit; some or most of them are probably COM QI, not XPCOM QI. I don't know whether those can show up in cycle collector, even.

At first glance, that's it for weird QI impls... but see below.

Getting the binding implementation has a number of ways it can claim to GC. I'll skip the rest of those here.

Right, QI to CC stuff will never reach GetBindingImplementation per above.

#97256 = uint32 nsMIMEInputStream::QueryInterface(nsIID*, void**)

OK, so this is a conditional QI, where we check a condition. We check the condition before we check whether the IID matched (see NS_IMPL_QUERY_BODY_CONDITIONAL). We could reverse that if that would make things easier to reason about because then for this stack we would only care about conditional QI to nsXPCOMCycleCollectionParticipant, which is not likely to exist. But as things stand, if we have a nsMIMEInputStream instance in Suspect, we will in fact do a QI on its underlying stream (to nsISeekableInputStream now, not the CC interface!) via IsSeekableInputStream, and that QI can in theory call out into JS if we have a JS-implemented underlying stream, which I think we can. So that is a real problem, looks like, and we should fix it. Probably by reversing the order of checks in NS_IMPL_QUERY_BODY_CONDITIONAL.