1247847 - Use smart pointers in nsNSSCertHelper.cpp to manage NSS resources

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

nsNSSCertHelper.cpp currently uses raw pointers, which are not as nice and easier to get wrong.

Comment 1

[:Cykesiopka]

Attachment: MozReview Request: Bug 1247847 - Use smart pointers in nsNSSCertHelper.cpp to manage NSS resources. r=keeler

This lets us remove things like gotos in the code, and makes resource ownership slightly clearer.

Review commit: https://reviewboard.mozilla.org/r/34721/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34721/

Comment 2

[:Cykesiopka]

https://reviewboard.mozilla.org/r/34721/#review31355

::: security/manager/ssl/nsNSSCertHelper.cpp:963
(Diff revision 1)
> -	if (SEC_ASN1DecodeItem(arena, &decoded, 
> +        if (SEC_ASN1DecodeItem(arena.get(), &decoded, 

Trailing whitespace.

::: security/manager/ssl/nsNSSCertHelper.cpp:1113
(Diff revision 1)
> -  if (SEC_QuickDERDecodeItem(arena, &decoded, 
> +  if (SEC_QuickDERDecodeItem(arena.get(), &decoded, 

Trailing whitespace.

Comment 3

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccc3b28a19de

I did brief testing with the cert viewer, and things seemed to work OK.
At the very least, this builds.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

As a heads-up, I probably won't get to this until Tuesday.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8718715 [details]
MozReview Request: Bug 1247847 - Use smart pointers in nsNSSCertHelper.cpp to manage NSS resources. r=keeler

https://reviewboard.mozilla.org/r/34721/#review31437

LGTM.

::: security/manager/ssl/ScopedNSSTypes.h:17
(Diff revision 1)
>  #ifndef MOZ_NO_MOZALLOC

For these, I've found it's useful to separate them out below the rest of the #includes (that way, selecting all other includes and sorting works without reordering the #ifdef/#endif)

::: security/manager/ssl/nsNSSCertHelper.cpp:1177
(Diff revision 1)
>    SECItem **itemList;

nit: ** placement if you want to address it here

::: security/manager/ssl/nsNSSCertHelper.cpp:1338
(Diff revision 1)
>    CRLDistributionPoint **points, *point;

nit: * again
Also, these should really be declared on separate lines.

Comment 6

[:Cykesiopka]

Comment on attachment 8718715 [details]
MozReview Request: Bug 1247847 - Use smart pointers in nsNSSCertHelper.cpp to manage NSS resources. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34721/diff/1-2/

Comment 7

[:Cykesiopka]

https://reviewboard.mozilla.org/r/34721/#review31437

Thanks for the review!

> For these, I've found it's useful to separate them out below the rest of the #includes (that way, selecting all other includes and sorting works without reordering the #ifdef/#endif)

Makes sense.

> nit: ** placement if you want to address it here

Done.

> nit: * again
> Also, these should really be declared on separate lines.

Done. There are still some of these remaining, but I will defer these and other cleanups to another time, just so the changes here don't spiral out of control.

Comment 8

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c34721b5f572

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/866193a135f8

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/866193a135f8