Shutdown assertions warning about potential dead-locks. When we hit these in nsCacheEntryDescriptor::Close at shutdown, they are almost always impossible dead-locks. This assertion is triggered whenever imgRequest objects are still hanging around after xpcom-shutdown. They hold an owning reference to nsCacheEntryDescriptor. However, by the time xpcom-shutdown runs, the nsCacheService has already closed down all descriptors, so nsCacheEntryDescriptor::Close becomes a no-op. So in these cases, we just enter the lock, find that mCacheEntry is null, and return. If I recall, the potential dead-lock assertion is about the cache service's lock and the xpcom component manager's lock. During shutdown we are sometimes in a state where the xpcom component manager's lock is held while entering the cache service's lock. That situation isn't a concern in this particular case. So, we can safely avoid these assertions. We can suppress them easily by moving the mCacheEntry null check outside of entering the lock (testing a pointer value is atomic). Of course, we then still have to perform the same check once we enter the lock. In the past, I used to fix these assertions by trying to ensure that all imgRequest objects were closed down during xpcom-shutdown or early enough. But, it seems like a difficult thing to enforce, and so we are bound to fight these assertions over and over again. I'd rather fix it once and for all by making the cache allow this use case. Patch coming up.

I'm torn about this... I really think we should enforce xpcom shutdown requirements (nobody is allowed to hold on to things in other xpcom module beyond xpcom-shutdown). Do we know why imgRequests are being held past shutdown?

yeah, well... that has been my take on this for a while too. however, when you consider that the cache entry descriptor at this point is just a shell of an object, it really doesn't make much difference. the cache service has already cleanly shut everything down. with some debugging, it looks like the owner of the imgRequest is a CSS object. i previously moved all of layout shutdown up ahead a bit to solve a bunch of these assertions, but it was a bit nerve racking to make that change. another similar solution to this problem is to make image lib release any open cache entry descriptors at xpcom-shutdown. but, that just seems like more code for not much gain. no other (few at most) necko objects complain about being held past xpcom shutdown (note: held is not the same as used).

> (testing a pointer value is atomic) Is that a valid assumption cross-platform?

> > (testing a pointer value is atomic) > > Is that a valid assumption cross-platform? We assume it is. If not, then Mozilla would crash-n-burn big time.

OK, how about this patch. It is a slight compromise over the other. Here, we only prevent the shutdown assertions in cases where people are only guilty of holding a reference to a nsCacheEntryDescriptor past xpcom-shutdown. If they try to call the Close method, we're back to square one. This allows us to still detect code that really should not be running past xpcom-shutdown, while allowing people to get away with holding nsCacheEntryDescriptor objects that are already dead (taking up almost no memory). I can imagine that this might help avoid problems in which GC'd languages don't GC nsCacheEntryDescriptor objects until after xpcom-shutdown or possibly on some background thread (e.g., which can and does happen with JNI).

Comment on attachment 177688 [details] [diff] [review] v2 patch I don't know enough to review this.

Comment on attachment 177688 [details] [diff] [review] v2 patch sr=me, fwiw. The logic here is pretty compact. /be

Comment on attachment 177688 [details] [diff] [review] v2 patch I guess this works. I don't quite understand, though, why something holds onto cache entries after shutdown? (JNI could forcefully trigger a GC, couldn't it? System.GC() exists... I guess it wouldn't help static class members...)

> (JNI could forcefully trigger a GC, couldn't it? System.GC() exists... I guess > it wouldn't help static class members...) That sounds like an option. I recall Javier mentioning that he had to proxy Release calls over to the main thread because GC would run on a background thread. In that case, we have race condition between xpcom-shutdown and that event being proxied to the main thread. In some cases, the event being proxied will come after xpcom-shutdown, so we'd end up holding references to objects such as these past xpcom-shutdown. Of course, this is purely hypothetical ;-)

fixed-on-trunk