1122973 - Stop using macros for NSS module factory constructors

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: templatize_nss_ctors

https://treeherder.mozilla.org/#/jobs?repo=try&revision=602a16361574
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea8f377a8e34

Comment 1

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

Comment on attachment 8550787 [details] [diff] [review]
templatize_nss_ctors

Review of attachment 8550787 [details] [diff] [review]:
-----------------------------------------------------------------

In general, this looks good. It's definitely a lot clearer and cleaner than those macros. However, let's update the style as well. Two spaces for indentation, */& in type names go on the left (e.g. void** aResult, not void **aResult), braces always go around conditional bodies, with the opening brace on the same line, and keep to < 80 characters when possible. r- for now.

::: security/manager/ssl/src/nsNSSModule.cpp
@@ +49,4 @@
>  
> +MOZ_ALWAYS_INLINE static bool IsProcessDefault()
> +{
> +    return GeckoProcessType_Default == XRE_GetProcessType();

This isn't in the style guide, but generally it's more common to switch the operands here (see the implementation of EnsureNSSInitializedChromeOrContent for example, which incidentally you could change to call this function instead).

@@ +53,5 @@
>  }
>  
> +template<class InstanceClass>
> +MOZ_ALWAYS_INLINE static nsresult
> +Instantiate(REFNSIID aIID, void **aResult)

nit: void** aResult

@@ +56,5 @@
> +MOZ_ALWAYS_INLINE static nsresult
> +Instantiate(REFNSIID aIID, void **aResult)
> +{
> +    InstanceClass* inst;
> +    inst = new InstanceClass();

We may as well have InstanceClass* inst = new InstanceClass(); all on one line.

@@ +65,4 @@
>  }
>  
> +template<class InstanceClass,
> +         nsresult (InstanceClass::*InitMethod)()>

Can we use references to class functions instead of pointers to class functions?

@@ +66,5 @@
>  
> +template<class InstanceClass,
> +         nsresult (InstanceClass::*InitMethod)()>
> +MOZ_ALWAYS_INLINE static nsresult
> +Instantiate(REFNSIID aIID, void **aResult)

nit: void** aResult

