Closed Bug 285519 Opened 19 years ago Closed 19 years ago

Shutdown assertions warning about potential dead-locks

Categories

(Core :: Networking: Cache, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: darin.moz, Assigned: darin.moz)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 279923
Attached patch v1 patch (obsolete) — Splinter Review
Attachment #176964 - Flags: review?(brendan)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta2
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.
Attached patch v2 patchSplinter Review
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)
Attachment #176964 - Flags: review?(brendan)
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 on attachment 177688 [details] [diff] [review]
v2 patch

sr=me, fwiw.  The logic here is pretty compact.

/be
Attachment #177688 - Flags: superreview+
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+
> (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
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.

Attachment

General

Creator:
Created:
Updated:
Size: