Closed Bug 1241646 Opened 4 years ago Closed 4 years ago

remove unused token arguments from nsIX509CertDB (and other interfaces?)

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: keeler, Assigned: bmanojkumar24, Mentored)

References

Details

(Keywords: addon-compat, Whiteboard: [good first bug])

Attachments

(1 file, 4 obsolete files)

A number of nsIX509CertDB functions have an argument 'nsISupports aToken' that the underlying implementation completely ignores. We should just remove those arguments for a cleaner interface/implementation. This may be an issue with other interfaces in security/manager/ssl. If so, those should be cleaned up as well.
Attachment #8711352 - Flags: review?(dkeeler)
Hi, I have removed  argument "nsISupports aToken" from all instances  found in the file "security/manager/ssl/nsIX509CertDB.idl" and their corresponding implementation in the file "security/manager/ssl/nsNSSCertificateDB.cpp". Please do let me know of any other changes need to be done. :)
Comment on attachment 8711352 [details]
MozReview Request: Bug 1241646 - remove unused token arguments from nsIX509CertDB (and other interfaces?)

https://reviewboard.mozilla.org/r/32121/#review28995

This is a good start! There are a few items to address.

::: security/manager/ssl/nsIX509CertDB.idl:49
(Diff revision 1)
>  [scriptable, uuid(a36c45fb-f7b5-423e-a0f7-ea1eb4fd60b5)]

This needs to be updated with a new value from 'uuidgen'.

::: security/manager/ssl/nsIX509CertDB.idl:65
(Diff revision 1)
>     *  @param aToken Optionally limits the scope of

Please update the documentation on all of these as well.

