220357 - PSM : consistency in use of cleaners between nsNSSIOLayer.cpp and nsCertPicker.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Jean-Marc Desperrier]

As it creates the same kind of objects, nsNSSIOLayer.cpp should use for
consistency the same method as nsCertPicker.cpp to ensure they get properly
destroyed.

This means using the NSSCleanupAutoPtrClass macro around the elements that must
be freed.

Maybe adding a little comment to explain that "CERTCertNicknamesCleaner
cnc(nicknames);" ensure that nicknames will be freed without having to
explicitly free would be a good think. 
I oversighted it at first, and only undestood after wondering why
CERT_DestroyCertList wasn't explicitly called in nsCertPicker.cpp.

Isn't there a simple way to change the macro so that you don't have to do :
CERTCertNicknames *nicknames = (getting new element ptr)
CERTCertNicknamesCleaner cnc(nicknames);
 but simply 
CERTCertNicknamesAutoCleanPtr nicknames = (getting new element ptr)
 ?
The "*" operator could be redefined to keep the pointer semantic.

Also I think the exact same kind of code gets reused a lot, the patch for
SignText has also very similar code to get a valid cert list, I wonder if it's
not possible to define some functions to reduce the size.

This is perhaps quite cosmetic, but I'm convinced nice looking, coherent code
has less bugs, and on the long term, you end up spending more time correcting
the bugs coming form inconcistencies than taking the time to write it clean first.

Comment 1

[Jean-Marc Desperrier]

This bug was never closed, but the current nsNSSIOLayer.cpp code properly uses NSSCleanupAutoPtrClass where useful.