419763 - logger thread should be joined on exit

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Julien Pierre]

Similar to 418360, we leave a thread running in selfserv before PR_Cleanup() is called. This can cause issues such as assertions. The fix is to start the thread as joinable and join it before PR_Cleanup.

Comment 1

[Julien Pierre]

Attachment: join logger thread, if started

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 305907 [details] [diff] [review]
join logger thread, if started

This patch is good, as far as it goes, but it attempts to join an 
unjoinable thread.  The thread must be made joinable when it is created.

2071    loggerThread = PR_CreateThread(PR_SYSTEM_THREAD, 
2072                    logger, NULL, PR_PRIORITY_NORMAL, 
2073                    useLocalThreads ? PR_LOCAL_THREAD:PR_GLOBAL_THREAD,
2074                    PR_UNJOINABLE_THREAD, 0);
                           ^^

Comment 3

[Julien Pierre]

Oops. Thanks for catching that, Nelson. Strangely enough, I had tested the patch (with the -L option) and didn't get a hang. It must be that PR_JoinThread skips and/or fails when trying to join unjoinable threads.

Comment 4

[Julien Pierre]

Attachment: also make thread joinable

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Julien, Yes, PR_JoinThread returns PR_FAILURE if the thread is unjoinable.
Maybe your patch should detect that case and either report a warning
or report a test failure.  What do you think?

Comment 6

[Julien Pierre]

Nelson,

Yes, that would be an improvement. I think the attached patch is good enough. It seems that not a single program under cmd checks for the return value of PR_JoinThread. This being a fairly optional thread, I wouldn't think it would be the first place to check it.

I notice that we don't join the worker threads in selfserv. We wait for a thread counter to go to zero instead . This may be OK, but it's conceivable that NSPR still executes some code after the final PZ_Unlock(qLock) in thread_wrapper which could cause NSPR reinitialization, and thus conflit with PR_Cleanup as well. I'm not saying it does today, just that there is a potential race there too if we keep the PR_Cleanup call. I would prefer to see a PR_JoinThread loop in terminateWorkerThreads to what we have now.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 306149 [details] [diff] [review]
also make thread joinable

r=nelson

Comment 8

[Julien Pierre]

Thanks, Nelson.

I checked this in to the trunk 

Checking in selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.83; previous revision: 1.82
done