::: security/manager/ssl/nsIX509CertDB.idl:255
(Diff revision 1)
> -  void importPKCS12File(in nsISupports aToken,
> +  void importPKCS12File(in nsIFile aFile);

This function actually does use the token, so this argument can't be removed.

::: security/manager/ssl/nsIX509CertDB.idl:268
(Diff revision 1)
> -  void exportPKCS12File(in nsISupports aToken,
> +  void exportPKCS12File(in nsIFile aFile,

Same thing - the token is actually used here.

::: security/manager/ssl/nsNSSCertificateDB.cpp:95
(Diff revision 1)
> -nsNSSCertificateDB::FindCertByNickname(nsISupports *aToken,
> +nsNSSCertificateDB::FindCertByNickname(const nsAString &nickname,

If you're interested, feel free to address some style nits while you're modifying these lines:

& and \* go to the left, as in 'const nsAString& nickname' or 'nsIX509Cert\*\* \_rvCert'.

When wrapping long lines of arguments, the first characters should line up. In this case, the 'n' in 'nsIX509Cert...' should align with the 'c' in 'const nsAString...'

Lines should be less than 80 characters.

There shouldn't be any trailing whitespace (e.g. see line 125).

(See the style guide at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style )

::: security/manager/ssl/nsNSSCertificateDB.cpp:181
(Diff revision 1)
> -nsNSSCertificateDB::FindCertNicknames(nsISupports *aToken, 
> +nsNSSCertificateDB::FindCertNicknames(uint32_t      aType,

Just as a heads-up, I'm simultaneously working on a patch to remove this function.

::: security/manager/ssl/nsNSSCertificateDB.cpp:1066
(Diff revision 1)
> -nsNSSCertificateDB::ImportCertsFromFile(nsISupports *aToken, 
> +nsNSSCertificateDB::ImportCertsFromFile(nsIFile *aFile,

Style: this line will probably be <80 characters if the aType argument is put on the same line.

::: security/manager/ssl/nsNSSCertificateDB.cpp:1322
(Diff revision 1)
> -nsNSSCertificateDB::FindCertByEmailAddress(nsISupports *aToken, const char *aEmailAddress, nsIX509Cert **_retval)
> +nsNSSCertificateDB::FindCertByEmailAddress(const char *aEmailAddress, nsIX509Cert **_retval)

Style: this line is too long
Attachment #8711352 - Flags: review?(dkeeler)
Assignee: nobody → bmanojkumar24
Attached patch 1241646.patch (obsolete) — Splinter Review
Had to post the patch here,instead of MozReview, I updated the extension, and then some how it keeps crashing :(
Attachment #8711352 - Attachment is obsolete: true
Attachment #8712040 - Flags: review?(dkeeler)
Comment on attachment 8712040 [details] [diff] [review]
1241646.patch

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

This is looking good. Just a couple more things to address.

::: security/manager/ssl/nsIX509CertDB.idl
@@ -57,4 @@
>    const unsigned long TRUSTED_SSL     = 1 << 0;
>    const unsigned long TRUSTED_EMAIL   = 1 << 1;
>    const unsigned long TRUSTED_OBJSIGN = 1 << 2;
>  

Up where the nsIX509CertDB interface is defined, you can replace the hex string with this: 5c16cd9b-5a73-47f1-ab0f-11ede7495cce

@@ +58,5 @@
>    const unsigned long TRUSTED_EMAIL   = 1 << 1;
>    const unsigned long TRUSTED_OBJSIGN = 1 << 2;
>  
>    /**
>     *  Given a nickname and optionally a token,

nit: also take out "and optionally a token"

::: security/manager/ssl/nsNSSCertificateDB.cpp
@@ +127,5 @@
>  }
>  
> +NS_IMETHODIMP
> +nsNSSCertificateDB::FindCertByDBKey(const char* aDBkey,
> +                                    nsIX509Cert** _cert)

nit: I think these two can be one line

@@ +199,5 @@
>  
>  NS_IMETHODIMP 
> +nsNSSCertificateDB::FindCertNicknames(uint32_t      aType,
> +                                     uint32_t*     _count,
> +                                     char16_t***  _certNames)

nit: these two lines should move right one more space. Also, there's no need for more than one space between the types and their names.

@@ +1173,2 @@
>                                       uint32_t          count,
> +                                     nsIX509Cert**     certs)

Again, no need for multiple spaces here. And anyway, since this function isn't changing, we probably don't need to address the preexisting style issues here.

@@ +1338,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsNSSCertificateDB::FindCertByEmailAddress(const char *aEmailAddress,
> +                                           nsIX509Cert **_retval)

nit: * to the left here as well
Attachment #8712040 - Flags: review?(dkeeler) → review-
Attached patch 1241646.patch (obsolete) — Splinter Review
Hope this looking good now :)
Attachment #8712040 - Attachment is obsolete: true
Attachment #8712526 - Flags: review?(dkeeler)
Comment on attachment 8712526 [details] [diff] [review]
1241646.patch

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

Ok - this looks great!
So that's part 1 of this bug. The second part is to update the code that calls any of the functions you modified. You can use dxr to find what you need to change - for example: https://dxr.mozilla.org/mozilla-central/search?q=findCertByNickname&redirect=false&case=false (ignore everything in obj-x86_64-unknown-linux-gnu/ or security/nss/ - most of what you'll need to update is in security/manager/).
Attachment #8712526 - Flags: review?(dkeeler) → review+
Attached patch 1241646.patch (obsolete) — Splinter Review
Hi,please let me know, if the changes are any good.
Attachment #8712526 - Attachment is obsolete: true
Attachment #8713057 - Flags: feedback?(dkeeler)
Comment on attachment 8713057 [details] [diff] [review]
1241646.patch

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

This is almost perfect, except for two places in C++ code that also need to be updated:

https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/security/manager/ssl/nsNSSIOLayer.cpp#2240 (the call to FindCertByDBKey)
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/security/LocalCertService.cpp#261 (no permalink for this one - looks like dxr is having issues. In any case, it's a call to FindCertByNickname)

::: security/manager/pki/resources/content/viewCertDetails.js
@@ +74,5 @@
>      //var tokenName = "";
>      //var pk11db = Components.classes[nsPK11TokenDB].getService(nsIPK11TokenDB);
>      //var token = pk11db.findTokenByName(tokenName);
>  
>      //var cert = certdb.findCertByNickname(token, myName);

Might as well remove these commented-out lines too.
Attachment #8713057 - Flags: feedback?(dkeeler) → feedback+
Attached patch 1241646.patchSplinter Review
Hi, I have made changes.Please review changes.
Attachment #8713057 - Attachment is obsolete: true
Attachment #8713949 - Flags: review?(dkeeler)
Comment on attachment 8713949 [details] [diff] [review]
1241646.patch

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

Great!
I started a try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3abb246d89c8
(One comment is that since this bug really only addresses nsIX509CertDB, we can remove the '(and other interfaces?)' bit from the patch commit message.)
Attachment #8713949 - Flags: review?(dkeeler) → review+
Try looked good, so I went ahead and checked the patch in. Thanks for working on this!
https://hg.mozilla.org/mozilla-central/rev/62d300c9c733
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.