Remove PL_DHashTableEnumerate() uses from nsNSSShutdown

RESOLVED FIXED in Firefox 42

Status

()

Core
Security: PSM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Because PLDHashTable::Iterator is much nicer than PL_DHashTableEnumerate().
(Assignee)

Comment 1

3 years ago
Created attachment 8624040 [details] [diff] [review]
Remove PL_DHashTableEnumerate() uses from nsNSSShutdown
Attachment #8624040 - Flags: review?(honzab.moz)
(Assignee)

Updated

3 years ago
Blocks: 1128407
Comment on attachment 8624040 [details] [diff] [review]
Remove PL_DHashTableEnumerate() uses from nsNSSShutdown

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

r=honzab if the comment are non-issue

::: security/manager/ssl/nsNSSShutDown.cpp
@@ +139,5 @@
> +      reinterpret_cast<nsOnPK11LogoutCancelObject*>(entry->obj);
> +    if (pklco) {
> +      pklco->logout();
> +    }
> +  }

I liked the blank lines in doPK11LogoutHelper, please keep them

@@ +185,5 @@
> +    {
> +      MutexAutoUnlock unlock(singleton->mListLock);
> +      entry->obj->shutdown(nsNSSShutDownObject::calledFromList);
> +    }
> +    iter.Remove();

should this be called under the lock as well or is the table thread safe?  but the previous code behaved the same way... so it's probably OK.  Or is there a bug here?

The "because other threads might be calling us" scares me a bit..  I don't see enough protection of the table (unless the iterator's Remove() handles threading..)
Attachment #8624040 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 3

3 years ago
> should this be called under the lock as well or is the table thread safe? 

The iter.Remove() is called within the scope of |lock|.

PLDHashTable is not thread-safe.

> but the previous code behaved the same way... so it's probably OK.  Or is
> there a bug here?
> 
> The "because other threads might be calling us" scares me a bit..  I don't
> see enough protection of the table (unless the iterator's Remove() handles
> threading..)

I don't understand the interaction between the two locks (singleton->mListLock and mListLock) here, nor why the former needs to be unlocked temporarily. I also found the comment and indeed the whole only-do-one-element-per-iterator approach quite odd.

In the absence of such insights, I chose to preserve the existing behaviour.
(In reply to Nicholas Nethercote [:njn] from comment #3)
> > should this be called under the lock as well or is the table thread safe? 
> 
> The iter.Remove() is called within the scope of |lock|.
> 
> PLDHashTable is not thread-safe.

It's "MutexAutoUnlock unlock(singleton->mListLock);" when the object shutdown() is called!  Good!

> 
> > but the previous code behaved the same way... so it's probably OK.  Or is
> > there a bug here?
> > 
> > The "because other threads might be calling us" scares me a bit..  I don't
> > see enough protection of the table (unless the iterator's Remove() handles
> > threading..)
> 
> I don't understand the interaction between the two locks
> (singleton->mListLock and mListLock) here, nor why the former needs to be
> unlocked temporarily. I also found the comment and indeed the whole
> only-do-one-element-per-iterator approach quite odd.
> 
> In the absence of such insights, I chose to preserve the existing behaviour.

Yep, please do.  I think it's absolutely alright.

Thanks.
https://hg.mozilla.org/mozilla-central/rev/0136c229704a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41

Updated

3 years ago
Target Milestone: mozilla41 → mozilla42
You need to log in before you can comment on or make changes to this bug.