937966 - The nsCycleCollector_isScanSafe assertion could be better

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

nsCycleCollector::Suspect has this assertion "suspected a non-scansafe pointer" which is not very helpful.  Really it seems to mean "is null or something that doesn't QI to nsXPCOMCycleCollectionParticipant", so it should say that.  We could also add a comment saying that it might mean that your QI is missing the CC participant stuff.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Clean up scan safe assertion in Suspect. r=smaug

Comment 2

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=8c89b622c618

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8338615 [details] [diff] [review]
Clean up scan safe assertion in Suspect. r=smaug

It is clearly way too early in the morning for me to be using git bz attach.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8338615 [details] [diff] [review]
Clean up scan safe assertion in Suspect. r=smaug

># HG changeset patch
># User Andrew McCreight <[e-mail]>
>
>Bug 937966 - Clean up scan safe assertion in Suspect. r=smaug
>
>diff --git a/xpcom/base/nsCycleCollector.cpp b/xpcom/base/nsCycleCollector.cpp
>index 4218376..f25c564 100644
>--- a/xpcom/base/nsCycleCollector.cpp
>+++ b/xpcom/base/nsCycleCollector.cpp
>@@ -2585,48 +2585,48 @@ nsCycleCollector::ForgetJSRuntime()
>     if (!mJSRuntime)
>         Fault("forgetting non-registered cycle collector JS runtime");
> 
>     mJSRuntime = nullptr;
> }
> 
> #ifdef DEBUG
> static bool
>-nsCycleCollector_isScanSafe(void *s, nsCycleCollectionParticipant *cp)
>+HasParticipant(void *aPtr, nsCycleCollectionParticipant *aParti)
> {
>-    if (!s)
>-        return false;
>-
>-    if (cp)
>+    if (aParti) {
>         return true;
>+    }
> 
>     nsXPCOMCycleCollectionParticipant *xcp;
>-    ToParticipant(static_cast<nsISupports*>(s), &xcp);
>-
>+    ToParticipant(static_cast<nsISupports*>(aPtr), &xcp);
>     return xcp != nullptr;
> }
> #endif
> 
> MOZ_ALWAYS_INLINE void
>-nsCycleCollector::Suspect(void *n, nsCycleCollectionParticipant *cp,
>+nsCycleCollector::Suspect(void *aPtr, nsCycleCollectionParticipant *aParti,
>                           nsCycleCollectingAutoRefCnt *aRefCnt)
> {
>     CheckThreadSafety();
> 
>     // Re-entering ::Suspect during collection used to be a fault, but
>     // we are canonicalizing nsISupports pointers using QI, so we will
>     // see some spurious refcount traffic here.
> 
>-    if (MOZ_UNLIKELY(mScanInProgress))
>+    if (MOZ_UNLIKELY(mScanInProgress)) {
>         return;
>+    }
> 
>-    MOZ_ASSERT(nsCycleCollector_isScanSafe(n, cp),
>-               "suspected a non-scansafe pointer");
>+    MOZ_ASSERT(aPtr, "Don't suspect null pointers");
> 
>-    mPurpleBuf.Put(n, cp, aRefCnt);
>+    MOZ_ASSERT(HasParticipant(aPtr, aParti),
>+               "Suspected nsISupports pointer must QI to nsXPCOMCycleCollectionParticipant");
>+
>+    mPurpleBuf.Put(aPtr, aParti, aRefCnt);
> }
> 
> void
> nsCycleCollector::CheckThreadSafety()
> {
> #ifdef DEBUG
>     nsIThread* currentThread = NS_GetCurrentThread();
>     // XXXkhuey we can be called so late in shutdown that NS_GetCurrentThread

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41e6b65a12a6

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41e6b65a12a6