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

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Andrew Schultz, Assigned: dougt)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
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
(Assignee)

Comment 1

16 years ago
Created attachment 103870 [details] [diff] [review]
proposed patch
(Assignee)

Comment 2

16 years ago
Andrew, can you test out this patch using valgrind?
(Reporter)

Comment 3

16 years ago
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

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

Comment 5

16 years ago
Comment on attachment 103870 [details] [diff] [review]
proposed patch

r=pavlov
Attachment #103870 - Flags: review+

Comment 6

16 years ago
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
Attachment #103870 - Flags: superreview+
(Assignee)

Comment 7

16 years ago
checked in to 1.3 trunk.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.