Closed Bug 824484 Opened 8 years ago Closed 8 years ago

Intermittent crash in test_privatebrowsing_cancel.js, test_bug430120.js [@ntdll.dll + 0x2fc47], with nsCacheEntryDescriptor::nsOutputStreamWrapper::Close() on the stack

Categories

(Core :: Networking: Cache, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: Gavin, Assigned: michal)

References

Details

(Keywords: crash, intermittent-failure)

Attachments

(2 files, 2 obsolete files)

Component: Private Browsing → Networking: Cache
Product: Firefox → Core
Summary: Intermittent crash in test_privatebrowsing_cancel.js [@ntdll.dll + 0x2fc47], with nsCacheEntryDescriptor::nsOutputStreamWrapper::Close() on the stack → Intermittent crash in test_privatebrowsing_cancel.js, test_bug430120.js [@ntdll.dll + 0x2fc47], with nsCacheEntryDescriptor::nsOutputStreamWrapper::Close() on the stack
Attached patch patch v1 (obsolete) — Splinter Review
Grabbing a reference to the stream under the cache lock in nsCacheService::CloseAllStreams() doesn't ensure that it won't go away since it might be already waiting for the lock in the destructor. This patch fixes the problem.
Assignee: nobody → michal.novotny
Status: NEW → ASSIGNED
Attachment #696883 - Flags: review?(bsmith)
Attached patch patch v2 (obsolete) — Splinter Review
With the change from bug #808997, input and output streams can live longer than cache service. So we must not grab the cache lock when we are not sure that the cache service exists, i.e. when mDescriptor is null.

tryserver push: https://tbpl.mozilla.org/?tree=Try&rev=3664f349ba47
Attachment #696883 - Attachment is obsolete: true
Attachment #696883 - Flags: review?(bsmith)
Attachment #697008 - Flags: review?(bsmith)
Blocks: 808997
Comment on attachment 697008 [details] [diff] [review]
patch v2

Brian seems to be busy, reassigning to Nick.
Attachment #697008 - Flags: review?(bsmith) → review?(hurley)
Comment on attachment 697008 [details] [diff] [review]
patch v2

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

Looks good to me, just one minor nit below.

::: netwerk/cache/nsCacheEntryDescriptor.cpp
@@ +674,5 @@
> +    count = NS_AtomicDecrementRefcnt(mRefCnt);
> +    NS_LOG_RELEASE(this, count, "nsCacheEntryDescriptor::nsInputStreamWrapper");
> +
> +    if (0 == count) {
> +        if (mDescriptor) {

Is there a particular reason to use mDescriptor instead of desc here? Just seems odd to me to use the thing you've already gotten a local reference to. If there's a reason I'm missing, please add a comment explaining why you use mDescriptor instead of desc before landing. (Same goes for other Release functions in other places in this patch.)
Attachment #697008 - Flags: review?(hurley) → review+
(In reply to Nick Hurley [:hurley] from comment #14)
> Is there a particular reason to use mDescriptor instead of desc here? Just
> seems odd to me to use the thing you've already gotten a local reference to.
> If there's a reason I'm missing, please add a comment explaining why you use
> mDescriptor instead of desc before landing. (Same goes for other Release
> functions in other places in this patch.)

mDescriptor can be nulled out on another thread between releasing mLock and acquiring cache lock. We would need to grab the cache lock under the mLock but then we would release these locks in non-LIFO order which causes warning here:

http://hg.mozilla.org/mozilla-central/annotate/6955309291ee/xpcom/glue/BlockingResourceBase.cpp#l167

Even with the current locking, it wouldn't be a big problem to use desc instead of mDescriptor because output streams just null out back-reference. But in case of input streams the assertion would need to be removed since the wrapper might be already removed from the array. Anyway, the current code is more clean and correct.
Attachment #697008 - Attachment is obsolete: true
Attachment #697567 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ac40d537b30a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.