Last Comment Bug 198634 - Fixes to OS/2 threading code
: Fixes to OS/2 threading code
Status: RESOLVED FIXED
:
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: 4.2
: x86 OS/2
: P1 critical (vote)
: 4.4
Assigned To: Wan-Teh Chang
: Wan-Teh Chang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-03-21 10:55 PST by Mike Kaply [:mkaply]
Modified: 2003-11-26 16:41 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix for problem (2.26 KB, patch)
2003-03-21 10:58 PST, Mike Kaply [:mkaply]
mozilla: review+
wtc: superreview+
Details | Diff | Splinter Review
Proper implementation of PR_DetachThread (723 bytes, patch)
2003-03-27 13:40 PST, Mike Kaply [:mkaply]
no flags Details | Diff | Splinter Review
Proper implementation of PR_DetachThread v1.1 (863 bytes, patch)
2003-04-08 16:08 PDT, Wan-Teh Chang
mozilla: review+
Details | Diff | Splinter Review

Description Mike Kaply [:mkaply] 2003-03-21 10:55:53 PST
We're getting asserts in our threading code which is causing the new NSS utility
not to work (shlibsign)

Fix coming.
Comment 1 Mike Kaply [:mkaply] 2003-03-21 10:58:41 PST
Created attachment 118047 [details] [diff] [review]
Fix for problem
Comment 2 Mike Kaply [:mkaply] 2003-03-21 12:56:51 PST
Comment on attachment 118047 [details] [diff] [review]
Fix for problem

r=mkaply - this patch was from Javier
Comment 3 Wan-Teh Chang 2003-03-22 07:04:25 PST
Patch checked into the NSPR TIP (4.3) and NSPRPUB_PRE_4_2_CLIENT_BRANCH
(mozilla 1.4alpha).

Is this for the assertion failure in PR_Cleanup()?  I seem to recall
you opened a bug about that but can't find it.  Maybe you told me about
it in email.
Comment 4 knut st. osmundsen 2003-03-22 10:21:42 PST
Question: shouldn't you implement PR_DetachThread too for OS2? 
_PRI_DeatchThread()/PR_DeatchThread isn't called if an attached thread exits
outside mozilla.
Meaning, NSPR on Win32 will catch this case in the DllMain() of NSPR, OS/2
won't. So, the result will be some leaked PRThread structs and a wrong thread
count (_pr_userActive).
Patch Suggestion:

PR_IMPLEMENT(void) PR_DetachThread(void)
{
#ifdef XP_OS2
    if (_pr_initialized) {
        me = _MD_GET_ATTACHED_THREAD();
        if ((me != NULL) && (me->flags & _PR_ATTACHED))
            _PRI_DetachThread();
    }
#endif
}
Comment 5 Mike Kaply [:mkaply] 2003-03-22 20:19:11 PST
When would PR_DetachThread ever get called?

Would we expect outside users of NSPR threads to do this?
Comment 6 knut st. osmundsen 2003-03-23 13:25:22 PST
Except for the testcase mozilla\nsprpub\pr\tests\attach.c no-one calls it. 
Guessing the intended usage was to call this before exiting a user thread
previously attached with PR_AttachThread(). Like we do from our private
DLL_THREAD_DETACH handler. 
Or if you like, you can export _PRI_DetachThread let me countinue call it, as
I'm doing now.
Comment 7 Mike Kaply [:mkaply] 2003-03-23 14:54:41 PST
I'd rather give you PR_DetachThread.

So it's mainly for externals to use. That's cool.

I'll put this in the NSPR I give you Monday.

Comment 8 Wan-Teh Chang 2003-03-24 17:10:42 PST
In the current version of NSPR, PR_DetachThread is a no-op.
It is only there for backward compatibility purposes.

What PR_DetechThread used to do should now be done by a
handler that is called on thread termination.  On Unix,
this is done as a thread-specific data destructor.  On
Win32, this is done as DLL_THREAD_DETACH code in DllMain().
Does OS/2 have something that you can use for this purpose?
Comment 9 knut st. osmundsen 2003-03-25 02:26:54 PST
As I pointed out in my first comment, no. That's why I need it.
Comment 10 Wan-Teh Chang 2003-03-25 07:07:33 PST
I see.  I asked because you mentioned your "DLL_THREAD_DETACH handler"
in comment 6.  Can we add a DLL_THREAD_DETACH handler to NSPR?
Comment 11 knut st. osmundsen 2003-03-25 14:10:01 PST
Sorry, about the confusion. I've a habit of forgetting to assert the context in
which I'm writing. We're 'porting' (wrapping are perhaps a better term as we
don't change the binary) a Win32 plugin to OS/2, and I was refering to our
_private_ DLL_THREAD_DETACH.
Comment 12 Mike Kaply [:mkaply] 2003-03-27 08:32:46 PST
To be very clear.

OS/2 has NO way to detect thread death.

We MUST have a PR_DetachThread for OS/2 and expect people to call it.

Patch coming.
Comment 13 knut st. osmundsen 2003-03-27 08:44:14 PST
Thanks.
Comment 14 Mike Kaply [:mkaply] 2003-03-27 13:40:38 PST
Created attachment 118697 [details] [diff] [review]
Proper implementation of PR_DetachThread

OK, here is an implementation of PR_DetachThread.

I deliberately did not make it XP_OS2. There should be a PR_DetachThread that
can be called if necessary. You can't always rely on platform specific
behaviors to detach threads.
Comment 15 Wan-Teh Chang 2003-03-31 16:23:21 PST
This should probably be ifdef'ed with

  #if !defined(WIN32)

because Win32 threads are detached in the DllMain()
function of nspr4.dll.

Mike, I think your patch is correct; it's just that
I need to think about it more to be sure that no code
can possibly depend on the current behavior.  So adding
the ifdef is safer.
Comment 16 Mike Kaply [:mkaply] 2003-04-08 12:16:08 PDT
Then lets just #ifdef XP_OS2 this so we can get it in.

Thanks
Comment 17 Wan-Teh Chang 2003-04-08 16:08:39 PDT
Created attachment 119883 [details] [diff] [review]
Proper implementation of PR_DetachThread v1.1

Please try this patch.	Thanks.
Comment 18 Mike Kaply [:mkaply] 2003-04-09 08:44:02 PDT
Comment on attachment 119883 [details] [diff] [review]
Proper implementation of PR_DetachThread v1.1

This looks good to me.
Comment 19 Wan-Teh Chang 2003-04-09 08:46:03 PDT
I checked in the PR_DetachThread patch on the
NSPRPUB_PRE_4_2_CLIENT_BRANCH (mozilla 1.4 beta)
last night.
Comment 20 Wan-Teh Chang 2003-04-09 19:23:21 PDT
I checked in the PR_DetachThread patch on the
NSPR trunk.

Note You need to log in before you can comment on or make changes to this bug.