nsSDR.cpp has some issues we can fix: - Use of deprecated Scoped.h features - gotos - Variables declared far from where they are actually used - Unnecessary variables There are other issues like manual memory management and unimplemented methods, but those can/will be fixed later.

Review commit: https://reviewboard.mozilla.org/r/37015/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37015/

https://reviewboard.mozilla.org/r/37015/#review33607 ::: security/manager/ssl/tests/unit/test_sdr.js:25 (Diff revision 1) > + // We have to login with the default password before we attempt to encrypt > + // or decrypt anything. It's more useful to actually say why we need to do this.

Comment on attachment 8724364 [details] MozReview Request: Bug 1250256 - Partially clean up nsSDR.cpp. r=keeler https://reviewboard.mozilla.org/r/37015/#review34021 Looks good - just a few comments. ::: security/manager/ssl/nsSDR.cpp:145 (Diff revision 1) > EncryptString(const char *text, char **_retval) This function could use some cleanup as well (e.g. * alignment, NS_IMETHODIMP on its own line, nsSecretDecoderRing::EncryptString all together, no need for the shutdown lock here, use of goto, manually-managed memory, etc.) Oh, I see - the intention wasn't to clean everything up. Well, it's up to you - this feels like a fairly easy win, but it's been this way for a while, so I can't imagine it would hurt to leave it some more. ::: security/manager/ssl/nsSDR.cpp:169 (Diff revision 1) > DecryptString(const char *crypt, char **_retval) Same here. ::: security/manager/ssl/nsSDR.cpp:246 (Diff revision 1) > nsNSSShutDownPreventionLock locker; This needs if (isAlreadyShutDown()) { return NS_ERROR_NOT_AVAILABLE; } ::: security/manager/ssl/nsSDR.cpp:265 (Diff revision 1) > nsNSSShutDownPreventionLock locker; Same here. ::: security/manager/ssl/tests/unit/head_psm.js:14 (Diff revision 1) > -var { ctypes } = Cu.import("resource://gre/modules/ctypes.jsm"); > +const { MockRegistrar } = Factoring out XPCOMUtils seems fine, but let's not add new things that only one test file needs (unless there's a reason they need to be loaded in head_psm.js?) ::: security/manager/ssl/tests/unit/test_sdr.js:54 (Diff revision 1) > + equal(input, sdr.decryptString(encrypted), Is the SDR deterministic? I think the default key is just "" - it might be worth it to compare encrypted to the expected encrypted value, if it is the same every time.

https://reviewboard.mozilla.org/r/37015/#review34021 Thanks for the review! > This function could use some cleanup as well (e.g. * alignment, NS_IMETHODIMP on its own line, nsSecretDecoderRing::EncryptString all together, no need for the shutdown lock here, use of goto, manually-managed memory, etc.) > Oh, I see - the intention wasn't to clean everything up. Well, it's up to you - this feels like a fairly easy win, but it's been this way for a while, so I can't imagine it would hurt to leave it some more. I made some of the additional changes. I have a long term goal of eradicating goto and manually managed memory in PSM, so hopefully I will get the remaining stuff cleaned up at some point. > Factoring out XPCOMUtils seems fine, but let's not add new things that only one test file needs (unless there's a reason they need to be loaded in head_psm.js?) Yeah, I was just being lazy, since a test file I'm adding in Bug 1250258 will make use of MockRegistrar as well. I plan on submitting the patch for that within a week or two, so I think keeping MockRegistrar here is OK for now. > Is the SDR deterministic? I think the default key is just "" - it might be worth it to compare encrypted to the expected encrypted value, if it is the same every time. Looks like at least in the context in which we use the SDR here, no. The encrypted value for the same input changes each time the test file is run.

Comment on attachment 8724364 [details] MozReview Request: Bug 1250256 - Partially clean up nsSDR.cpp. r=keeler Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37015/diff/1-2/