Closed
Bug 1296218
Opened 8 years ago
Closed 8 years ago
Clean up PK11PasswordPromptRunnable::RunOnTargetThread()
Categories
(Core :: Security: PSM, defect, P1)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: Cykesiopka, Assigned: Cykesiopka)
References
()
Details
(Whiteboard: [psm-assigned])
Attachments
(1 file)
The method should be updated to modern PSM style and the manual memory management should be removed.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
mozreview-review |
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 3•8 years ago
|
||
mozreview-review |
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.
Attachment #8782475 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 4•8 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db6f65eaaf52d12beb6eb72af0cd1e91a5e2374a
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c807d56a4f21 Clean up PK11PasswordPromptRunnable::RunOnTargetThread(). r=keeler
Keywords: checkin-needed
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c807d56a4f21
Status: ASSIGNED → RESOLVED
Closed: 8 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.
Description
•