1. Load the testcase 2. Quit Firefox Result: Crash (bp-1a98674c-72b7-459b-915d-fd6b52120626)

Kan-Ru, are you going to look at this?

On Windows: bp-84a87242-1517-4ab1-810c-e81e12120626.

Looks like the lock table is cleared before the wakelock is GCed 51 ClearOnShutdown(&sLockTable);

> Looks like the lock table is cleared before the wakelock is GCed Sigh. And I can't really make ClearOnShutdown happen after that GC/CC; we need to release the refs before the GC/CC so that the GC/CC deletes those objects. Can you just register a shutdown observer and empty out the hashtable? That should fix this. (But I'm starting to believe that ClearOnShutdown is not the right API, in general.)

Comment on attachment 638737 [details] [diff] [review] Clear sLockTable on shutdown >+class ClearHashtableOnShutdown : public nsIObserver { >+public: >+ NS_DECL_ISUPPORTS >+ NS_DECL_NSIOBSERVER >+}; Nit: Please put the class in an anonymous namespace. >+NS_IMPL_ISUPPORTS1(ClearHashtableOnShutdown, nsIObserver) >+ >+NS_IMETHODIMP >+ClearHashtableOnShutdown::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* data) >+{ >+ if (strcmp(aTopic, "xpcom-shutdown")) { >+ return NS_OK; >+ } >+ >+ nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); >+ obs->RemoveObserver(this, "xpcom-shutdown"); I don't think you need to do this. >+ sLockTable = nsnull; I wonder if we can trigger a call to ModifyWakeLock or GetWakeLockInfo during shutdown. For example, nsNPAPIPluginInstance::~nsNPAPIPluginInstance() calls ModifyWakeLock -- can we guarantee that all plugins are destructed before xpcom-shutdown starts? (We didn't have this problem with ClearOnShutdown(), in theory anyway, because that runs after xpcom-shutdown completes.) It seems safer to me to set sLockTable to use !!sLockTable how you use currently use sInitialized. That way, if someone happens to touch the lock table too late, we won't crash and burn. > static void > Init() > { > sLockTable = new nsDataHashtable<nsStringHashKey, LockCount>(); > sLockTable->Init(); >- ClearOnShutdown(&sLockTable); >+ >+ nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); >+ obs->AddObserver(new ClearHashtableOnShutdown(), "xpcom-shutdown", false); If we change things so that you can call Init() during or after shutdown, you need to check that the observer service is not null. I'd like to have another look, although I'd be willing to re-review as-is if you can convince me that the code is safe as written!

(In reply to Justin Lebar [:jlebar] from comment #7) > >+ sLockTable = nsnull; > > I wonder if we can trigger a call to ModifyWakeLock or GetWakeLockInfo > during shutdown. For example, > nsNPAPIPluginInstance::~nsNPAPIPluginInstance() calls ModifyWakeLock -- can > we guarantee that all plugins are destructed before xpcom-shutdown starts? > (We didn't have this problem with ClearOnShutdown(), in theory anyway, > because that runs after xpcom-shutdown completes.) If ClearOnShutdown runs after xpcom-shutdown completes then we are clearing sLockTable earlier than that! It would have the same problem as the old code. > It seems safer to me to set sLockTable to use !!sLockTable how you use > currently use sInitialized. That way, if someone happens to touch the lock > table too late, we won't crash and burn. Let's guard ModifyWakeLock with sIsShutingdown and do nothing in that case. Does it make sense?

> If ClearOnShutdown runs after xpcom-shutdown completes then we are clearing sLockTable earlier > than that! I'm not sure what you mean; either we completely understand each other, or we completely misunderstand. :)

Comment on attachment 639034 [details] [diff] [review] Clear sLockTable on shutdown >+NS_IMETHODIMP >+ClearHashtableOnShutdown::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* data) >+{ >+ if (!strcmp(aTopic, "xpcom-shutdown")) { Nit: Please MOZ_ASSERT this instead of using an if statement. >+ if (sIsShutingdown) { Nit: sIsShuttingDown.

Address the nits.