@@ +69,5 @@
> +MOZ_ALWAYS_INLINE static nsresult
> +Instantiate(REFNSIID aIID, void **aResult)
> +{
> +    InstanceClass* inst;
> +    inst = new InstanceClass();

Same here - let's have these two statements all on one line.

@@ +70,5 @@
> +Instantiate(REFNSIID aIID, void **aResult)
> +{
> +    InstanceClass* inst;
> +    inst = new InstanceClass();
> +    NS_ADDREF(inst);

Same here with nsCOMPtr instead of manual refcounting, if possible.

@@ +72,5 @@
> +    InstanceClass* inst;
> +    inst = new InstanceClass();
> +    NS_ADDREF(inst);
> +    nsresult rv = (inst->*InitMethod)();
> +    if(NS_SUCCEEDED(rv)) {

nit: if (NS_SUCCEEDED...
Also, since it's possible to not take this branch, let's set *aResult = nullptr in case it doesn't get taken.

@@ +83,5 @@
> +template<EnsureNSSOperator ensureOperator,
> +         class InstanceClassChrome,
> +         class InstanceClassContent>
> +static nsresult
> +Constructor(nsISupports *aOuter, REFNSIID aIID, void **aResult)

nit: nsISupports* aOuter, void** aResult

@@ +93,5 @@
> +        rv = NS_ERROR_NO_AGGREGATION;
> +        return rv;
> +    }
> +
> +    if (!EnsureNSSInitialized(ensureOperator))

nit: braces around conditional bodies

@@ +96,5 @@
> +
> +    if (!EnsureNSSInitialized(ensureOperator))
> +        return NS_ERROR_FAILURE;
> +
> +    if (IsProcessDefault())

same

@@ +102,5 @@
> +    else
> +        rv = Instantiate<InstanceClassContent>(aIID, aResult);
> +
> +    if (ensureOperator == nssLoadingComponent)
> +    {

nit: brace at the end of the previous line

@@ +103,5 @@
> +        rv = Instantiate<InstanceClassContent>(aIID, aResult);
> +
> +    if (ensureOperator == nssLoadingComponent)
> +    {
> +        if (NS_SUCCEEDED(rv))

nit: braces around bodies

@@ +141,5 @@
> +{
> +    nsresult rv;
> +
> +    *aResult = nullptr;
> +    if (nullptr != aOuter) {

Again, let's switch these operands.

@@ +143,5 @@
> +
> +    *aResult = nullptr;
> +    if (nullptr != aOuter) {
> +        rv = NS_ERROR_NO_AGGREGATION;
> +        return rv;

Let's simplify this to 'return NS_ERROR_NO_AGGREGATION' and move the declaration of rv closer to where it's used.

@@ +232,3 @@
>  static const mozilla::Module::CIDEntry kNSSCIDs[] = {
> +  { &kNS_NSSCOMPONENT_CID, false, nullptr, Constructor<nssLoadingComponent, nsNSSComponent,
> +                                                       &nsNSSComponent::Init> },

Let's break these lines so they're < 80 characters.

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Use templates instead of macros for NSS module favtory construntors, v2

Fixed style nits.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #1)
> @@ +65,4 @@
> >  }
> >  
> > +template<class InstanceClass,
> > +         nsresult (InstanceClass::*InitMethod)()>
> 
> Can we use references to class functions instead of pointers to class
> functions?

Looks like we can't.
http://stackoverflow.com/questions/21952386/why-doesnt-reference-to-member-exist-in-c

> @@ +70,5 @@
> > +Instantiate(REFNSIID aIID, void **aResult)
> > +{
> > +    InstanceClass* inst;
> > +    inst = new InstanceClass();
> > +    NS_ADDREF(inst);
> 
> Same here with nsCOMPtr instead of manual refcounting, if possible.

nsCOMPtr didn't work because of the ambiguous conversion to nsISupports*. I used nsRefPtr instead.

> @@ +72,5 @@
> > +    InstanceClass* inst;
> > +    inst = new InstanceClass();
> > +    NS_ADDREF(inst);
> > +    nsresult rv = (inst->*InitMethod)();
> > +    if(NS_SUCCEEDED(rv)) {
> 
> nit: if (NS_SUCCEEDED...
> Also, since it's possible to not take this branch, let's set *aResult =
> nullptr in case it doesn't get taken.

Instantiate() is only called from Constructor() and Constructor() sets aResult to nullptr at the top of the function.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Use templates instead of macros for NSS module favtory construntors, v3

Resolved conflicts with bug 1114882.

Comment 4

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

Comment on attachment 8553779 [details] [diff] [review]
Use templates instead of macros for NSS module favtory construntors, v3

Review of attachment 8553779 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. However, I have some suggestions for further refactoring that will make it much less likely we'll do something wrong in the future (see in particular the third comment). r- for now.

::: security/manager/ssl/src/nsNSSModule.cpp
@@ +55,5 @@
> +template<class InstanceClass>
> +MOZ_ALWAYS_INLINE static nsresult
> +Instantiate(REFNSIID aIID, void** aResult)
> +{
> +  nsRefPtr<InstanceClass> inst = new InstanceClass();

Actually, this was a poor suggestion on my part. The underlying code for QueryInterface (as far as I can tell) uses NS_ADDREF / NS_RELEASE, so we should stick to that to be sure that this refcounting is compatible. (And, indeed, when I compile with this patch there are errors relating to this.)

@@ +62,4 @@
>  }
>  
> +template<class InstanceClass,
> +         nsresult (InstanceClass::*InitMethod)()>

Looks like these two lines will fit on one line.

@@ +136,5 @@
> +         class InstanceClassContent,
> +         nsresult (InstanceClassChrome::*InitMethodChrome)(),
> +         nsresult (InstanceClassContent::*InitMethodContent)()>
> +static nsresult
> +Constructor(nsISupports* aOuter, REFNSIID aIID, void** aResult)

I didn't realize there were two very similar Constructor functions as a result of sometimes having an init method and sometimes not. The code has already diverged unintentionally. I think having two of these functions could be a source of bugs in the future. We should define the constructor that doesn't take init methods in terms of the other one, like so:

return Constructor<ensureOperator, InstanceClassChrome, InstanceClassContent, nullptr, nullptr>(aOuter, aIID, aResult);

If we also only have one Instantiate method (and define the other one as 'return Instantiate<InstanceClass, nullptr>;'), it actually makes the code quite clean (all we have to do is something like 'if (InitMethod) { ... }' in Instantiate).

Make sure that the code for handling ensureOperator == nssEnsureChromeOrContent is in the implementation of Constructor.

@@ +226,5 @@
> +// Use the special factory constructor for everything this module implements,
> +// because all code could potentially require the NSS library.
> +// Our factory constructor takes an optional EnsureNSSOperator template parameter.
> +// Only for the nsNSSComponent, set this to nssLoadingComponent.
> +// For classes available from a content process, set this to nssEnsureOnChromeOnly.

A couple of these lines are >80 chars.

@@ +260,5 @@
> +  { &kNS_CRYPTO_HMAC_CID, false, nullptr, Constructor<nsCryptoHMAC> },
> +  { &kNS_CERT_PICKER_CID, false, nullptr, Constructor<nsCertPicker> },
> +  { &kNS_NTLMAUTHMODULE_CID, false, nullptr,
> +    Constructor<nssEnsure, nsNTLMAuthModule, &nsNTLMAuthModule::InitTest> },
> +  { &kNS_STREAMCIPHER_CID, false, nullptr, Constructor<nsStreamCipher> },

nsStreamCipher was removed recently, by the way.

Comment 5

[Masatoshi Kimura [:emk]]

I couldn't use nullptr for member function pointer template argument because of a MSVC 2013 bug :(
https://connect.microsoft.com/VisualStudio/feedback/details/807614/vc-2013-doesnt-accept-nullptr-as-default-value-for-member-function-pointer-template-argument

Comment 6

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

Since bug 1186064 has been fixed, I think we can move forward with this. Any interest in doing so?

Comment 7

[Masatoshi Kimura [:emk]]

Sure.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Bug 1122973 - Use templates instead of macros for NSS module factory constructors.

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

Comment 9

[Masatoshi Kimura [:emk]]

> z:/build/build/src/security/manager/ssl/nsNSSModule.cpp(227): error C2440: 'initializing':
> cannot convert from 'overloaded-function' to 'mozilla::Module::ConstructorProcPtr'

:(

Build does not fail locally. Are compilers on automation too old?

Comment 10

[Masatoshi Kimura [:emk]]

Ah, Win32 calling convention bites me again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=463853b36cf247ba8c26f1f0577605a8399b7bc2

Comment 11

[Masatoshi Kimura [:emk]]

Comment on attachment 8831405 [details]
Bug 1122973 - Use templates instead of macros for NSS module factory constructors.

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

Comment 12

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

Comment on attachment 8831405 [details]
Bug 1122973 - Use templates instead of macros for NSS module factory constructors.

https://reviewboard.mozilla.org/r/107982/#review109410

LGTM with comments addressed, but I think we should also get another set of eyes on this.

::: security/manager/ssl/nsNSSModule.cpp:58
(Diff revision 2)
> -                                        Init)
> +MOZ_ALWAYS_INLINE static nsresult
> +Instantiate(REFNSIID aIID, void** aResult)
> +{
> +  InstanceClass* inst = new InstanceClass();
> +  NS_ADDREF(inst);
> +  nsresult rv = InitMethod != nullptr ? (inst->*InitMethod)() : NS_OK;

nit: no need for comparison to nullptr (as in, we can just say `InitMethod ? (inst->*InitMethod)() : NS_OK`)

::: security/manager/ssl/nsNSSModule.cpp:72
(Diff revision 2)
> +         nsresult (InstanceClass::*InitMethod)() = nullptr>
> +static nsresult
> +Constructor(nsISupports* aOuter, REFNSIID aIID, void** aResult)
> +{
> +  *aResult = nullptr;
> +  if (aOuter != nullptr) {

nit: same here w/nullptr comparison

::: security/manager/ssl/nsNSSU2FToken.h:33
(Diff revision 2)
>    // For nsNSSShutDownObject
>    virtual void virtualDestroyNSSReference() override;
>    void destructorSafeDestroyNSSReference();
>  
> +  // Required to satisfy Win32 where nsresult != NS_IMETHOD
> +  nsresult Initialize() { return Init(); }

Let's actually fix this by removing init from nsINSSU2FToken.idl.

Comment 13

[Masatoshi Kimura [:emk]]

Comment on attachment 8831405 [details]
Bug 1122973 - Use templates instead of macros for NSS module factory constructors.

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

Comment 14

[Masatoshi Kimura [:emk]]

Comment on attachment 8831405 [details]
Bug 1122973 - Use templates instead of macros for NSS module factory constructors.

https://reviewboard.mozilla.org/r/107982/#review109410

> nit: no need for comparison to nullptr (as in, we can just say `InitMethod ? (inst->*InitMethod)() : NS_OK`)

I had to add the comparison to silence compiler warnings on Linux/macOS.

> Let's actually fix this by removing init from nsINSSU2FToken.idl.

Done.

Comment 15

[:Cykesiopka]

Comment on attachment 8831405 [details]
Bug 1122973 - Use templates instead of macros for NSS module factory constructors.

https://reviewboard.mozilla.org/r/107982/#review110924

Sorry for the delay.

Anyways, thanks for doing this. Not having all these horrible macros is a lot nicer.

Comment 16

[Masatoshi Kimura [:emk]]

Comment on attachment 8831405 [details]
Bug 1122973 - Use templates instead of macros for NSS module factory constructors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107982/diff/3-4/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a1ed6f4ed6a
Use templates instead of macros for NSS module factory constructors. r=Cykesiopka,keeler

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/3a1ed6f4ed6a