Closed Bug 1262645 Opened 8 years ago Closed 8 years ago

Address misc issues with nsGetUserCertChoice()

Categories

(Core :: Security: PSM, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

()

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

nsGetUserCertChoice() has various issues:
 - Returning a failure result when failing to get a pref value instead of more gracefully falling back to a default.
 - Using an enum instead of a more strongly typed enum class.
 - Using a pref branch instead of the preferred Preferences.h API.
 - Manual memory management.
 - Unnecessary use of pointers.

These can/should be fixed.
The follow issues are fixed:
  - Returning a failure result when failing to get a pref value instead of more
    gracefully falling back to a default.
  - Using an enum instead of a more strongly typed enum class.
  - Using a pref branch instead of the preferred Preferences.h API.
  - Manual memory management.
  - Unnecessary use of pointers.

Review commit: https://reviewboard.mozilla.org/r/46825/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46825/
Attachment #8741907 - Flags: review?(dkeeler)
Attachment #8741907 - Flags: review?(dkeeler) → review+
Comment on attachment 8741907 [details]
MozReview Request: Bug 1262645 - Address misc issues with nsGetUserCertChoice(). r=keeler

https://reviewboard.mozilla.org/r/46825/#review43467

Cool.

::: security/manager/ssl/nsNSSIOLayer.cpp:69
(Diff revision 1)
>    key.AppendASCII(":");
>    key.AppendInt(port);
>  }
>  
> -// SSM_UserCertChoice: enum for cert choice info
> -typedef enum {ASK, AUTO} SSM_UserCertChoice;
> +// Possible behaviors for choosing a cert for client auth.
> +enum class UserCertChoice {

Seems like this doesn't need to be defined so far away from where it's used, but not a big deal.
https://reviewboard.mozilla.org/r/46825/#review43467

Thanks for the review!

> Seems like this doesn't need to be defined so far away from where it's used, but not a big deal.

Fair point, moved.
Comment on attachment 8741907 [details]
MozReview Request: Bug 1262645 - Address misc issues with nsGetUserCertChoice(). r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46825/diff/1-2/
Attachment #8741907 - Attachment description: MozReview Request: Bug 1262645 - Address misc issues with nsGetUserCertChoice(). → MozReview Request: Bug 1262645 - Address misc issues with nsGetUserCertChoice(). r=keeler
https://hg.mozilla.org/mozilla-central/rev/313b65f1fc52
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.