Last Comment Bug 321366 - Crash [@PR_Close].[@nsDiskCacheStreamIO::Flush()]
: Crash [@PR_Close].[@nsDiskCacheStreamIO::Flush()]
Status: RESOLVED FIXED
[camino-1.0.1][camino-1.0.2]
: crash, fixed1.8.0.5, fixed1.8.1, topcrash
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: 1.8 Branch
: All All
: P1 critical (vote)
: mozilla1.8.1
Assigned To: Mark Mentovai
:
Mentors:
: 326929 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-12-23 13:21 PST by Mark Mentovai
Modified: 2011-06-13 10:01 PDT (History)
6 users (show)
dveditz: blocking1.8.0.4-
dveditz: blocking1.8.0.5+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Stack trace (27.81 KB, text/plain)
2005-12-23 13:22 PST, Mark Mentovai
no flags Details
analysis of TB17282316 (17.91 KB, text/plain; charset=utf-8)
2006-04-21 18:55 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
Don't call PR_Close(0), pass error condition back up to caller (1.83 KB, patch)
2006-04-24 17:34 PDT, Mark Mentovai
darin.moz: review+
Details | Diff | Review
As checked in (2.05 KB, patch)
2006-04-25 11:09 PDT, Mark Mentovai
dveditz: approval1.8.0.4-
Details | Diff | Review
1.8/1.8.0 branch version of patch (1.89 KB, patch)
2006-04-26 13:10 PDT, Mark Mentovai
darin.moz: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Review

Description Mark Mentovai 2005-12-23 13:21:48 PST
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.
Comment 1 Mark Mentovai 2005-12-23 13:22:35 PST
Created attachment 206729 [details]
Stack trace
Comment 2 Walter Wlodarski 2006-02-09 20:56:11 PST
Seems to be the same problem : http://forums.mozillazine.org/viewtopic.php?t=379065
Comment 3 Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-04-02 07:56:20 PDT
*** Bug 326929 has been marked as a duplicate of this bug. ***
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-04-21 18:55:38 PDT
Created attachment 219388 [details]
analysis of TB17282316

This analysis shows that the crash is because the fd passed to PR_Close is null.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-04-21 18:57:01 PDT
(This is one of the top crashes on Linux and Mac; doesn't appear to be showing up much on Windows.)
Comment 6 Daniel Veditz [:dveditz] 2006-04-24 12:52:22 PDT
Looks like this is missing 1.8.0.3, try 1.8.0.4
Comment 7 Mark Mentovai 2006-04-24 17:18:39 PDT
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.
Comment 8 Mark Mentovai 2006-04-24 17:34:06 PDT
Created attachment 219691 [details] [diff] [review]
Don't call PR_Close(0), pass error condition back up to caller

This should fix it.
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2006-04-24 18:07:02 PDT
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 Darin Fisher 2006-04-24 19:26:09 PDT
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
Comment 11 Mark Mentovai 2006-04-24 20:22:45 PDT
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 Darin Fisher 2006-04-24 22:24:05 PDT
> 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 :(
Comment 13 Mark Mentovai 2006-04-25 11:09:24 PDT
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.
Comment 14 Mark Mentovai 2006-04-25 11:09:53 PDT
Fixed on trunk.
Comment 15 Daniel Veditz [:dveditz] 2006-04-26 12:02:10 PDT
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.
Comment 16 Mark Mentovai 2006-04-26 13:10:17 PDT
Created attachment 219916 [details] [diff] [review]
1.8/1.8.0 branch version of patch
Comment 17 Darin Fisher 2006-04-26 14:32:17 PDT
Comment on attachment 219916 [details] [diff] [review]
1.8/1.8.0 branch version of patch

a=darin for the MOZILLA_1_8_BRANCH
Comment 18 Mark Mentovai 2006-04-26 15:01:35 PDT
Checked in on MOZILLA_1_8_BRANCH for 1.8.1.
Comment 19 Daniel Veditz [:dveditz] 2006-06-05 11:30:03 PDT
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
Comment 20 Mark Mentovai 2006-06-06 09:31:05 PDT
Checked in on MOZILLA_1_8_0_BRANCH for 1.8.0.5.

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