crash [@ je_free | CloseDir]

RESOLVED FIXED in Firefox 10

Status

()

Core
Networking: File
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Mats Palmgren (vacation - back in August), Assigned: Mats Palmgren (vacation - back in August))

Tracking

({crash})

Trunk
mozilla11
x86
Windows XP
crash
Points:
---

Firefox Tracking Flags

(firefox10 fixed)

Details

(Whiteboard: [qa-][CLOSEME:2012-03-21], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

bp-a5bb0ffc-e274-42a6-ae0c-a0c302111207

je_free                           memory/jemalloc/jemalloc.c:6580
CloseDir                          xpcom/io/nsLocalFileWin.cpp:579
nsDirEnumerator::HasMoreElements  xpcom/io/nsLocalFileWin.cpp:633
nsDeleteDir::RemoveDir            netwerk/cache/nsDeleteDir.cpp:433
nsDeleteDir::RemoveDir            netwerk/cache/nsDeleteDir.cpp:446
nsDeleteDir::TimerCallback        netwerk/cache/nsDeleteDir.cpp:216
nsTimerImpl::Fire
nsTimerEvent::Run
nsThread::ProcessNextEvent
nsThreadStartupEvent::`vector deleting destructor'      
nsThread::ThreadFunc
_PR_NativeRunThread
@0x7264 
_getptd_noexit
pr_root
_callthreadstartex
_threadstartex
BaseThreadStart
CloseDir unconditionally deletes the nsDir arg.
http://hg.mozilla.org/mozilla-central/annotate/1bd7482ad4d1/xpcom/io/nsLocalFileWin.cpp#l579

If it returns a failure code, mDir is not nulled out at the call sites.
http://hg.mozilla.org/mozilla-central/annotate/1bd7482ad4d1/xpcom/io/nsLocalFileWin.cpp#l633
so I suspect the crash is a double delete on mDir.
Created attachment 581097 [details] [diff] [review]
fix

Something like this might fix it....
Attachment #581097 - Flags: review?(netzen)
I'm not on the XPCOM peer list but I checked with bsmedberg and he mentioned I can take the review.  So I'll review it shortly.
Comment on attachment 581097 [details] [diff] [review]
fix

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

I think it might be cleanest to simply pass in a reference to the pointer.
It would also be a patch with only a single character addition.

PR_DELETE will NULL out the pointer as per:
https://developer.mozilla.org/en/PR_DELETE

> static nsresult
> CloseDir(nsDir *&d)
Attachment #581097 - Flags: review?(netzen)
Created attachment 581630 [details] [diff] [review]
fix, v2
Assignee: nobody → matspal
Attachment #581630 - Flags: review?(netzen)
Attachment #581097 - Attachment is obsolete: true
Comment on attachment 581630 [details] [diff] [review]
fix, v2

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

Looks good!

::: xpcom/io/nsLocalFileWin.cpp
@@ +575,5 @@
>  {
>      NS_ENSURE_ARG(d);
>  
>      BOOL isOk = FindClose(d->handle);
> +    PR_DELETE(d);  // nulls out the slot |d| refers to (mDir)

nit: please put the comment above PR_DELETE
// PR_DELETE also nulls out the passed in pointer
Attachment #581630 - Flags: review?(netzen) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9af6b613457
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
Target Milestone: mozilla10 → mozilla11
https://hg.mozilla.org/mozilla-central/rev/e9af6b613457
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Comment on attachment 581630 [details] [diff] [review]
fix, v2

low-risk crash fix
Attachment #581630 - Flags: approval-mozilla-beta?

Updated

6 years ago
Attachment #581630 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 10

6 years ago
http://hg.mozilla.org/releases/mozilla-beta/rev/9588ec86d6e6

Already in aurora as it landed on central when Fx11 was there.
status-firefox10: --- → fixed
Is this something QA can verify? If so, what's the test case?
Whiteboard: [qa?]
I don't think one can reproduce this easily.
Whiteboard: [qa?] → [qa-]
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #11)
> Is this something QA can verify?

I wish there was a flag for "someone should come back to this bug in three
months time and query crash-stats.mozilla.com for the signature(s) and reopen
the bug if it still occurs in builds that have the patch".

I do that sometimes... but not very systematically I'm afraid.  Is that
something QA can do?
We can use the closeme whiteboard tag for that. In other words, if this crash is not seen in Crashstats 3 months from now, mark bug verified.
Whiteboard: [qa-] → [qa-][CLOSEME:2012-03-21]
Sounds like a good way to verify, thanks!
You need to log in before you can comment on or make changes to this bug.