Clean up nsISecretDecoderRing and implementation

RESOLVED FIXED in Firefox 51

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
There are a wide array of issues with nsISecretDecoderRing and its implementation that should be fixed.

nsISecretDecoderRing.idl:
1) The file lives under netwerk/, even though it has basically nothing to do with Necko.
  1a) It should be moved to PSM.
2) encrypt() and decrypt() are defined in the IDL as C++ only.
  2a) This doesn't seem particularly useful, since AFAICT the only real consumers of SecretDecoderRing are JS callers protecting strings.
  2b) The methods should be removed from the IDL, and treated as private to the implementation.
3) encryptString() and decryptString() take raw C strings.
  3a) Having them take and return nsACStrings might be nicer.
4) Contains nsISecretDecoderRingConfig, which has never been implemented.
  4a) The interface should be removed.

nsSDR.cpp:
1) Does manual memory management.
  1a) Smart pointers, RAII types etc should be used.
2) Uses goto.
3) Uses C style casts.
  3a) C++ casts should be used instead.
4) Has inconsistent and difficult to follow parameter and variable names.
I was thinking it might be nice to also rename nsSDR.{h,cpp} to something more intuitive and consistent with pretty much the entire rest of Gecko: either nsSecretDecoderRing.{h,cpp} (which is the old style, but at least is the most common interface/implementation naming style) or SecretDecoderRing.{h,cpp}, which moves away from the old, unnecessary "ns" prefix. I don't know if the consistency benefits outweigh the downsides of renaming the files, but it's something to consider.
(Assignee)

Comment 2

2 years ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #1)
> I was thinking it might be nice to also rename nsSDR.{h,cpp} to something
> more intuitive and consistent with pretty much the entire rest of Gecko:
> either nsSecretDecoderRing.{h,cpp} (which is the old style, but at least is
> the most common interface/implementation naming style) or
> SecretDecoderRing.{h,cpp}, which moves away from the old, unnecessary "ns"
> prefix. I don't know if the consistency benefits outweigh the downsides of
> renaming the files, but it's something to consider.

Yeah, we might as well rename the files as well. There doesn't seem to be any real harm to doing so as long as we |hg mv| the files.
(Assignee)

Updated

2 years ago
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [psm-cleanup] → [psm-assigned]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8781799 [details]
Bug 1275841 - Rename nsSDR.(cpp|h) to SecretDecoderRing.(cpp|h).

https://reviewboard.mozilla.org/r/72124/#review69920

Cool - lgtm.

::: security/manager/ssl/nsCertOverrideService.cpp:19
(Diff revision 1)
>  #include "nsCRT.h"
>  #include "nsILineInputStream.h"
>  #include "nsIObserver.h"
>  #include "nsIObserverService.h"
> +#include "nsIOutputStream.h"
> +#include "nsISafeOutputStream.h"

These changes seem unrelated, but it's not a big deal (were they from a different patch/bug?)

::: security/manager/ssl/nsNSSModule.cpp:265
(Diff revision 1)
>  
>  static const mozilla::Module::CIDEntry kNSSCIDs[] = {
>    { &kNS_NSSCOMPONENT_CID, false, nullptr, nsNSSComponentConstructor },
>    { &kNS_SSLSOCKETPROVIDER_CID, false, nullptr, nsSSLSocketProviderConstructor },
>    { &kNS_STARTTLSSOCKETPROVIDER_CID, false, nullptr, nsTLSSocketProviderConstructor },
> -  { &kNS_SDR_CID, false, nullptr, nsSecretDecoderRingConstructor },
> +  { &kNS_SDR_CID, false, nullptr, SecretDecoderRingConstructor },

Hmmm - do we also want to update "NS_SDR_CID"?
Attachment #8781799 - Flags: review?(dkeeler) → review+
Comment on attachment 8781801 [details]
Bug 1275841 - Remove unnecessary methods and interfaces from nsISecretDecoderRing.idl.

https://reviewboard.mozilla.org/r/72128/#review69998

Nice
Attachment #8781801 - Flags: review?(dkeeler) → review+
Comment on attachment 8781802 [details]
Bug 1275841 - Make nsISecretDecoderRing.idl encryptString() and decryptString() use the Mozilla string classes.

https://reviewboard.mozilla.org/r/72130/#review69996

::: security/manager/ssl/SecretDecoderRing.cpp:25
(Diff revision 1)
>  #include "nsThreadUtils.h"
>  #include "pk11func.h"
>  #include "pk11sdr.h" // For PK11SDR_Encrypt, PK11SDR_Decrypt
> -#include "plstr.h"
>  #include "ssl.h" // For SSL_ClearSessionCache
>  #include "stdlib.h"

Is there anything that actually still uses stdlib in this file after this patch? The memcpy was the only thing I could find, and it's gone after this.
Comment on attachment 8781802 [details]
Bug 1275841 - Make nsISecretDecoderRing.idl encryptString() and decryptString() use the Mozilla string classes.

https://reviewboard.mozilla.org/r/72130/#review70002

Whoops. Hit publish too soon. This is great - r=me.
Attachment #8781802 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8781799 [details]
Bug 1275841 - Rename nsSDR.(cpp|h) to SecretDecoderRing.(cpp|h).

https://reviewboard.mozilla.org/r/72124/#review69920

> These changes seem unrelated, but it's not a big deal (were they from a different patch/bug?)

The changes are necessary for the build to succeed.
I imagine this is because the rename means SecretDecoderRing.(h|cpp) is no longer part of the unified file nsCertOverrideService.cpp belongs to.

> Hmmm - do we also want to update "NS_SDR_CID"?

Yes, I think that makes sense.
I will rename ```NS_SDR_CONTRACTID``` and remove the conditional definition ifdefs as well.
(I have no idea what "base code" means, and why SecretDecoderRing.h isn't the logical place to put the define.)
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8781802 [details]
Bug 1275841 - Make nsISecretDecoderRing.idl encryptString() and decryptString() use the Mozilla string classes.

https://reviewboard.mozilla.org/r/72130/#review69996

> Is there anything that actually still uses stdlib in this file after this patch? The memcpy was the only thing I could find, and it's gone after this.

No - I'll remove the include and anything else that looks unused.

Comment 13

2 years ago
mozreview-review
Comment on attachment 8781800 [details]
Bug 1275841 - Move nsISecretDecoderRing.idl from netwerk/ to security/manager/ssl.

https://reviewboard.mozilla.org/r/72126/#review70498
Attachment #8781800 - Flags: review?(mcmanus) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 years ago
Thanks for the reviews!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db6f65eaaf52
Keywords: checkin-needed

Comment 19

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eda8e1c8c04
Rename nsSDR.(cpp|h) to SecretDecoderRing.(cpp|h). r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/9406ee085794
Move nsISecretDecoderRing.idl from netwerk/ to security/manager/ssl. r=mcmanus
https://hg.mozilla.org/integration/mozilla-inbound/rev/655dfcd1a769
Remove unnecessary methods and interfaces from nsISecretDecoderRing.idl. r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/72357ad5852b
Make nsISecretDecoderRing.idl encryptString() and decryptString() use the Mozilla string classes. r=keeler
Keywords: checkin-needed

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8eda8e1c8c04
https://hg.mozilla.org/mozilla-central/rev/9406ee085794
https://hg.mozilla.org/mozilla-central/rev/655dfcd1a769
https://hg.mozilla.org/mozilla-central/rev/72357ad5852b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.