Closed Bug 802378 Opened 8 years ago Closed 8 years ago

PSM code cleanup

Categories

(Core :: Security: PSM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(7 files)

No description provided.
Not sure how to handle this one.

The changes are:

* Replace "<X> != NULL" with "<X>"
* Replace "<X> == NULL" with "!<X>"
* replace "NULL" with "nullptr.

Unfortunately, there are many such changes.
Attachment #672067 - Flags: review?(dkeeler)
This is mostly for Camilo's benefit but he's gone for a while.

This patch is 90%+ whitespace-only. I will attach the diff -w version of the patch. I recommend you review just the diff -w version.
Attachment #672071 - Flags: review?(dkeeler)
Comment on attachment 672050 [details] [diff] [review]
Part 1: Replace nsRfPtr uses with RefPtr uses in security/manager

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

Yes, this looks like it accomplishes what you set out to do.

::: security/manager/ssl/src/nsCertTree.h
@@ +126,5 @@
>  
>    treeArrayEl *GetThreadDescAtIndex(int32_t _index);
>    already_AddRefed<nsIX509Cert> 
>      GetCertAtIndex(int32_t _index, int32_t *outAbsoluteCertOffset = nullptr);
> +  mozilla::TemporaryRef<nsCertTreeDispInfo> 

Looks like some unnecessary whitespace at the end of this line.
On that note, there's a lot of unnecessary whitespace near and around the changes you're making - do you want to take care of those as well?
Attachment #672050 - Flags: review?(dkeeler) → review+
The NSS header files should all now include extern "C" declarations so we don't need to wrap them in extern "C" any more.

Here is the try run of all the above patches:
https://tbpl.mozilla.org/?tree=Try&rev=c20be467f413

If the extern "C" patch causes build failures then we'll fix the NSS header files.
Attachment #672073 - Flags: review?(dkeeler)
(In reply to David Keeler from comment #7)
> Looks like some unnecessary whitespace at the end of this line.
> On that note, there's a lot of unnecessary whitespace near and around the
> changes you're making - do you want to take care of those as well?

Thanks for pointing that out. I will trim all the unnecessary whitespace from the "+" lines of my patch before committing them. I won't remove the extra whitespace from the lines I did not touch though.

Also, I am going to fold all of these patches together before checkin, to minimize the impact they have on "hg blame."
Comment on attachment 672051 [details] [diff] [review]
Part 2: Replace nsAutoPtr with Scoped.h types in security/manager

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

Looks good. One comment would be that since you're taking out '#include "nsAutoPtr.h"' but not always adding in '#include "nsCOMPtr.h"', I'm assuming these files pick up that include elsewhere (when they need nsCOMPtr<>). In general, do we only care when this breaks something, or do we want to explicitly add in direct includes wherever they're needed?
Attachment #672051 - Flags: review?(dkeeler) → review+
(In reply to David Keeler from comment #10)
> Comment on attachment 672051 [details] [diff] [review]
> Looks good. One comment would be that since you're taking out '#include
> "nsAutoPtr.h"' but not always adding in '#include "nsCOMPtr.h"', I'm
> assuming these files pick up that include elsewhere (when they need
> nsCOMPtr<>). In general, do we only care when this breaks something, or do
> we want to explicitly add in direct includes wherever they're needed?

In some (most?), the #include was just totally unnecessary--maybe the code that used nsAutoPtr got removed but the #include wasn't removed.

Generally, when I remove code, I comment out all the headers in that source code file, then re-add them back until the the code compiles, then remove the ones that are still commented out. Generally I try to minimize the number of #includes.
Comment on attachment 672056 [details] [diff] [review]
Part 3: Remove error checking for operator new

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

I'm assuming operator new does the right thing anyway when it fails?
Anyway, I found another few cases you can probably take out on nsCertTree.cpp:713, nsCrypto.cpp:2028, nsClientAuthRemember.cpp:201, nsCertOverrideService.cpp:651, and TransportSecurityInfo.h:141 (maybe you had good reasons for not including these, but they seem to follow the patterns of what you've taken out).
Attachment #672056 - Flags: review?(dkeeler) → review+
Comment on attachment 672067 [details] [diff] [review]
Part 4: Use nullptr consistently in security/manager

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

My two cents is I'm not super keen on replacing "<X> != NULL" with "<X>" and "<X> == NULL" with "!<X>".
Anyway, on line 68 of nsOCSPResponder.cpp, there's a use of NULL (this and the other may be because I think I'm on a newer checkout than you - I got some conflicts when I applied the patch to my tree).

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +212,1 @@
>    if (mErrorMessageCached.IsEmpty()) {

On line 820 of this file there's a use of NULL.

::: security/manager/ssl/src/nsCertTree.cpp
@@ -710,5 @@
>    uint32_t count = mDispInfo.Length();
>    mNumOrgs = CountOrganizations();
>    mTreeArray = new treeArrayEl[mNumOrgs];
> -  if (!mTreeArray)
> -    return NS_ERROR_OUT_OF_MEMORY;

Looks like this snuck in from part 2. Since you're folding them together anyway, this is probably fine.

::: security/manager/ssl/src/nsNSSCertHelper.cpp
@@ +149,1 @@
>      return rv;

There's a comment around line 945 that references NULL - should it be changed too?

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +907,5 @@
>      goto loser;
>    }
>  
>    cert = CERT_NewTempCertificate(CERT_GetDefaultCertDB(), collectArgs->rawCerts,
> +                	         nullptr, false, true);

Looks like there's a tab in here - replace with spaces?

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +142,1 @@
>  

There's a comment around line 1832 that mentions "NULL" - is that something that should be changed as well?

@@ +1647,5 @@
>                  hostLower = _str_to_lower(PL_strdup(hostname));
>              }
>  
>              /* the hostname satisfies the constraint */
> +            if (((substring = strstr(hostLower, pattern)) != nullptr) &&

Looks like this doesn't follow the pattern of "<X>" instead of "<X> != NULL".

::: security/manager/ssl/src/nsPK11TokenDB.cpp
@@ +373,5 @@
>    NS_ConvertUTF16toUTF8 aUtf8OldPassword(oldPassword);
>    NS_ConvertUTF16toUTF8 aUtf8NewPassword(newPassword);
>  
>    rv = PK11_ChangePW(mSlot, 
> +         (oldPassword ? const_cast<char *>(aUtf8OldPassword.get()) : nullptr), 

trailing whitespace
Attachment #672067 - Flags: review?(dkeeler) → review+
(In reply to David Keeler from comment #12)
> I'm assuming operator new does the right thing anyway when it fails?

https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation

> Anyway, I found another few cases you can probably take out on
> nsCertTree.cpp:713

Fixed in the next patch, as you found.

> nsCrypto.cpp:2028,

I will fix this before checkin.

> nsClientAuthRemember.cpp:201,
> nsCertOverrideService.cpp:651,
> TransportSecurityInfo.h:141

I will pull out the hashtable-related changes from my patch. It seems PutEntry is more complicated than I thought, regarding whether it can return null.

(In reply to David Keeler from comment #13)
> My two cents is I'm not super keen on replacing "<X> != NULL" with "<X>" and
> "<X> == NULL" with "!<X>".

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#C.2FC.2B.2B_practices

> Anyway, on line 68 of nsOCSPResponder.cpp, there's a use of NULL (this and
> the other may be because I think I'm on a newer checkout than you - I got
> some conflicts when I applied the patch to my tree).

It is because I have another patch that does "hg rm nsOCSPResponder.cpp". I will just leave nsOCSPResponder.cpp untouched.

> ::: security/manager/ssl/src/TransportSecurityInfo.cpp
> @@ +212,1 @@
> >    if (mErrorMessageCached.IsEmpty()) {
> 
> On line 820 of this file there's a use of NULL.

Thanks. Also due to a locally-applied patch that changed that line. I will fix this before I commit.

> ::: security/manager/ssl/src/nsNSSCertHelper.cpp
> @@ +149,1 @@
> >      return rv;
> 
> There's a comment around line 945 that references NULL - should it be
> changed too?

Generally I did not change the comments (though there are some exceptions). We can change the comments in another pass if we think it is important. What do you think?

> >    cert = CERT_NewTempCertificate(CERT_GetDefaultCertDB(), collectArgs->rawCerts,
> > +                	         nullptr, false, true);
> 
> Looks like there's a tab in here - replace with spaces?

OK.

> >              /* the hostname satisfies the constraint */
> > +            if (((substring = strstr(hostLower, pattern)) != nullptr) &&
> 
> Looks like this doesn't follow the pattern of "<X>" instead of "<X> != NULL".

This form is an exception, because some compilers will warn you that you might be using operator= when you mean to use operator==, unless you have an explicit comparison operator.
Comment on attachment 672072 [details] [diff] [review]
Part 5, diff -w version

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

::: security/manager/ssl/src/nsCertTree.cpp
@@ +1454,5 @@
>  void 
>  nsCertTree::CmpInitCriterion(nsIX509Cert *cert, CompareCacheHashEntry *entry,
>                               sortCriterion crit, int32_t level)
>  {
> +  NS_ENSURE_TRUE(cert != 0 && entry !=0, RETURN_NOTHING);

"NS_ENSURE_TRUE(cert && entry, RETURN_NOTHING);" ?

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1283,5 @@
>    nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);
>  
>    int32_t ocspEnabled;
>    pref->GetIntPref("security.OCSP.enabled", &ocspEnabled);
> +  *aOcspOn = ocspEnabled; 

trailing whitespace
Attachment #672072 - Flags: review+
Attachment #672071 - Flags: review?(dkeeler) → review+
Comment on attachment 672073 [details] [diff] [review]
Part 6: Remove uses of extern "C" for NSS headers in security/manager

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

Does md4.h (do we even still use md4?) need modification too? Here's what I mean:

#ifdef __cplusplus
extern "C" {
#endif

#include "mozilla/StandardInteger.h"  <-- move this outside the 'extern "C"'?
Attachment #672073 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ff177abfbe

All the above patches had the review comments addressed and were folded together.

BTW, the purpose of this change is to make it easier for newcomers to contribute to PSM. All of the above changes were made based on questions asked by new people and/or based on reviews of new contributors' code. Hopefully, this change will also reduce the likelihood of future mass changes affecting PSM.
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/a8ff177abfbe
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
I've backed this out with Brian's OK just to help in the investigation of bug 803267. Assuming it doesn't change the crash stats associated with that bug I'll put it back asap - hopefully thursday.

 https://hg.mozilla.org/integration/mozilla-inbound/rev/82d78390fda1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/9a4531d2d243
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.