Closed
Bug 198634
Opened 21 years ago
Closed 21 years ago
Fixes to OS/2 threading code
Categories
(NSPR :: NSPR, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
4.4
People
(Reporter: mkaply, Assigned: wtc)
Details
Attachments
(2 files, 1 obsolete file)
2.26 KB,
patch
|
mkaply
:
review+
wtc
:
superreview+
|
Details | Diff | Splinter Review |
863 bytes,
patch
|
mkaply
:
review+
|
Details | Diff | Splinter Review |
We're getting asserts in our threading code which is causing the new NSS utility not to work (shlibsign) Fix coming.
Reporter | ||
Comment 1•21 years ago
|
||
Reporter | ||
Comment 2•21 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•21 years ago
|
Attachment #118047 -
Flags: superreview+
Assignee | ||
Comment 3•21 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
Closed: 21 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → 4.3
Comment 4•21 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•21 years ago
|
||
When would PR_DetachThread ever get called? Would we expect outside users of NSPR threads to do this?
Comment 6•21 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•21 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•21 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•21 years ago
|
||
As I pointed out in my first comment, no. That's why I need it.
Assignee | ||
Comment 10•21 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•21 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•21 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•21 years ago
|
||
Thanks.
Reporter | ||
Comment 14•21 years ago
|
||
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•21 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•21 years ago
|
||
Then lets just #ifdef XP_OS2 this so we can get it in. Thanks
Assignee | ||
Comment 17•21 years ago
|
||
Please try this patch. Thanks.
Attachment #118697 -
Attachment is obsolete: true
Reporter | ||
Comment 18•21 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•21 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
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
Whiteboard: [4.3.1]
Assignee | ||
Comment 20•21 years ago
|
||
I checked in the PR_DetachThread patch on the NSPR trunk.
Assignee | ||
Updated•21 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.
Description
•