nsThread Init races with Main

RESOLVED FIXED

Status

()

Core
XPCOM
P3
normal
RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: John Bandhauer, Assigned: John Bandhauer)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

18 years ago
While testing multithreaded JS I ran into a case where nsThread crashes when
trying to call self->GetState from nsThread::Main with a passed mThread of
nsnull (though in the debugger mThread looks good. I wrote an extention to
TestThreads.cpp to trigger this. Then I looked at the code close enough to
figure out the problem...

nsThread::GetState is using mThread which is set by the return value of the call
to PR_CreateThread. nsThread::GetState is called on the new created thread from
nsThread::Main. However, nspr can start running the created thread's threadProc
before PR_CreateThread returns. This is documented in prthread.h. So, the
setting of mThread in Init races with the user of mThread in Main.

My fix is to do as sugested in prthread.h and use a lock to sync the running of
Main to happen after PR_CreateThread has finished. This costs us one PRLock per
thread.

I'll attach my patches for TestThreads.cpp and nsThread.*

Also, I extended nsIThread a bit to add named constants for the nspr priority,
scope, and state #defines. And, I added support for getting the current thread
and sleep to the interface. All to make nsIThread more usable from non-C++
languages where the #defines and static members are not available.

I'd like review and approval to check this all in.
(Assignee)

Comment 1

18 years ago
Created attachment 19824 [details] [diff] [review]
proposed fix and testcase additions
(Assignee)

Comment 2

18 years ago
Created attachment 19825 [details] [diff] [review]
proposed fix and testcase additions
(Assignee)

Comment 3

18 years ago
Oops. Double submit of the attachments. They are identical.
Status: NEW → ASSIGNED
Keywords: patch
Typo: Until, not Untill (but "till" is an alternative spelling of that old
preposition).

Is a PRLock the right synchronization device, or could the wait be "long-ish"?
In any case, can't you PR_DestroyLock after Main starts and returns from the
handshake?

/be
(Assignee)

Comment 5

18 years ago
I'll fix the typo and do the PR_DestroyLock early...

void
nsThread::WaitUntilReadyToStartMain()
{
    PR_Lock(mStartLock);
    PR_Unlock(mStartLock);
    PR_DestroyLock(mStartLock);
    mStartLock = nsnull;
}

I'm inclined to leave the conditional cleanup in the dtor since there is at
least one way that Main might not run.

What would you have me use rather than a PRLock? On Win32 and Linux, at least,
we've observed that it is rarely the case that Main starts running before
PR_CreateThread returns. Do you really think it is worth creating a monitor? Or
going to the trouble of maintaining a class static shared lock and using a
condvar? It is not that big a deal to me, it just seems like overkill.
Lock is ok with me if the wait is rarely done, and short if done.  I was asking
for more details about how long one thread might be spinning on the lock.

/be
r=brendan@mozilla.org, forgot to say.

/be

Comment 8

18 years ago
Looks good. sr=waterson
(Assignee)

Comment 9

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