59 bytes, text/x-review-board-request
nsIX509Cert::GetCert returns a CERTCertificate whose reference count has already been increased. When IsCertificateDistrustImminent calls CertDNIsInList(rootCert->GetCert(), RootSymantecDNs) and CertDNIsInList(aCert->GetCert(), RootAppleAndGoogleDNs), the reference count on those certificates never gets a corresponding decrement, so we keep those certificates alive until shut down. This can be demonstrated by setting MOZ_LOG="pipnss:4", running Firefox, connecting to any host, and then shutting down. "[Main Thread]: E/pipnss NSS SHUTDOWN FAILURE" will be in the console output.
[Tracking Requested - why for this release]: this could potentially keep more certificates alive in memory than before (note that all certificates are ref-counted/de-duplicated, so the excess memory would probably only be on the order of how many distinct domains are visited per session) (and it may be that things like history, the network cache, etc., keep these certificate alive anyway, in which case this wouldn't have a noticeable impact).
Comment on attachment 8935559 [details] bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent https://reviewboard.mozilla.org/r/206434/#review212108
Attachment #8935559 - Flags: review?(jjones) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/89457644e922 add owning handles so cert references don't leak in IsCertificateDistrustImminent r=jcj
Please request Beta approval on this when you get a chance.
Comment on attachment 8935559 [details] bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent Approval Request Comment [Feature/Bug causing the regression]: bug 1409259 [User impact if declined]: potentially unnecessary extra memory usage [Is this code covered by automated tests?]: the feature added in bug 1409259 has tests, but not the resource leak [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: yes - steps to verify in comment 0 [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: very low risk [Why is the change risky/not risky?]: the fix involves adding a smart pointer to keep track of a resource - it's unlikely this will cause any problems [String changes made/needed]: none
Attachment #8935559 - Flags: approval-mozilla-beta?
Comment on attachment 8935559 [details] bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent Memory leak that regressed in 58. Let's uplift for 58.0b12.
Attachment #8935559 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Andrei, another issue good to verify in beta once this lands.
I have managed to reproduce this issue using an affected 58 Beta 10 build. The following failure "[Main Thread]: E/pipnss NSS SHUTDOWN FAILURE" was displayed in the logs/console output after closing Firefox. This is verified fixed on 58.0b12 (20171218174357), 59.0a1 (2017-12-19) across platforms: Win 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64. I can no longer see failures related to pipnss, "[Main Thread]: D/pipnss NSS shutdown =====>> OK <<=====".
You need to log in before you can comment on or make changes to this bug.