1296218 - Clean up PK11PasswordPromptRunnable::RunOnTargetThread()

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[:Cykesiopka]

The method should be updated to modern PSM style and the manual memory management should be removed.

Comment 1

[:Cykesiopka]

Attachment: Bug 1296218 - Clean up PK11PasswordPromptRunnable::RunOnTargetThread().

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

Comment 2

[:Cykesiopka]

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

[Dana Keeler (she/her) [:keeler]]

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 4

[:Cykesiopka]

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 5

[:Cykesiopka]

Comment on attachment 8782475 [details]
Bug 1296218 - Clean up PK11PasswordPromptRunnable::RunOnTargetThread().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72634/diff/1-2/

Comment 6

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db6f65eaaf52d12beb6eb72af0cd1e91a5e2374a

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c807d56a4f21
Clean up PK11PasswordPromptRunnable::RunOnTargetThread(). r=keeler

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c807d56a4f21