If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Clean up PK11PasswordPromptRunnable::RunOnTargetThread()

RESOLVED FIXED in Firefox 51

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [psm-assigned], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
The method should be updated to modern PSM style and the manual memory management should be removed.
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year 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

a year 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

a year 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

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db6f65eaaf52d12beb6eb72af0cd1e91a5e2374a
Keywords: checkin-needed

Comment 7

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c807d56a4f21
Clean up PK11PasswordPromptRunnable::RunOnTargetThread(). r=keeler
Keywords: checkin-needed

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c807d56a4f21
Status: ASSIGNED → RESOLVED
Last Resolved: a year 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.