Closed Bug 1182959 Opened 5 years ago Closed 5 years ago

Use nsTHashTable::Iterator in security/

Categories

(Core :: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(5 files)

Because iterators are so much nicer than enumerate functions.

There are eight occurrences of EnumerateEntries() in security/ to be dealt with.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attachment #8635856 - Flags: review?(honzab.moz)
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); ?
Attachment #8635853 - Flags: review?(honzab.moz) → review-
Oh god... you split that to 5 patches...

Please submit one patch when you modify just a single file...
Attachment #8635854 - Flags: review?(honzab.moz)
Attachment #8635855 - Flags: review?(honzab.moz)
Attachment #8635856 - Flags: review?(honzab.moz)
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..
Attachment #8635853 - Flags: review- → review+
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?
Attachment #8635857 - Flags: review?(honzab.moz) → review+
Attachment #8635854 - Flags: review+
Attachment #8635855 - Flags: review+
Attachment #8635856 - Flags: 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

iter.Get() never returns nullptr, and the check was unnecessary with the old code as well.
You need to log in before you can comment on or make changes to this bug.