175932 - UMR: PostTimerEvent() does not check return value from GetPRThread()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew Schultz]

valgrind says: Conditional jump or move depends on uninitialised value(s)
 nsEventQueueServiceImpl::GetThreadEventQueue(PRThread*, nsIEventQueue**)
(nsEventQueueService.cpp:360)
 nsTimerImpl::PostTimerEvent() (nsTimerImpl.cpp:478)
 TimerThread::Run() (TimerThread.cpp:226)
 nsThread::Main(void*) (nsThread.cpp:121)

steps to reproduce:
1. start Mozilla browser under valgrind
2. Window->Mail
bug only occurs under valgrind, probably because it's dog-slow.

what happens:
1. nsTimerImpl() does mCallingThread->GetPRThread(&thread);
2. GetPRThread() returns NS_ERROR_FAILURE because it's mDead, doesn't set |*result|
3. nsTimerImpl() continues in ignorant bliss
gThread->mEventQueueService->GetThreadEventQueue(thread, getter_AddRefs(queue));
4. GetThreadEventQueue() attempts to use (unintialized) |thread|

nsTimerImpl() should check the return value from GetPRThread() and act appropriately

Comment 1

[Doug Turner (:dougt)]

Attachment: proposed patch

Comment 2

[Doug Turner (:dougt)]

Andrew, can you test out this patch using valgrind?

Comment 3

[Andrew Schultz]

I tried out the patch, and valgrind did not give a UMR, but Mozilla did give the
WARNING you added, so the patch is working.

Comment 4

[Stuart Parmenter]

i'm curious who is using a timer on their own thread

Comment 5

[Stuart Parmenter]

Comment on attachment 103870 [details] [diff] [review]
proposed patch

r=pavlov

Comment 6

[Darin Fisher]

Comment on attachment 103870 [details] [diff] [review]
proposed patch

>Index: nsThread.cpp

> NS_IMETHODIMP
> nsThread::GetPRThread(PRThread* *result)
> {
>+    if (mDead) {
>+        *result = nsnull;
>         return NS_ERROR_FAILURE;
>+    }    
>     *result = mThread;
>     return NS_OK;
> }

ugh... if callers checked return value, then we wouldn't have to
null the out param.  a quick LXR search shows a number of callers
that get it wrong (e.g. nsDnsService).	would be nice to fix-up
callers instead of making GetPRThread null the out param.

sr=darin

Comment 7

[Doug Turner (:dougt)]

checked in to 1.3 trunk.