Closed Bug 710060 Opened 10 years ago Closed 10 years ago

crash [@ je_free | CloseDir]

Categories

(Core :: Networking: File, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla11
Tracking Status
firefox10 --- fixed

People

(Reporter: mats, Assigned: mats)

Details

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

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
Attached patch fix (obsolete) — Splinter Review
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)
Attached patch fix, v2Splinter Review
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
Closed: 10 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?
Attachment #581630 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
http://hg.mozilla.org/releases/mozilla-beta/rev/9588ec86d6e6

Already in aurora as it landed on central when Fx11 was there.
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.