Last Comment Bug 710060 - crash [@ je_free | CloseDir]
: crash [@ je_free | CloseDir]
: crash
Product: Core
Classification: Components
Component: Networking: File (show other bugs)
: Trunk
: x86 Windows XP
-- critical (vote)
: mozilla11
Assigned To: Mats Palmgren (:mats)
: Patrick McManus [:mcmanus]
Depends on:
  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: ---

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

Description User image Mats Palmgren (:mats) 2011-12-12 16:34:02 PST

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
nsThreadStartupEvent::`vector deleting destructor'      
Comment 1 User image Mats Palmgren (:mats) 2011-12-12 16:38:56 PST
CloseDir unconditionally deletes the nsDir arg.

If it returns a failure code, mDir is not nulled out at the call sites.
so I suspect the crash is a double delete on mDir.
Comment 2 User image Mats Palmgren (:mats) 2011-12-12 16:43:26 PST
Created attachment 581097 [details] [diff] [review]

Something like this might fix it....
Comment 3 User image 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 User image Brian R. Bondy [:bbondy] 2011-12-13 18:32:18 PST
Comment on attachment 581097 [details] [diff] [review]

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:

> static nsresult
> CloseDir(nsDir *&d)
Comment 5 User image Mats Palmgren (:mats) 2011-12-14 07:09:46 PST
Created attachment 581630 [details] [diff] [review]
fix, v2
Comment 6 User image 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 User image Matt Brubeck (:mbrubeck) 2011-12-18 15:32:49 PST
Comment 9 User image 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 User image christian 2011-12-27 14:30:17 PST

Already in aurora as it landed on central when Fx11 was there.
Comment 11 User image 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 User image Brian R. Bondy [:bbondy] 2011-12-28 13:26:36 PST
I don't think one can reproduce this easily.
Comment 13 User image 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 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 User image 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 User image 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.