710060 - crash [@ je_free | CloseDir]

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[Mats Palmgren (inactive)]

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

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

Attachment: fix

Something like this might fix it....

Comment 3

[Brian R. Bondy [:bbondy]]

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

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

Attachment: fix, v2

Comment 6

[Brian R. Bondy [:bbondy]]

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 7

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9af6b613457

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/e9af6b613457

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 581630 [details] [diff] [review]
fix, v2

low-risk crash fix

Comment 10

[LegNeato]

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

Already in aurora as it landed on central when Fx11 was there.

Comment 11

[u279076]

Is this something QA can verify? If so, what's the test case?

Comment 12

[Brian R. Bondy [:bbondy]]

I don't think one can reproduce this easily.

Comment 13

[Mats Palmgren (inactive)]

(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

[u279076]

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

Sounds like a good way to verify, thanks!