Closed
Bug 321366
Opened 19 years ago
Closed 18 years ago
Crash [@PR_Close].[@nsDiskCacheStreamIO::Flush()]
Categories
(Core :: Networking: Cache, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.8.1
People
(Reporter: mark, Assigned: mark)
References
Details
(4 keywords, Whiteboard: [camino-1.0.1][camino-1.0.2])
Crash Data
Attachments
(4 files, 1 obsolete file)
27.81 KB,
text/plain
|
Details | |
17.91 KB,
text/plain; charset=utf-8
|
Details | |
2.05 KB,
patch
|
dveditz
:
approval1.8.0.4-
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
darin.moz
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.5+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Comment 2•19 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. ***
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•18 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1
Comment 6•18 years ago
|
||
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•18 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•18 years ago
|
||
This should fix it.
Assignee: darin → mark
Attachment #219691 -
Flags: review?(darin)
Comment 9•18 years ago
|
||
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•18 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•18 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•18 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•18 years ago
|
||
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•18 years ago
|
||
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 15•18 years ago
|
||
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•18 years ago
|
||
Attachment #219916 -
Flags: approval1.8.0.4?
Attachment #219916 -
Flags: approval-branch-1.8.1?(darin)
Assignee | ||
Updated•18 years ago
|
Attachment #219773 -
Flags: approval1.8.0.5?
Attachment #219773 -
Flags: approval-branch-1.8.1?(darin)
Assignee | ||
Updated•18 years ago
|
Attachment #219916 -
Flags: approval1.8.0.4? → approval1.8.0.5?
Assignee | ||
Updated•18 years ago
|
Whiteboard: [camino-1.0.1]
Comment 17•18 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+
Updated•18 years ago
|
Flags: blocking1.8.0.5? → blocking1.8.0.5+
Assignee | ||
Updated•18 years ago
|
Whiteboard: [camino-1.0.1] → [camino-1.0.1][camino-1.0.2]
Comment 19•18 years ago
|
||
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•18 years ago
|
||
Checked in on MOZILLA_1_8_0_BRANCH for 1.8.0.5.
Keywords: fixed1.8.0.5
Updated•13 years ago
|
Crash Signature: [@PR_Close]
[@nsDiskCacheStreamIO::Flush()]
You need to log in
before you can comment on or make changes to this bug.
Description
•