198634 - Fixes to OS/2 threading code

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Mike Kaply [:mkaply]]

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]]

Attachment: Fix for problem

Comment 2

[Mike Kaply [:mkaply]]

Comment on attachment 118047 [details] [diff] [review]
Fix for problem

r=mkaply - this patch was from Javier

Comment 3

[Wan-Teh Chang]

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]

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]]

When would PR_DetachThread ever get called?

Would we expect outside users of NSPR threads to do this?

Comment 6

[knut st. osmundsen]

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]]

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]

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]

As I pointed out in my first comment, no. That's why I need it.

Comment 10

[Wan-Teh Chang]

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]

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]]

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]

Thanks.

Comment 14

[Mike Kaply [:mkaply]]

Attachment: 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]

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]]

Then lets just #ifdef XP_OS2 this so we can get it in.

Thanks

Comment 17

[Wan-Teh Chang]

Attachment: Proper implementation of PR_DetachThread v1.1

Please try this patch.	Thanks.

Comment 18

[Mike Kaply [:mkaply]]

Comment on attachment 119883 [details] [diff] [review]
Proper implementation of PR_DetachThread v1.1

This looks good to me.

Comment 19

[Wan-Teh Chang]

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]

I checked in the PR_DetachThread patch on the
NSPR trunk.