As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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 User image 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 User image Mike Kaply [:mkaply] 2003-03-21 10:58:41 PST
Created attachment 118047 [details] [diff] [review]
Fix for problem
Comment 2 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image knut st. osmundsen 2003-03-27 08:44:14 PST
Thanks.
Comment 14 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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.