Last Comment Bug 745200 - nsDiskCacheStreamIO::Flush() is sometimes called without holding the cache lock
: nsDiskCacheStreamIO::Flush() is sometimes called without holding the cache lock
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: 11 Branch
: All All
: -- critical (vote)
: mozilla15
Assigned To: Michal Novotny (:michal)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-13 08:04 PDT by Michal Novotny (:michal)
Modified: 2012-04-25 07:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (1.00 KB, patch)
2012-04-13 08:04 PDT, Michal Novotny (:michal)
hurley: review+
brian: feedback+
Details | Diff | Review

Description Michal Novotny (:michal) 2012-04-13 08:04:36 PDT
Created attachment 614786 [details] [diff] [review]
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) 2012-04-13 08:05:23 PDT
Tryserver looks good: https://tbpl.mozilla.org/?tree=Try&rev=1210d8fe6b81
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-04-18 17:46:27 PDT
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 Nicholas Hurley [:nwgh][:hurley] 2012-04-19 10:33:23 PDT
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) 2012-04-24 16:08:15 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/df802571ace4
Comment 5 Michal Novotny (:michal) 2012-04-24 16:09:11 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-25 07:26:31 PDT
https://hg.mozilla.org/mozilla-central/rev/df802571ace4

Note You need to log in before you can comment on or make changes to this bug.