Closed
Bug 485318
Opened 15 years ago
Closed 15 years ago
Failure when re-initializing the library
Categories
(NSPR :: NSPR, defect)
NSPR
NSPR
Tracking
(Not tracked)
VERIFIED
FIXED
4.8
People
(Reporter: ludovico.cavedon, Assigned: wtc)
Details
Attachments
(4 files, 4 obsolete files)
441 bytes,
text/plain
|
Details | |
782 bytes,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
2.14 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
4.21 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.7) Gecko/2009030423 Ubuntu/8.10 (intrepid) Firefox/3.0.7 Build Identifier: 4.7.1 The libraries fails to reinitialize correctly after an initialization/cleanup cycle and deadlocks. Reproducible: Always Steps to Reproduce: Run attached testcase. Actual Results: Init 1 Cleanup 1 success=1 Init 2 Cleanup 2 <deadlocked here> Expected Results: Init 1 Cleanup 1 success=1 Init 2 Cleanup 2 success=1
Reporter | ||
Comment 1•15 years ago
|
||
Reporter | ||
Comment 2•15 years ago
|
||
pt_book.user was not decremented upon cleanup
Reporter | ||
Comment 3•15 years ago
|
||
In addition, if I run the same testcase with NSPR_LOG_MODULES=all:5, I get: Init 1 -136541440[8ed2580]: Loaded library a.out (init) Cleanup 1 -136541440[8ed2580]: PR_Cleanup: shutting down NSPR success=1 Init 2 Segmentation fault This happens because logFile is not set to NULL. I am attaching a fix.
Updated•15 years ago
|
Attachment #369453 -
Flags: review?(wtc)
Comment 4•15 years ago
|
||
I converted this to a CVS diff to facilitate review with Bonsai's patch review tools.
Attachment #369454 -
Attachment is obsolete: true
Comment 5•15 years ago
|
||
Ludovico, what do you think of this alternative?
Reporter | ||
Comment 6•15 years ago
|
||
Yes, of course, you alternative is better! Thanks!
Comment 7•15 years ago
|
||
Comment on attachment 369458 [details] [diff] [review] Ludovico Cavedon's patch for log reinit, made to work with stdio too (checked in) Wan-Teh, please review this bug's two tiny patches.
Attachment #369458 -
Flags: review?(wtc)
Updated•15 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•15 years ago
|
Attachment #369453 -
Flags: review?(ted.mielczarek)
Updated•15 years ago
|
Attachment #369458 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 8•15 years ago
|
||
Comment on attachment 369453 [details] [diff] [review] fix Thanks for the bug report and the patches. This patch is on the right track, but it is incorrect when pt_book.this_many is 0 because it'll make pt_book.user become -1. There are two ways to fix this: 1. Replace --pt_book.user; by pt_book.user = 0; 2. In _PR_InitThreads, reset pt_book.system and pt_book.user to 0. The root cause of this bug is that NSPR wasn't designed for cleanup and re-initialization.
Attachment #369453 -
Flags: review?(wtc)
Attachment #369453 -
Flags: review?(ted.mielczarek)
Attachment #369453 -
Flags: review-
Updated•15 years ago
|
Attachment #369458 -
Attachment is obsolete: true
Attachment #369458 -
Flags: review?(wtc)
Attachment #369458 -
Flags: review?(ted.mielczarek)
Comment 9•15 years ago
|
||
Comment on attachment 369458 [details] [diff] [review] Ludovico Cavedon's patch for log reinit, made to work with stdio too (checked in) I take Wan-Teh's r- to the preceding patch (on which this one was based) to be an r- for this patch, too.
Updated•15 years ago
|
Attachment #369456 -
Attachment description: Ludovico Cavedon's patch for log reinit, covered to CVS diff → Ludovico Cavedon's patch for log reinit, converted to CVS diff
Attachment #369456 -
Attachment is obsolete: true
Assignee | ||
Comment 10•15 years ago
|
||
Ludovico, thanks a lot for your patches. Your fix for PR_Cleanup is close but not quite right. Could you please test this patch? Since the primordial thread can be either a "system" or a "user" thread, we need to decrement the appropriate counter. Note that we have three implementations of PR_Cleanup. This patch fixes two of them. I didn't fix the third implementation (for BeOS only) because it doesn't seem to need the fix and I'm not sure if anyone is still maintaining that port.
Attachment #369453 -
Attachment is obsolete: true
Attachment #376149 -
Flags: review?(nelson)
Assignee | ||
Updated•15 years ago
|
Attachment #369458 -
Attachment description: Ludovico Cavedon's patch for log reinit, made to work with stdio too → Ludovico Cavedon's patch for log reinit, made to work with stdio too (checked in)
Attachment #369458 -
Attachment is obsolete: false
Attachment #369458 -
Flags: review+
Assignee | ||
Comment 11•15 years ago
|
||
Comment on attachment 369458 [details] [diff] [review] Ludovico Cavedon's patch for log reinit, made to work with stdio too (checked in) r=wtc. So PR_LogPrint may be called during NSPR initialization, before the logging subsystem is initialized, and PR_LogPrint tests 'logFile' to determine if the logging system is initialized. So it's important for _PR_LogCleanup to set 'logFile' to NULL. Nice fix! I checked in this patch on the NSPR trunk (NSPR 4.8). Checking in prlog.c; /cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v <-- prlog.c new revision: 3.48; previous revision: 3.47 done
Assignee | ||
Comment 12•15 years ago
|
||
I converted Ludovico's testcase to a test program.
Attachment #376158 -
Flags: review?(nelson)
Assignee | ||
Comment 13•15 years ago
|
||
Comment on attachment 376149 [details] [diff] [review] Fix for PR_Cleanup (checked in) Nelson, here are two bonsai links to help you review this patch. The code I added to PR_Cleanup is intended to undo the effect of the following code in _PR_InitThreads, which is called during NSPR initialization: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/threads/combined/pruthr.c&rev=3.37&mark=129-136#126 http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/pthreads/ptthread.c&rev=3.83&mark=917-927#915
Comment 14•15 years ago
|
||
Comment on attachment 376149 [details] [diff] [review] Fix for PR_Cleanup (checked in) r=nelson. Nit: In some places, this patch uses -- to decretment, and in others it uses -= 1. Seems inconsistent.
Attachment #376149 -
Flags: review?(nelson) → review+
Comment 15•15 years ago
|
||
Comment on attachment 376158 [details] [diff] [review] Add a test program This test program always reports success (returns 0) even when it fails. I think it should return a non-zero value if either PR_Cleanup calls fails.
Attachment #376158 -
Flags: review?(nelson) → review-
Assignee | ||
Comment 16•15 years ago
|
||
Comment on attachment 376149 [details] [diff] [review] Fix for PR_Cleanup (checked in) The inconsistent use of -- and -= 1 is to match the style used in the respective file. My own style is --, but ptthread.c uses -= 1 (Alan Freier's code). I checked in the patch on the NSPR trunk (NSPR 4.8). Checking in misc/prinit.c; /cvsroot/mozilla/nsprpub/pr/src/misc/prinit.c,v <-- prinit.c new revision: 3.52; previous revision: 3.51 done Checking in pthreads/ptthread.c; /cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v <-- ptthread.c new revision: 3.84; previous revision: 3.83 done
Attachment #376149 -
Attachment description: Fix for PR_Cleanup → Fix for PR_Cleanup (checked in)
Assignee | ||
Comment 17•15 years ago
|
||
The test program now returns 1 if PR_Cleanup fails. I also changed the arguments it passes to PR_Init to the recommended values: https://developer.mozilla.org/en/PR_Init
Attachment #376158 -
Attachment is obsolete: true
Attachment #376356 -
Flags: review?(nelson)
Updated•15 years ago
|
Attachment #376356 -
Flags: review?(nelson) → review+
Comment 18•15 years ago
|
||
Comment on attachment 376356 [details] [diff] [review] Add a test program v2 (checked in) r=nelson
Assignee | ||
Comment 19•15 years ago
|
||
Comment on attachment 376356 [details] [diff] [review] Add a test program v2 (checked in) I checked in the patch on the NSPR trunk (NSPR 4.8). Checking in Makefile.in; /cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v <-- Makefile.in new revision: 1.60; previous revision: 1.59 done RCS file: /cvsroot/mozilla/nsprpub/pr/tests/reinit.c,v done Checking in reinit.c; /cvsroot/mozilla/nsprpub/pr/tests/reinit.c,v <-- reinit.c initial revision: 1.1 done Checking in runtests.pl; /cvsroot/mozilla/nsprpub/pr/tests/runtests.pl,v <-- runtests.pl new revision: 1.5; previous revision: 1.4 done Checking in runtests.sh; /cvsroot/mozilla/nsprpub/pr/tests/runtests.sh,v <-- runtests.sh new revision: 1.8; previous revision: 1.7 done
Attachment #376356 -
Attachment description: Add a test program v2 → Add a test program v2 (checked in)
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
OS: Linux → All
Resolution: --- → FIXED
Target Milestone: --- → 4.8
Reporter | ||
Comment 20•15 years ago
|
||
I tested it and indeed fixes my problem! Thank you all for the support!
Assignee | ||
Updated•15 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•