The module list lock serves two distinct purposes which, IMO, encourage reentry on the read lock:
* This lock protects the global module lists.
* it also protects changes to the slot array (module->slots) and slot count
* (module->slotCount) in each module.
This is a problem because the module list lock is implemented as an
NSSRW_Lock, which supports recursive write locking but not recursive read locking. Deadlock occurs if
1. thread A takes a read lock,
2. thread B queues to write, and then
3. thread A tries to take another read lock.
In this scenario, thread A will wait for the write lock to be released, even though the write lock is not held and thread A is itself preventing the write lock from being acquired.
Some Firefox shutdown hangs can be attributed to this pattern. In these cases,
nsNSSComponent::HasActiveSmartCards takes a module list read lock in order to iterate over the module list (purpose 1). In each iteration it calls
SECMOD_HasRemovableSlots which takes another read lock (pupose 2).
Ideally we would have separate locks for the global module list and the per-module slot lists, but this would involve changing the
Alternatively we can make it the caller's responsibility to acquire the module list read lock for functions like
SECMOD_HasRemovableSlots. I'll attach a patch which does this, but I'm open to suggestions.