951483 - Add back nsCycleCollector::ShutdownCollect assertion

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

The patch that was landed for bug 945813, in contrast to the one I reviewed, removed the shutdown assertion in ShutdownCollect.  That should get added back in.

Comment 1

[Andy Wingo [:wingo]]

Ack!  I am terribly sorry for this, it was completely unintentional.  :-(

Comment 2

[Andy Wingo [:wingo]]

It seems to be a landing problem.  The patch that was reviewed includes this:

--- a/xpcom/base/nsCycleCollector.cpp	
+++ a/xpcom/base/nsCycleCollector.cpp	
@@ -2687,22 +2687,23 @@ nsCycleCollector::CleanupAfterCollection
     if (mJSRuntime) {
         mJSRuntime->EndCycleCollectionCallback(mResults);
     }
 }
 
 void
 nsCycleCollector::ShutdownCollect()
 {
-    for (uint32_t i = 0; i < DEFAULT_SHUTDOWN_COLLECTIONS; ++i) {
-        NS_ASSERTION(i < NORMAL_SHUTDOWN_COLLECTIONS, "Extra shutdown CC");
+    uint32_t i;
+    for (i = 0; i < DEFAULT_SHUTDOWN_COLLECTIONS; ++i) {
         if (!Collect(ShutdownCC, nullptr)) {
             break;
         }
     }
+    NS_ASSERTION(i < NORMAL_SHUTDOWN_COLLECTIONS, "Extra shutdown CC");
 }
 
 bool
 nsCycleCollector::Collect(ccType aCCType,
                           nsICycleCollectorListener *aManualListener)
 {
     CheckThreadSafety();

I didn't mean to include this change, but it is harmless and has the benefit of knowing in a debug-mode backtrace how many collections it took in the end.  But indeed it seems that the assertion itself didn't make it into https://hg.mozilla.org/integration/mozilla-inbound/rev/815cd189ae2c.

Attached patch adds it back.  I'll kick off a try build.

Comment 3

[Andy Wingo [:wingo]]

Attachment: Add back nsCycleCollector::ShutdownCollect assertion

Comment 4

[Andy Wingo [:wingo]]

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

Comment 5

[Andrew McCreight [:mccr8]]

Thanks!

Comment 6

[Andy Wingo [:wingo]]

Comment on attachment 8349302 [details] [diff] [review]
Add back nsCycleCollector::ShutdownCollect assertion

TBPL had good xpcshell builds, though the debug build timed out with an orange.  Assuming that is unrelated, probably we are good to go here.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab3b1938d32

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ab3b1938d32