The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla15

Status

()

Core
Networking: Cache
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: michal, Assigned: michal)

Tracking

11 Branch
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
Attachment #614786 - Flags: review?(bsmith)
(Assignee)

Comment 1

5 years ago
Tryserver looks good: https://tbpl.mozilla.org/?tree=Try&rev=1210d8fe6b81
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.
Attachment #614786 - Flags: review?(hurley)
Attachment #614786 - Flags: review?(bsmith)
Attachment #614786 - Flags: feedback+
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
Attachment #614786 - Flags: review?(hurley) → review+
(Assignee)

Comment 4

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/df802571ace4
(Assignee)

Comment 5

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/df802571ace4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.