485318 - Failure when re-initializing the library

Status: VERIFIED FIXED

(NSPR :: NSPR, defect)

Description

[Ludovico Cavedon]

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

Comment 1

[Ludovico Cavedon]

Attachment: testcase

Comment 2

[Ludovico Cavedon]

Attachment: fix

pt_book.user was not decremented upon cleanup

Comment 3

[Ludovico Cavedon]

Attachment: fix for log reinit

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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Ludovico Cavedon's patch for log reinit, converted to CVS diff

I converted this to a CVS diff to facilitate review with Bonsai's patch
review tools.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Ludovico Cavedon's patch for log reinit, made to work with stdio too (checked in)

Ludovico, what do you think of this alternative?

Comment 6

[Ludovico Cavedon]

Yes, of course, you alternative is better!
Thanks!

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 8

[Wan-Teh Chang]

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.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 10

[Wan-Teh Chang]

Attachment: Fix for PR_Cleanup (checked in)

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.

Comment 11

[Wan-Teh Chang]

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

Comment 12

[Wan-Teh Chang]

Attachment: Add a test program

I converted Ludovico's testcase to a test program.

Comment 13

[Wan-Teh Chang]

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

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 16

[Wan-Teh Chang]

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

Comment 17

[Wan-Teh Chang]

Attachment: Add a test program v2 (checked in)

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

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 376356 [details] [diff] [review]
Add a test program v2 (checked in)

r=nelson

Comment 19

[Wan-Teh Chang]

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

Comment 20

[Ludovico Cavedon]

I tested it and indeed fixes my problem!
Thank you all for the support!