1085031 - Cleanup SSLStatus a bit

Status: RESOLVED WONTFIX

(Core :: Security: PSM, defect)

Description

[Tom S]

Attachment: cleanup-ssl

Comment 1

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

Comment on attachment 8507432 [details] [diff] [review]
cleanup-ssl

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

Looks great - I just have a few comments. See in particular the last comment. If you agree that that's a good idea, it might make sense to fold these changes in with those in bug 886752.

::: security/manager/ssl/public/nsISSLStatus.idl
@@ +7,5 @@
>  #include "nsISupports.idl"
>  
>  interface nsIX509Cert;
>  
>  [scriptable, uuid(fa9ba95b-ca3b-498a-b889-7c79cf28fee8)]

This uuid will need refreshing.

::: security/manager/ssl/src/nsSSLStatus.cpp
@@ +171,2 @@
>                                              NS_GET_IID(nsIX509Cert),
>                                              true);

nit: update indentation on these two lines

@@ +205,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsSSLStatus::GetHelperForLanguage(uint32_t aLanguage, nsISupports** _retval)

nit: update _retval here?

@@ +231,3 @@
>  {
>    *aClassID = (nsCID*) nsMemory::Alloc(sizeof(nsCID));
>    if (!*aClassID)

nit: braces around conditional body?

@@ +251,5 @@
>  
>  static NS_DEFINE_CID(kSSLStatusCID, NS_SSLSTATUS_CID);
>  
>  NS_IMETHODIMP
> +nsSSLStatus::GetClassIDNoAlloc(nsCID* aClassIDNoAlloc)

Many of these functions don't check that their arguments aren't null - do we care?

::: security/manager/ssl/src/nsSSLStatus.h
@@ +33,5 @@
>    nsCOMPtr<nsIX509Cert> mServerCert;
>  
>    uint32_t mKeyLength;
>    uint32_t mSecretKeyLength;
> +  nsCString mCipherName;

If we're going to change this, how about this: instead of storing the whole cipher name, we just store the cipher suite id. Then, if it's ever accessed, we look it up with SSL_GetCipherSuiteInfo. That way, we can also add direct access to the cipher suite id for no extra space cost (and, in fact, we save space this way).
This will require updating TRANSPORTSECURITYINFOMAGIC again, so it would be nice if these changes all happened at once.

Comment 2

[Tom S]

>Many of these functions don't check that their arguments aren't null - do we care?
Generally I don't think we use NS_ENSURE_ARG_POINTER at all anymore. I just thought it's better than what we had before.

I am going to fold this with the other patch.