Closed
Bug 285519
Opened 19 years ago
Closed 19 years ago
Shutdown assertions warning about potential dead-locks
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
mozilla1.8beta2
People
(Reporter: darin.moz, Assigned: darin.moz)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
1.92 KB,
patch
|
Biesinger
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•19 years ago
|
||
Attachment #176964 -
Flags: review?(brendan)
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta2
Comment 2•19 years ago
|
||
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?
Assignee | ||
Comment 3•19 years ago
|
||
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).
Comment 4•19 years ago
|
||
> (testing a pointer value is atomic)
Is that a valid assumption cross-platform?
Assignee | ||
Comment 5•19 years ago
|
||
> > (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.
Assignee | ||
Comment 6•19 years ago
|
||
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).
Attachment #176964 -
Attachment is obsolete: true
Attachment #177688 -
Flags: review?(benjamin)
Assignee | ||
Updated•19 years ago
|
Attachment #176964 -
Flags: review?(brendan)
Comment 7•19 years ago
|
||
Comment on attachment 177688 [details] [diff] [review] v2 patch I don't know enough to review this.
Attachment #177688 -
Flags: review?(benjamin) → review?(cbiesinger)
Comment 8•19 years ago
|
||
Comment on attachment 177688 [details] [diff] [review] v2 patch sr=me, fwiw. The logic here is pretty compact. /be
Attachment #177688 -
Flags: superreview+
Comment 9•19 years ago
|
||
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...)
Attachment #177688 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Comment 10•19 years ago
|
||
> (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 ;-)
Assignee | ||
Comment 11•19 years ago
|
||
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•