Closed Bug 403957 Opened 13 years ago Closed 13 years ago

nss_InitLock should be replaced with PZ_NewLock in softoken

Categories

(NSS :: Libraries, defect, P2)

3.11
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.9

People

(Reporter: julien.pierre, Assigned: julien.pierre)

Details

Attachments

(3 files, 5 obsolete files)

Patch forthcoming.
Priority: -- → P2
Attached patch Patch for trunk (obsolete) — Splinter Review
Attachment #288890 - Flags: superreview?(wtc)
Attachment #288890 - Flags: review?(nelson)
Attached patch Duplicate patch. Please ignore. (obsolete) — Splinter Review
Attachment #288891 - Flags: superreview?
Attachment #288891 - Flags: review?(nelson)
Attached patch Patch for branch (obsolete) — Splinter Review
Attachment #288892 - Flags: superreview?(rrelyea)
Attachment #288892 - Flags: review?(nelson)
Comment on attachment 288891 [details] [diff] [review]
Duplicate patch. Please ignore.

Deleting this patch which got submitted twice.
Attachment #288891 - Attachment is obsolete: true
Attachment #288891 - Flags: superreview?
Attachment #288891 - Flags: review?(nelson)
Summary: nss_InitLock should be replaced with PZ_NewLock in many places → nss_InitLock should be replaced with PZ_NewLock in softoken
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?
Attachment #288892 - Flags: review+
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 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?
Attachment #288890 - Flags: superreview?(wtc) → superreview?(rrelyea)
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.
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?
Ooops. It was missing a piece.
Attachment #288892 - Attachment is obsolete: true
Attachment #288907 - Flags: superreview?(wtc)
Attachment #288907 - Flags: review?(nelson)
Attachment #288892 - Flags: superreview?(rrelyea)
Attachment #288892 - Flags: review?(nelson)
Attached patch Wrong patch file. Please ignore. (obsolete) — Splinter Review
Attachment #288890 - Attachment is obsolete: true
Attachment #288924 - Flags: superreview?(rrelyea)
Attachment #288924 - Flags: review?(nelson)
Attachment #288890 - Flags: superreview?(rrelyea)
Attachment #288890 - Flags: review?(nelson)
Attachment #288907 - Attachment is obsolete: true
Attachment #288926 - Flags: superreview?(rrelyea)
Attachment #288926 - Flags: review?(nelson)
Attachment #288907 - Flags: superreview?(wtc)
Attachment #288907 - Flags: review?(nelson)
Attachment #288924 - Attachment is obsolete: true
Attachment #288924 - Flags: superreview?(rrelyea)
Attachment #288924 - Flags: review?(nelson)
Attachment #288907 - Attachment description: Also fix keydb_InitLocks → Patch for branch v2. Also fix keydb_InitLocks
Attachment #288891 - Attachment description: Patch for branch → Duplicate patch. Please ignore.
Attachment #288924 - Attachment description: Patch for trunk v2 . Also remove unnecessary include → Wrong patch file. Please ignore.
Attachment #288924 - Flags: approval1.9?
Attachment #288924 - Flags: approval1.8.1.11?
Attachment #288924 - Flags: approval1.9?
Attachment #288924 - Flags: approval1.8.1.11?
Attachment #288926 - Flags: review?(nelson) → review+
Attachment #288928 - Flags: review?(nelson) → review+
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);
Attachment #288926 - Flags: review+
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.
Attachment #288928 - Flags: review+
Attachment #288926 - Flags: superreview?(rrelyea)
Attachment #288928 - Flags: superreview?(rrelyea)
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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
These two short functions are used only once.  The code is
easier to understand without them.
Attachment #289025 - Flags: review?(julien.pierre.boogz)
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)
Attachment #289025 - Flags: review+
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
Attachment #289025 - Flags: review?(julien.pierre.boogz)
You need to log in before you can comment on or make changes to this bug.