745200 - nsDiskCacheStreamIO::Flush() is sometimes called without holding the cache lock

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

Attachment: fix

While doing review of bug #723577, I found out that nsDiskCacheBinding::~nsDiskCacheBinding() is called outside the cache lock. It then (potentially) calls nsDiskCacheStreamIO::ClearBinding() which calls nsDiskCacheStreamIO::Flush(). Fortunately, it seems that this case is very rare.

Comment 1

[Michal Novotny [:michal]]

Tryserver looks good: https://tbpl.mozilla.org/?tree=Try&rev=1210d8fe6b81

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 614786 [details] [diff] [review]
fix

Review of attachment 614786 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to me, based on Michal's explanation. Hoever, it would be good to get a second opinion from someone who understands the cache locking stuff better than me. In particular, is it possible for an nsDiskCacheBinding to be destroyed without using NS_ProxyRelease(), in code that is already holding the cache service lock?

This should have two comments added to it, explaining what Michal explained to me:

1. In the place where NS_ProxyRelease() is called, indicate that the released thing is a nsDiskCacheBinding and that the destructor of the nsDiskCacheBinding will take the cache service lock.

2. In the destructor of nsDiskCacheBinding, point out where we call NS_ProxyRelease to release the binding.

Comment 3

[u408661]

Comment on attachment 614786 [details] [diff] [review]
fix

Review of attachment 614786 [details] [diff] [review]:
-----------------------------------------------------------------

This looks just fine to me, I don't see anywhere that an nsDiskCacheBinding will be destroyed while already holding the lock. I agree with Brian that we should comment about the relationship between ReleaseObject_Locked and nsDiskCacheBinding.

r=me with those comments added

Comment 4

[Michal Novotny [:michal]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/df802571ace4

Comment 5

[Michal Novotny [:michal]]

(In reply to Brian Smith (:bsmith) from comment #2)
> 1. In the place where NS_ProxyRelease() is called, indicate that the
> released thing is a nsDiskCacheBinding and that the destructor of the
> nsDiskCacheBinding will take the cache service lock.

I've added the comment only to the destructor of nsDiskCacheBinding since we use nsCacheService::ReleaseObject_Locked() to release also other objects.

Comment 6

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/df802571ace4