+++ This bug was initially created as a clone of Bug #767240 +++ This patch adds types such as ScopedSECKEYPrivateKey, ScopedCERTCertificate, etc. to a header in PSM that act like smart pointers for commonly-used NSS types. The advantages over NSSCleanupAutoPtrClass are: 1. These are more consistent with the other Scoped* types in Gecko. 2. These are defined in a single header, whereas the currently-used pattern for NSSCleanupAutoPtrClass has us declare the same Cleanup types over and over again in each .cpp file. 3. ScopedNSSTypes.h is exported so code outside of PSM can use it. 4. NSSCleanupAutoPtrClass requires us to write code like this: #include "nsNSSCleaner.h" NSSCleanupAutoPtrClass(Type, TYPE_DestroyType) Type * pointer; TypeCleaner pointerCleaner(pointer); pointer = TYPE_CreateType(....); whereas with this header, we can write just: #include "ScopedNSSTypes.h" ScopedType pointer(TYPE_CreateType(....)); Assuming this gets r+d, I will file a follow-up bug for converting the current uses of NSSCleanupAutoPtrClass to Scoped*.

Note that these types are exported from PSM so that other code (in particular, the WebRTC code and BrowserID code) can use them. I did not change the void* use of NSSCleanupAutoPtr and I did not change any uses of NSSCleanupAutoPtr_WithParam. I would like to do that in another bug.

The SSLtunnel code has a couple hand-rolled wrappers like these: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/ssltunnel/ssltunnel.cpp#207 Should be trivial to replace those with these new classes. Very nice!

Here is the try run. Looks good: https://tbpl.mozilla.org/?tree=Try&rev=3dfe39c5d680

Comment on attachment 665220 [details] [diff] [review] Part 2: Replace almost all uses of NSSCleanupAutoPtr Review of attachment 665220 [details] [diff] [review]: ----------------------------------------------------------------- r- for serious omissions. ::: security/manager/ssl/src/nsCrypto.cpp @@ +705,5 @@ > NS_ASSERTION(intSlot,"Couldn't get the internal slot"); > > if (!PK11_DoesMechanism(intSlot, mechanism)) { > // Set to null, and the subsequent code will not attempt to use it. > PK11_FreeSlot(intSlot); Update this code to just |intSlot = nullptr;| otherwise you have double free. @@ +830,5 @@ > > //If we generated the key pair on the internal slot because the > // keys were going to be escrowed, move the keys over right now. > if (mustMoveKey) { > + ScopedSECKEYPrivateKey newPrivKey(PK11_LoadPrivKey(slot, trailing white space @@ -848,5 @@ > - // after the requests are made. This call only gives up > - // our reference to the key object and does not actually > - // physically remove it from the card itself. > - // The actual delete calls are being made in the destructors > - // of the cleaner helper instances. For curiosity, why are you removing this comment? ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ +1635,5 @@ > nickname = tmp; > PR_smprintf_free(tmp); > } > > + ScopedCERTCertificate dummycert; At [1] dummycert is being destroyed. You have to just assign dummycert = nullptr with your new code or you have a double deref. [1] http://hg.mozilla.org/mozilla-central/annotate/85df971e0db1/security/manager/ssl/src/nsNSSCertificateDB.cpp#l1674 @@ +1695,2 @@ > CERTCertDBHandle *certdb = CERT_GetDefaultCertDB(); > + ScopedCERTCertificate tmpCert(CERT_FindCertByDERCert(certdb, &der)); Here similar issue at [2], just remove call to destroy function probably. [2] http://hg.mozilla.org/mozilla-central/annotate/85df971e0db1/security/manager/ssl/src/nsNSSCertificateDB.cpp#l1727 ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +1958,5 @@ > if (CERT_LIST_END(node, certList)) { > goto noCert; > } > > + ScopedCERTCertificate low_prio_nonrep_cert; Please check the logic at [3]. You may need to add forget() or change the pointer mishmash here. [3] http://hg.mozilla.org/mozilla-central/annotate/85df971e0db1/security/manager/ssl/src/nsNSSIOLayer.cpp#l2002

Comment on attachment 665219 [details] [diff] [review] Part 1: Create scoped pointer types for NSS types https://hg.mozilla.org/mozilla-central/rev/ddd4e4a5f337 Checked in so that WebRTC can start using these types.

Honza, thanks for your careful review. In order to ensure there are no more double-free problems, I removed all the CERT_Destroy* calls from the functions/files I modified.

Here is a try run that includes these changes amongst many others. Ignore the xpcshell test failures; they are intentional in this try run and unrelated to this bug.

Here is a try run that includes these changes amongst many others. Ignore the xpcshell test failures; they are intentional in this try run and unrelated to this bug: https://tbpl.mozilla.org/?tree=Try&rev=af50fdec15a6

Comment on attachment 683899 [details] [diff] [review] Part 2: Replace almost all uses of NSSCleanupAutoPtr and most manual uses of CERT_Destroy* ad friends Review of attachment 683899 [details] [diff] [review]: ----------------------------------------------------------------- Not that deep review as the last time (too time consuming). I checked the major issues has been fixed. r=honzab. ::: security/manager/ssl/src/nsNSSCallbacks.h @@ +16,5 @@ > #include "mozilla/Mutex.h" > #include "mozilla/Attributes.h" > +#include "nsString.h" > + > +class nsILoadGroup; Changes from a different bug?

https://hg.mozilla.org/integration/mozilla-inbound/rev/c966b16e4fb5 (In reply to Honza Bambas (:mayhemer) from comment #11) > ::: security/manager/ssl/src/nsNSSCallbacks.h > @@ +16,5 @@ > > #include "mozilla/Mutex.h" > > #include "mozilla/Attributes.h" > > +#include "nsString.h" > > + > > +class nsILoadGroup; > > Changes from a different bug? This is needed due to the removal of an #include from an included header file. Thanks for the review!

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ff6c02ef74 https://hg.mozilla.org/integration/mozilla-inbound/rev/ede38378e570

https://hg.mozilla.org/integration/mozilla-inbound/rev/57e047e64019 https://hg.mozilla.org/integration/mozilla-inbound/rev/a2da6577872b

https://hg.mozilla.org/mozilla-central/rev/57e047e64019 https://hg.mozilla.org/mozilla-central/rev/a2da6577872b