1335576 - clean up nsNSSCertHelper.cpp a bit to enable other improvements

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

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

nsNSSCertHelper.cpp could use some improvements:
* style-wise, it's all over the place
* it needlessly passes around a pointer to nsINSSComponent in more or less every function (needless because what it really wants is the pipnss string bundle, which doesn't need to be part of nsINSSComponent anyway, particularly for main-thread-only uses, which this happens to be (the vast majority of this file is just to implement nsIX509Cert.ASN1Structure)).

Doing this will simplify some improvements to nsINSSComponent I have planned.

Note that this bug is not intended to address all current issues with nsNSSCertHelper.cpp. For example, in many cases return values are not checked when they should be. This patch will not change any behavior in this regard.

Comment 1

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

Attachment: bug 1335576 - run clang-format on nsNSSCertHelper.cpp

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

Comment 2

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

Attachment: bug 1335576 - stop passing nsINSSComponent around everywhere in nsNSSCertHelper.cpp

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

Comment 3

[:Cykesiopka]

Comment on attachment 8832258 [details]
bug 1335576 - run clang-format on nsNSSCertHelper.cpp

https://reviewboard.mozilla.org/r/108594/#review110634

Comment 4

[:Cykesiopka]

Comment on attachment 8832259 [details]
bug 1335576 - stop passing nsINSSComponent around everywhere in nsNSSCertHelper.cpp

https://reviewboard.mozilla.org/r/108596/#review110636

Much nicer.

::: security/manager/ssl/nsNSSCertHelper.cpp:22
(Diff revision 1)
>  #include "nsNSSCertTrust.h"
>  #include "nsNSSCertValidity.h"
>  #include "nsNSSCertificate.h"
> -#include "nsNSSComponent.h"
>  #include "nsServiceManagerUtils.h"
> +#include "nsString.h"

Nit: Looks like the header file already includes `nsString.h`.

::: security/manager/ssl/nsNSSCertHelper.cpp:81
(Diff revision 1)
>  };
>  
>  static const unsigned int numOids = (sizeof more_oids) / (sizeof more_oids[0]);
>  
>  static nsresult
> -ProcessVersion(SECItem* versionItem,
> +GetPIPNSSBundle(nsIStringBundle** pipnssBundle)

We should include nsIStringBundle.h for this.

::: security/manager/ssl/nsNSSCertHelper.cpp:83
(Diff revision 1)
>  static const unsigned int numOids = (sizeof more_oids) / (sizeof more_oids[0]);
>  
>  static nsresult
> -ProcessVersion(SECItem* versionItem,
> -               nsINSSComponent* nssComponent,
> -               nsIASN1PrintableItem** retItem)
> +GetPIPNSSBundle(nsIStringBundle** pipnssBundle)
> +{
> +  nsCOMPtr<nsIStringBundleService> bundleService(

We should include nsIStringBundleService.h for this.

::: security/manager/ssl/nsNSSCertHelper.cpp:107
(Diff revision 1)
> +    return rv;
> +  }
> +  result.Truncate();
> +  nsXPIDLString xpidlString;
> +  rv = pipnssBundle->GetStringFromName(NS_ConvertASCIItoUTF16(stringName).get(),
> +                                       getter_Copies(xpidlString));

Nit: It looks like we can actually just pass `result` directly here and below, and simplify some of the code.

::: security/manager/ssl/nsNSSCertHelper.cpp:1439
(Diff revision 1)
> -        nssComponent->GetPIPNSSBundleString("CertDumpAffiliationChanged",
> +        GetPIPNSSBundleString("CertDumpAffiliationChanged",
>                                              local);

Nit: Indentation here is off.

Comment 5

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

Comment on attachment 8832259 [details]
bug 1335576 - stop passing nsINSSComponent around everywhere in nsNSSCertHelper.cpp

https://reviewboard.mozilla.org/r/108596/#review110636

Thanks! (I realize this probably wasn't the most fun review...)

> Nit: Looks like the header file already includes `nsString.h`.

Ah - fixed.

> We should include nsIStringBundle.h for this.

Added.

> We should include nsIStringBundleService.h for this.

Looks like nsIStringBundleService is just defined in nsIStringBundle.idl: https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/intl/strres/nsIStringBundle.idl#53

> Nit: It looks like we can actually just pass `result` directly here and below, and simplify some of the code.

Good call.

> Nit: Indentation here is off.

Good catch.

Comment 6

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

Comment on attachment 8832258 [details]
bug 1335576 - run clang-format on nsNSSCertHelper.cpp

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

Comment 7

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

Comment on attachment 8832259 [details]
bug 1335576 - stop passing nsINSSComponent around everywhere in nsNSSCertHelper.cpp

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

Comment 8

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

Here's try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa234323fe15
I believe the failures are all due to bug 1336654.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b15133c98cc
run clang-format on nsNSSCertHelper.cpp r=Cykesiopka
https://hg.mozilla.org/integration/autoland/rev/19cd77acf2e0
stop passing nsINSSComponent around everywhere in nsNSSCertHelper.cpp r=Cykesiopka

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2b15133c98cc
https://hg.mozilla.org/mozilla-central/rev/19cd77acf2e0