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

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P1)

Description

[Mark Mentovai]

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]

Attachment: Stack trace

Comment 2

[Walter Wlodarski]

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)]

*** Bug 326929 has been marked as a duplicate of this bug. ***

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

(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]]

Looks like this is missing 1.8.0.3, try 1.8.0.4

Comment 7

[Mark Mentovai]

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]

Attachment: 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)]

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]

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]

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]

> 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]

Attachment: 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]

Fixed on trunk.

Comment 15

[Daniel Veditz [:dveditz]]

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]

Attachment: 1.8/1.8.0 branch version of patch

Comment 17

[Darin Fisher]

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]

Checked in on MOZILLA_1_8_BRANCH for 1.8.1.

Comment 19

[Daniel Veditz [:dveditz]]

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]

Checked in on MOZILLA_1_8_0_BRANCH for 1.8.0.5.