The default bug view has changed. See this FAQ.

Fixes to OS/2 threading code

RESOLVED FIXED in 4.4

Status

NSPR
NSPR
P1
critical
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: mkaply, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

14 years ago
We're getting asserts in our threading code which is causing the new NSS utility
not to work (shlibsign)

Fix coming.
(Reporter)

Comment 1

14 years ago
Created attachment 118047 [details] [diff] [review]
Fix for problem
(Reporter)

Comment 2

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

r=mkaply - this patch was from Javier
Attachment #118047 - Flags: review+
(Assignee)

Updated

14 years ago
Attachment #118047 - Flags: superreview+
(Assignee)

Comment 3

14 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → 4.3

Comment 4

14 years ago
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
}
(Reporter)

Comment 5

14 years ago
When would PR_DetachThread ever get called?

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

Comment 6

14 years ago
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.
(Reporter)

Comment 7

14 years ago
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.

(Assignee)

Comment 8

14 years ago
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

14 years ago
As I pointed out in my first comment, no. That's why I need it.
(Assignee)

Comment 10

14 years ago
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

14 years ago
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.
(Reporter)

Comment 12

14 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 13

14 years ago
Thanks.
(Reporter)

Comment 14

14 years ago
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.
(Assignee)

Comment 15

14 years ago
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.
(Reporter)

Comment 16

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

Thanks
(Assignee)

Comment 17

14 years ago
Created attachment 119883 [details] [diff] [review]
Proper implementation of PR_DetachThread v1.1

Please try this patch.	Thanks.
Attachment #118697 - Attachment is obsolete: true
(Reporter)

Comment 18

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

This looks good to me.
Attachment #119883 - Flags: review+
(Assignee)

Comment 19

14 years ago
I checked in the PR_DetachThread patch on the
NSPRPUB_PRE_4_2_CLIENT_BRANCH (mozilla 1.4 beta)
last night.
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
Whiteboard: [4.3.1]
(Assignee)

Comment 20

14 years ago
I checked in the PR_DetachThread patch on the
NSPR trunk.
(Assignee)

Updated

14 years ago
Whiteboard: [4.3.1]
Target Milestone: 4.3 → 4.4
You need to log in before you can comment on or make changes to this bug.