403957 - nss_InitLock should be replaced with PZ_NewLock in softoken

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

Patch forthcoming.

Comment 1

[Julien Pierre]

Attachment: Patch for trunk

Comment 2

[Julien Pierre]

Attachment: Duplicate patch. Please ignore.

Comment 3

[Julien Pierre]

Attachment: Patch for branch

Comment 4

[Julien Pierre]

Comment on attachment 288891 [details] [diff] [review]
Duplicate patch. Please ignore.

Deleting this patch which got submitted twice.

Comment 5

[Wan-Teh Chang]

Comment on attachment 288892 [details] [diff] [review]
Patch for branch

Julien,

Only after browsing in LXR for 20 minutes can I convince
myself that certdb_InitDBLock and nsslowcert_InitLocks are
called during softoken initialization:

NSC_Initialize
nsc_CommonInitialize
SFTK_SlotInit
SFTK_SlotReInit
sftk_DBInit
sftkdbCall_open
legacy_Open (via legacy_glue_open)
nsslowcert_InitLocks

NSC_Initialize
nsc_CommonInitialize
SFTK_SlotInit
SFTK_SlotReInit
sftk_DBInit
sftkdbCall_open
legacy_Open (via legacy_glue_open)
lg_OpenCertDB
nsslowcert_OpenCertDB
certdb_InitDBLock

So this patch is correct, but I hope its correctness can be
established more easily.

In nsslowcert_InitLocks, should we destroy the locks
we have created if we can't create all three locks?

Comment 6

[Wan-Teh Chang]

Comment on attachment 288892 [details] [diff] [review]
Patch for branch

I just realized this patch is for the NSS_3_11_BRANCH.

I suggest that we delete the certdb_InitDBLock function
and have nsslowcert_InitLocks create dbLock.

Comment 7

[Wan-Teh Chang]

Comment on attachment 288890 [details] [diff] [review]
Patch for trunk

I'll let Bob review this patch.  Bob, why do we use a lock
(handle->lock) for each key db and a single lock (dbLock)
for all all cert db's?

Comment 8

[Julien Pierre]

Wan-Teh,

Sorry it was so difficult to figure out. Bug 273637 dealt with the issues of the softoken locks. The patch made them safe, but still used nss_InitLock . Reading that bug probably makes things clearer than browsing the source for 20 mins.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Julien, your two patches modify a different number of functions.
The branch patch does not modify function keydb_InitLocks.
Does it not exist on the branch? or is your branch patch missing a piece?

Comment 10

[Julien Pierre]

Attachment: Patch for branch v2. Also fix keydb_InitLocks

Ooops. It was missing a piece.

Comment 11

[Julien Pierre]

Attachment: Wrong patch file. Please ignore.

Comment 12

[Julien Pierre]

Attachment: Patch for branch v3. Also remove unnecessary include

Comment 13

[Julien Pierre]

Attachment: Patch for trunk v2 . Also remove unnecessary include . Attach the correct file this time. Sigh.

Comment 14

[Wan-Teh Chang]

Comment on attachment 288926 [details] [diff] [review]
Patch for branch v3. Also remove unnecessary include

r=wtc.

You can remove the keydb_InitLocks function and replace
the keydb_InitLocks(handle) call in nsslowkey_NewHandle
by

    handle->lock = PZ_NewLock(nssILockKeyDB);

Comment 15

[Wan-Teh Chang]

Comment on attachment 288928 [details] [diff] [review]
Patch for trunk v2 . Also remove unnecessary include . Attach the correct file this time. Sigh.

r=wtc.

Same here.  Using a lock per key db handle makes it easier
to see that the lock's creation doesn't need to be protected
by any "init/call once" method.

Comment 16

[Julien Pierre]

Nelson, Wan-Teh,

Thanks for the reviews. I checked the patches in to the trunk :

Checking in legacydb/keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/keydb.c,v  <--  keydb.c
new revision: 1.7; previous revision: 1.6
done
Checking in legacydb/pcertdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/pcertdb.c,v  <--  pcertdb.c
new revision: 1.6; previous revision: 1.5
done

And to the branch :

Checking in keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/Attic/keydb.c,v  <--  keydb.c
new revision: 1.40.2.7; previous revision: 1.40.2.6
done
Checking in pcertdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/Attic/pcertdb.c,v  <--  pcertdb.c
new revision: 1.53.2.11; previous revision: 1.53.2.10
done

Comment 17

[Wan-Teh Chang]

Attachment: Inline keydb_InitLocks and keydb_DestroyLocks

These two short functions are used only once.  The code is
easier to understand without them.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 289025 [details] [diff] [review]
Inline keydb_InitLocks and keydb_DestroyLocks

r=nelson 
(who wishes that patches would be attached only to (re)opened bugs)

Comment 19

[Wan-Teh Chang]

Comment on attachment 289025 [details] [diff] [review]
Inline keydb_InitLocks and keydb_DestroyLocks

I checked in this patch on the NSS trunk (NSS 3.12).

Checking in keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/keydb.c,v  <--  keydb.c
new revision: 1.8; previous revision: 1.7
done