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.
Created attachment 305907 [details] [diff] [review] join logger thread, if started
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); ^^
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.
Created attachment 306149 [details] [diff] [review] also make thread joinable
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?
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 on attachment 306149 [details] [diff] [review] also make thread joinable r=nelson
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