|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
58 bytes, text/x-review-board-request
|Details | Review|
The method should be updated to modern PSM style and the manual memory management should be removed.
Comment on attachment 8782475 [details] Bug 1296218 - Clean up PK11PasswordPromptRunnable::RunOnTargetThread(). https://reviewboard.mozilla.org/r/72634/#review70276 I tested this patch manually by enabling the master password, importing a client cert, and connecting to a site that requests client auth. It seems to work. I'm not really sure how to write a test that triggers this code however - suggestions welcome.
Comment on attachment 8782475 [details] Bug 1296218 - Clean up PK11PasswordPromptRunnable::RunOnTargetThread(). https://reviewboard.mozilla.org/r/72634/#review70398 Cool. Writing a test for this was a bit of a puzzle that I more or less accidentally solved. If you want to solve it as well, I can start you out with saying register a mock nsIWindowWatcher that implements getNewPrompter that returns something that implements nsIPrompt (really only promptPassword). For setting the initial password, I used https://dxr.mozilla.org/mozilla-central/rev/97a52326b06a07930216ebefa5af333271578904/security/manager/ssl/tests/unit/head_psm.js#735 as a reference. Then, to actually get the password prompt to come up, I used nsISecretDecoderRing.encryptString. Anyway, if you want to write that, that's cool. Otherwise, we can open a follow-up and I'll post what I have. ::: security/manager/ssl/nsNSSCallbacks.cpp:805 (Diff revision 1) > - > - if (NS_FAILED(rv)) > return; > + } > > // Although the exact value is ignored, we must not pass invalid bool values I think this comment applies to |checkState|, so it might be nice to have it directly above it. Also, we could probably improve it a bit: "|checkState| is unused because |checkMsg| (the argument just before it) is null, but XPConnect requires it to point to a valid bool nonetheless." ::: security/manager/ssl/nsNSSCallbacks.cpp:809 (Diff revision 1) > > // Although the exact value is ignored, we must not pass invalid bool values > // through XPConnect. > + nsXPIDLString password; > bool checkState = false; > - rv = prompt->PromptPassword(nullptr, promptString.get(), &password, nullptr, > + bool value = false; This is a preexisting problem, but I think we can improve the name of |value|. It seems it's true if the user clicked ok and false if they clicked cancel, so maybe we can rename it |userWantsToContinue| or |userClickedOK| or something.
Comment on attachment 8782475 [details] Bug 1296218 - Clean up PK11PasswordPromptRunnable::RunOnTargetThread(). https://reviewboard.mozilla.org/r/72634/#review70398 Thanks for the review! Also, thanks for solving how to write a test. I don't mind writing it, but you seem to have gotten pretty far, so I think it'll be less work overall if you posted what you already have. I filed Bug 1296619 for adding the test.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/c807d56a4f21 Clean up PK11PasswordPromptRunnable::RunOnTargetThread(). r=keeler