Last Comment Bug 710060 - crash [@ je_free | CloseDir]
: crash [@ je_free | CloseDir]
Status: RESOLVED FIXED
[qa-][CLOSEME:2012-03-21]
: crash
Product: Core
Classification: Components
Component: Networking: File (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: mozilla11
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-12 16:34 PST by Mats Palmgren (:mats)
Modified: 2011-12-28 15:15 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
fix (1.85 KB, patch)
2011-12-12 16:43 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
fix, v2 (2.22 KB, patch)
2011-12-14 07:09 PST, Mats Palmgren (:mats)
netzen: review+
christian: approval‑mozilla‑beta+
Details | Diff | Review

Description Mats Palmgren (:mats) 2011-12-12 16:34:02 PST
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
Comment 1 Mats Palmgren (:mats) 2011-12-12 16:38:56 PST
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.
Comment 2 Mats Palmgren (:mats) 2011-12-12 16:43:26 PST
Created attachment 581097 [details] [diff] [review]
fix

Something like this might fix it....
Comment 3 Brian R. Bondy [:bbondy] 2011-12-13 11:22:22 PST
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 4 Brian R. Bondy [:bbondy] 2011-12-13 18:32:18 PST
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)
Comment 5 Mats Palmgren (:mats) 2011-12-14 07:09:46 PST
Created attachment 581630 [details] [diff] [review]
fix, v2
Comment 6 Brian R. Bondy [:bbondy] 2011-12-14 07:14:05 PST
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
Comment 8 Matt Brubeck (:mbrubeck) 2011-12-18 15:32:49 PST
https://hg.mozilla.org/mozilla-central/rev/e9af6b613457
Comment 9 Mats Palmgren (:mats) 2011-12-22 03:53:03 PST
Comment on attachment 581630 [details] [diff] [review]
fix, v2

low-risk crash fix
Comment 10 christian 2011-12-27 14:30:17 PST
http://hg.mozilla.org/releases/mozilla-beta/rev/9588ec86d6e6

Already in aurora as it landed on central when Fx11 was there.
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 13:15:48 PST
Is this something QA can verify? If so, what's the test case?
Comment 12 Brian R. Bondy [:bbondy] 2011-12-28 13:26:36 PST
I don't think one can reproduce this easily.
Comment 13 Mats Palmgren (:mats) 2011-12-28 13:40:40 PST
(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?
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 13:44:01 PST
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.
Comment 15 Brian R. Bondy [:bbondy] 2011-12-28 15:15:35 PST
Sounds like a good way to verify, thanks!

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