Crash [@PR_Close].[@nsDiskCacheStreamIO::Flush()]

RESOLVED FIXED in mozilla1.8.1

Status

()

Core
Networking: Cache
P1
critical
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: Mark Mentovai, Assigned: Mark Mentovai)

Tracking

(4 keywords)

1.8 Branch
mozilla1.8.1
crash, fixed1.8.0.5, fixed1.8.1, topcrash
Points:
---
Bug Flags:
blocking1.8.0.4 -
blocking1.8.0.5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [camino-1.0.1][camino-1.0.2], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

12 years ago
I just experienced a crash in PR_Close, with nsCacheEntryDescriptor::nsOutputStreamWrapper::~nsOutputStreamWrapper and nsHttpChannel::OnStopRequest on the stack.  A full stack trace will be attached.  The crash occurred as a page was finishing loading.

Talkback data show that this is a cross-product and cross-platform crash, and all crashes with PR_Close at the top of the stack show up on the topcrash list.

Some recent TBIDs: TB13206859 TB13199365 TB13191944.  These are from Linux and Mac.  Crashes on Windows don't have usable stacks, TB13202136 TB13201471.
(Assignee)

Comment 1

12 years ago
Created attachment 206729 [details]
Stack trace

Comment 2

11 years ago
Seems to be the same problem : http://forums.mozillazine.org/viewtopic.php?t=379065
*** Bug 326929 has been marked as a duplicate of this bug. ***
Created attachment 219388 [details]
analysis of TB17282316

This analysis shows that the crash is because the fd passed to PR_Close is null.
Flags: blocking1.8.0.3?
(This is one of the top crashes on Linux and Mac; doesn't appear to be showing up much on Windows.)

Updated

11 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1
Looks like this is missing 1.8.0.3, try 1.8.0.4
Flags: blocking1.8.0.4?
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3-
(Assignee)

Comment 7

11 years ago
mFD is also 0 in the OS X ppc report (attachment 206729 [details]), see $r3.  This is occurring on win32 too, but it's not showing up as high in the results.

Here's what it looks like is happening:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/cache/src/nsDiskCacheStreams.cpp&rev=1.17.4.1&mark=484,489,675,680,686#469

489 is the PR_Close that crashes.  The FlushBufferToFile call immediately before PR_Close is supposed to open a file if one is not open:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/cache/src/nsDiskCacheStreams.cpp&rev=1.17.4.1&mark=484,489,675,680,686#669

But FlushBufferToFile has two chances to fail and return early before actually opening a file and setting mFD.
(Assignee)

Comment 8

11 years ago
Created attachment 219691 [details] [diff] [review]
Don't call PR_Close(0), pass error condition back up to caller

This should fix it.
Assignee: darin → mark
Attachment #219691 - Flags: review?(darin)
Comment on attachment 219691 [details] [diff] [review]
Don't call PR_Close(0), pass error condition back up to caller

well, this bug clearly shows that mFD can be null at this point. so shouldn't the assertion about mFD being nonnull be removed? and possibly the earlier one about NS_SUCCEEDED(rv) as well?

Comment 10

11 years ago
Comment on attachment 219691 [details] [diff] [review]
Don't call PR_Close(0), pass error condition back up to caller

>Index: mozilla/netwerk/cache/src/nsDiskCacheStreams.cpp
...
>         rv = FlushBufferToFile();       // will initialize DataFileLocation() if necessary
>         NS_ASSERTION(NS_SUCCEEDED(rv), "FlushBufferToFile() failed");
...
>+        NS_ENSURE_SUCCESS(rv, rv);

Perhaps there is no reason to emit a NS_WARNING following the
assertion failure?  |if (NS_FAILED(rv)) return rv;| instead?

r=darin
Attachment #219691 - Flags: review?(darin) → review+
(Assignee)

Comment 11

11 years ago
Now that I think about it, I can see what biesi means.  These conditions are errors, but they don't need to be fatal.  (Yeah, we rarely make assertions fatal, but you know what I mean.)

Darin, how about getting rid of the NS_ASSERTIONS, using NS_ENSURE_SUCCESS for the return as in the patch, and adding an NS_WARNING inside the !mFD block?

Comment 12

11 years ago
> Darin, how about getting rid of the NS_ASSERTIONS, using NS_ENSURE_SUCCESS for
> the return as in the patch, and adding an NS_WARNING inside the !mFD block?

That does sound better.  The cache definitely uses NS_ASSERTION when it means NS_WARNING in more than one place :(
(Assignee)

Comment 13

11 years ago
Created attachment 219773 [details] [diff] [review]
As checked in

This can make 1.8.0.3 after all if you want it to - but if it needs bake time, it can be in 1.8.0.4 for sure.
Attachment #219691 - Attachment is obsolete: true
Attachment #219773 - Flags: approval1.8.0.3?
Attachment #219773 - Flags: approval-branch-1.8.1?(darin)
(Assignee)

Comment 14

11 years ago
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Comment on attachment 219773 [details] [diff] [review]
As checked in

Still too late for this release, but thanks for the patch! Nominating for the next one.
Attachment #219773 - Flags: approval1.8.0.5?
Attachment #219773 - Flags: approval1.8.0.4?
Attachment #219773 - Flags: approval1.8.0.4-
(Assignee)

Comment 16

11 years ago
Created attachment 219916 [details] [diff] [review]
1.8/1.8.0 branch version of patch
Attachment #219916 - Flags: approval1.8.0.4?
Attachment #219916 - Flags: approval-branch-1.8.1?(darin)
(Assignee)

Updated

11 years ago
Attachment #219773 - Flags: approval1.8.0.5?
Attachment #219773 - Flags: approval-branch-1.8.1?(darin)
(Assignee)

Updated

11 years ago
Attachment #219916 - Flags: approval1.8.0.4? → approval1.8.0.5?
(Assignee)

Updated

11 years ago
Whiteboard: [camino-1.0.1]

Comment 17

11 years ago
Comment on attachment 219916 [details] [diff] [review]
1.8/1.8.0 branch version of patch

a=darin for the MOZILLA_1_8_BRANCH
Attachment #219916 - Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+
(Assignee)

Comment 18

11 years ago
Checked in on MOZILLA_1_8_BRANCH for 1.8.1.
Keywords: fixed1.8.1
Flags: blocking1.8.0.5? → blocking1.8.0.5+
(Assignee)

Updated

11 years ago
Whiteboard: [camino-1.0.1] → [camino-1.0.1][camino-1.0.2]
Comment on attachment 219916 [details] [diff] [review]
1.8/1.8.0 branch version of patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #219916 - Flags: approval1.8.0.5? → approval1.8.0.5+
(Assignee)

Comment 20

11 years ago
Checked in on MOZILLA_1_8_0_BRANCH for 1.8.0.5.
Keywords: fixed1.8.0.5
Crash Signature: [@PR_Close] [@nsDiskCacheStreamIO::Flush()]
You need to log in before you can comment on or make changes to this bug.