957812 - Use NSSRWLock instead of PRRWLock in sslSessionID

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Patch

Both NSSRWLock and PRRWLock have debug code that enforces lock ranking.

For PRRWLock, this code is enabled if DEBUG is defined.

For NSSRWLock, this code is enabled if DEBUG_RANK_ORDER is defined.
Since DEBUG_RANK_ORDER is an uncommon macro, this code is effectively
never used with NSSRWLock.

I found that this code has several bugs (see NSPR bug 957458). So it
is better to use NSSRWLock. Also, NSSRWLock is more commonly used than
PRRWLock in NSS:

http://mxr.mozilla.org/nss/search?string=PRRWLock
http://mxr.mozilla.org/nss/ident?i=NSSRWLock

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8357423 [details] [diff] [review]
Patch

Review of attachment 8357423 [details] [diff] [review]:
-----------------------------------------------------------------

I originally used the NSPR lock because I had thought that the NSPR version was lighter-weight. I believe I thought that the NSS variant had additional re-entrance guarantees that made it at least slightly less efficient. But, it is hard to remember now.

Comment 2

[Wan-Teh Chang]

Brian: I think you are right. I forgot that NSSRWLock is re-entrant. NSSRWLock is
only slightly more expensive than PRRWLock, requiring a PR_GetCurrentThread() call
to acquire a lock. But I am worried that my patch will result in the confusion
that the |lock| member of sslSessionID can be locked recursively.

Although the lock rank checking code of PRRWLock has several bugs, we only hit
a minor one (the thread-specific lock stack is created unnecessarily) in debug
builds because this lock has no lock rank. So I am willing to abandon my patch.