A patch to improve the responsiveness of SECMOD_WaitForAnyTokenEvent

RESOLVED FIXED in 3.9.6

Status

NSS
Libraries
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.95 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
SECMOD_WaitForAnyTokenEvent is a new function
added in NSS 3.9.3 to handle token insertion
and removal better (bug 252702).

This bug is a patch from Bob Relyea to improve
the responsiveness of that function.
(Assignee)

Comment 1

14 years ago
Created attachment 176326 [details] [diff] [review]
Bob's patch
Attachment #176326 - Flags: review?(nelson)
Comment on attachment 176326 [details] [diff] [review]
Bob's patch

I have a question about this patch's null pointer checking.
Is it doing the right amount?  Too little?  Too much?

The patch introduces code that follows a chain of 3 pointers,
slot, slot->nssToken, and then slot->nssToken->slot.
It checks the first two for NULL before using them, but not the
third one. 

If the first two pointers are permitted to be NULL legitimately,
but the third pointer may not legitimately be NULL if the first 
two are non-NULL, then this code is correct.  But if the third
pointer may legitimately be NULL, then it would see that an additional
test is needed.  I'm not aware of any document that tells us whether
the third pointer is or is not permitted ot be NULL.  So, I don't
know if this NULL pointer needs to be checked or not.

Any ideas?

This patch is fine, except possibly for this issue.
Comment on attachment 176326 [details] [diff] [review]
Bob's patch

r=nelson, with the following suggestions,

>+    /* if we are in the delay period for the "isPresent" call, reset
>+     * the delay since we know things have probably changed... */
>+    if (slot && slot->nssToken) {
>+	nssSlot_ResetDelay(slot->nssToken->slot);
>+    }

I would prefer the following test:

>+    if (slot && slot->nssToken && slot->nssToken->slot) {

or alternatively change nssSlot_ResetDelay to check its input for NULL.
Attachment #176326 - Flags: review?(nelson) → review+
(Assignee)

Comment 4

14 years ago
Created attachment 176643 [details] [diff] [review]
Bob's original patch

Nelson, thanks for the code review.  Your suggested
change is actually in Bob's original patch.  Therefore,
I decided to check in his original patch instead.

The reason I removed the slot->nssToken->slot test
is that by code inspection I concluded that that test
is not necessary, and I've also confirmed with Bob
that that test was defensive programming and wasn't
there to fix a crash he encountered.  But I think it
is more work for me to convince you and other code
reviewers why that test isn't necessary.  So I am
happy to go with defensive programming.
Attachment #176326 - Attachment is obsolete: true
(Assignee)

Comment 5

14 years ago
I checked in Bob's original patch on the tip (NSS 3.10)
and NSS_3_9_BRANCH (NSS 3.9.6).
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.9.6
You need to log in before you can comment on or make changes to this bug.