1182959 - Use nsTHashTable::Iterator in security/

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Nicholas Nethercote [inactive]]

Because iterators are so much nicer than enumerate functions.

There are eight occurrences of EnumerateEntries() in security/ to be dealt with.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Use nsTHashtable::Iterator in CertBlockList

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use nsTHashtable::Iterator in CertBlockList

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use nsTHashtable::Iterator in CertBlockList

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove BlocklistSaveInfo

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Use nsTHashtable::Iterator in nsCertOverrideService

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8635853 [details] [diff] [review]
(part 1) - Use nsTHashtable::Iterator in CertBlockList

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

::: security/manager/ssl/CertBlocklist.cpp
@@ +439,3 @@
>  // Write issuer data to the output stream
>  PLDHashOperator
>  WriteIssuer(nsCStringHashKey* aHashKey, void* aUserArg)

and what about saveInfo.issuers.EnumerateEntries(WriteIssuer, &saveInfo); ?

Comment 7

[Honza Bambas (:mayhemer)]

Oh god... you split that to 5 patches...

Please submit one patch when you modify just a single file...

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8635853 [details] [diff] [review]
(part 1) - Use nsTHashtable::Iterator in CertBlockList

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

OK, I'll walk the split patches, but it's pretty confusing..

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8635857 [details] [diff] [review]
(part 5) - Use nsTHashtable::Iterator in nsCertOverrideService

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

::: security/manager/ssl/nsCertOverrideService.cpp
@@ +168,5 @@
>  {
> +  ReentrantMonitorAutoEnter lock(monitor);
> +  for (auto iter = mSettingsTable.Iter(); !iter.Done(); iter.Next()) {
> +    nsCertOverrideEntry *entry = iter.Get();
> +    if (entry->mSettings.mIsTemporary) {

I think you still need to check !entry ?  or there always is a entry?  maybe the check in the old code is just a precaution that never failed

@@ +589,5 @@
>  {
>    ReentrantMonitorAutoEnter lock(monitor);
>    uint32_t overrideCount = 0;
> +  for (auto iter = mSettingsTable.Iter(); !iter.Done(); iter.Next()) {
> +    if (!iter.Get()->mSettings.mIsTemporary) {

same here?

Comment 10

[Nicholas Nethercote [inactive]]

> ::: security/manager/ssl/nsCertOverrideService.cpp
> @@ +168,5 @@
> >  {
> > +  ReentrantMonitorAutoEnter lock(monitor);
> > +  for (auto iter = mSettingsTable.Iter(); !iter.Done(); iter.Next()) {
> > +    nsCertOverrideEntry *entry = iter.Get();
> > +    if (entry->mSettings.mIsTemporary) {
> 
> I think you still need to check !entry ?  or there always is a entry?  maybe
> the check in the old code is just a precaution that never failed

iter.Get() never returns nullptr, and the check was unnecessary with the old code as well.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9c46f6bd26
https://hg.mozilla.org/integration/mozilla-inbound/rev/c05735a106ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/06466d6c7a6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a6c0871edf7
https://hg.mozilla.org/integration/mozilla-inbound/rev/30f211278f31

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e9c46f6bd26
https://hg.mozilla.org/mozilla-central/rev/c05735a106ce
https://hg.mozilla.org/mozilla-central/rev/06466d6c7a6f
https://hg.mozilla.org/mozilla-central/rev/4a6c0871edf7
https://hg.mozilla.org/mozilla-central/rev/30f211278f31