961454 - PSM peers discuss whitespace too much during code reviews

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: ws-1-CertVerifier.patch

These patches contain:

1. Removed trailing whitespace
2. Replaced "Foo * foo" and "Foo *foo" with "Foo* foo", and same for "&".
3. Return type on its own line.
4. C-style comments replaced with C++-style comments.
5. Unnecessary comments removed.
6. Some reformatting of lines that I changed with the above changes, or near them.
7. Removal of commented-out code.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: ws-2-nsNSSComponent.patch

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: ws-3-nsNSSIOLayer.patch

Note that it is extremely unlikely that we're going to ever support that (non-standard?) scope-of-use extension, which was intended to restrict the set of websites that a certificate can be used with, so I just deleted all that commented-out/#ifdefed-out code.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: ws-3-nsNSSIOLayer.patch

Forgot to amend commit after fixing type.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: ws-4-SSLServerCertVerification.patch

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: ws-5-IdentityChecking.patch

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: ws-5-IdentityChecking.patch

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: ws-6-nsNSSCertificate.patch

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The patches I submitted cover the files we modify most often, and they are also the files that will be modified by the insanity::pkix integration. I suggest that for the rest of the files, we clean them up in a lazy fashion: If you modify other PSM files, then feel free to do the whitespace cleanup for *in a separate patch* in the same bug as the patch that makes the semantic changes.

Comment 10

[Camilo Viecco (:cviecco)]

Comment on attachment 8362182 [details] [diff] [review]
ws-4-SSLServerCertVerification.patch

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

Great bug name.

Comment 11

[Camilo Viecco (:cviecco)]

Comment on attachment 8362186 [details] [diff] [review]
ws-6-nsNSSCertificate.patch

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

r+ with explanations in the bug

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +520,5 @@
>    nsMemory::Free(key.data); // SECItem is a 'c' type without a destrutor
>    return (*aDbKey) ? NS_OK : NS_ERROR_FAILURE;
>  }
>  
> +NS_IMETHODIMP

This is not abut this patch, but should we leave the comment until we constify the input?

@@ -830,1 @@
>  NS_IMETHODIMP

Why remove the comment?

Comment 12

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

Comment on attachment 8362178 [details] [diff] [review]
ws-1-CertVerifier.patch

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

Awesome.

Comment 13

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

Comment on attachment 8362181 [details] [diff] [review]
ws-3-nsNSSIOLayer.patch

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

r- for now because there are still some of the same style issues remaining after this patch (e.g. there appear to be some trailing tabs, some */& still aren't in the right place, and there are still some /* */-style comments).
Thanks for doing this, Brian.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +69,2 @@
>  getSiteKey(const nsACString & hostName, uint16_t port,
>             /*out*/ nsCSubstring & key)

" & " -> "& "

@@ +344,5 @@
>  }
>  
>  NS_IMETHODIMP
>  nsNSSSocketInfo::JoinConnection(const nsACString & npnProtocol,
>                                  const nsACString & hostname,

" & " -> "& "

@@ +468,5 @@
>    if (isAlreadyShutDown())
>      return NS_ERROR_NOT_AVAILABLE;
>  
>    if (SECSuccess != SSL_OptionSet(mFd, SSL_SECURITY, true))
>      return NS_ERROR_FAILURE;		

Still some unnecessary whitespace at the end of this line.

@@ +1797,5 @@
>    ClientAuthDataRunnable(CERTDistNames* caNames,
>                           CERTCertificate** pRetCert,
>                           SECKEYPrivateKey** pRetKey,
>                           nsNSSSocketInfo * info,
> +                         CERTCertificate * serverCert)

This line and the one above still do ' * ', not '* '.

@@ +1929,4 @@
>      goto loser;
>    }
>  
>    /* find valid user cert and key pair */	

Still some whitespace at the end of this line. Also, C-style comments here and below.

@@ +2326,2 @@
>                         const char *proxyHost, const char *host, int32_t port,
>                         nsNSSSocketInfo *infoObject)

' *' -> '* '

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: ws-3.1-nsNSSIOLayer.patch

I fixed some more whitespace issues in this patch.

Comment 15

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

Comment on attachment 8363291 [details] [diff] [review]
ws-3.1-nsNSSIOLayer.patch

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

I still found '&' spacing problems in getSiteKey, GetNegotiatedNPN, JoinConnection, SetNPNList, getSocketInfoIfRunning, rememberTolerantAtVersion, rememberIntolerantAtVersion, adjustForTLSIntolerance, CloseSocketAndDestroy, setRenegoUnrestrictedSites, isRenegoUnrestrictedSite, ClientAuthDataRunnable (the constructor and declaration of member variables), ClientAuthDataRunnable::RunOnTargetThread (many local variables), nsSSLIOLayerImportFD, nsSSLIOLayerSetOptions, and nsSSLIOLayerAddToSocket (sslSock).
There's trailing tabs in ActivateSSL, as well (and some leading tabs in nsGetUserCertChoice).
Also, something strange happened with indentation in ClientAuthDataRunnable::RunOnTargetThread.
Let me know if you would rather be doing something else with your time and I can finish this up (or do a follow-up in a separate bug).

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +1953,5 @@
>        if (privKey) {
>          if (hasExplicitKeyUsageNonRepudiation(node->cert)) {
>            privKey = nullptr;
>            // Not a prefered cert
>            if (!low_prio_nonrep_cert) // did not yet find a low prio cert

braces for this 'if' too?

@@ +1963,5 @@
>          }
>        }
>        keyError = PR_GetError();
>        if (keyError == SEC_ERROR_BAD_PASSWORD) {
> +        // problem with password: bail

inconsistent indentation - I suggest the next line be un-indented two spaces.

@@ +1979,5 @@
>      if (!cert) {
>          goto noCert;
>      }
> +  } else { // Not Auto => ask
> +    //  Get the SSL Certificate

nit: don't need two spaces before "Get"

@@ +2006,1 @@
>        canceled = true;

looks like this line needs an indent

@@ +2008,1 @@
>        nsCOMPtr<nsIX509CertDB> certdb;

I think this whole function needs re-indenting from here on...

@@ +2120,5 @@
>      int32_t CertsToUse;
>      for (CertsToUse = 0, node = CERT_LIST_HEAD(certList);
>           !CERT_LIST_END(node, certList) && CertsToUse < nicknames->numnicknames;
>           node = CERT_LIST_NEXT(node)
> +        ) {

maybe the closing parenthesis and brace can be on the previous line?

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: All patches rolled into one.

David, the previous patch (ws-3.1-nsNSSIOlayer.patch) was a "diff -w". That's why the whitespace-only changes didn't show up. The "diff -w" shows that the big indention changes I did were whitespace-only. Please review the nsNSSIOLayer.cpp changes in this folded patch.

After we land the insanity::pkix-related changes, we can do other reformatting, especially related to braces, if that helps people. The main intent of these patches is to make the review of the insanity::pkix-related changes easier.

Comment 17

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

Comment on attachment 8363369 [details] [diff] [review]
All patches rolled into one.

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

Looks good.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +1745,5 @@
> +  ret = pref->GetCharPref("security.default_personal_cert", &mode);
> +  if (NS_FAILED(ret)) {
> +    goto loser;
> +  }
> +  

Splinter says there's still some unnecessary whitespace on otherwise empty lines in this function. Your call if you want to address it now.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/914e2811cfca

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/914e2811cfca