1464520 - avoid a localization dependency in nsNSSComponent by hard-coding the builtin root module name

Status: RESOLVED FIXED

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

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Historically, part of what has made nsNSSComponent initialization difficult is the localized string bundles nsNSSComponent creates and holds on to. Eventually it would be best to not need these at all. Currently, we localize the name of the builtin roots module. It's fairly easy to remove this dependency - we can just hard-code an internal name that we map to the localized version whenever we display it to the user (of course, we have to reverse the mapping when going the other direction).

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: bug 1464520 - hard-code the builtin roots module name to avoid a dependency on l10n in nsNSSComponent

nsNSSComponent startup and shutdown would be simpler if there were no direct
dependencies on localized strings. This patch removes a dependency on the
localized name of the builtin roots module by hard-coding the name internally
and then mapping it to/from the localized version as appropriate.

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

Comment 2

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8980775 [details]
bug 1464520 - hard-code the builtin roots module name to avoid a dependency on l10n in nsNSSComponent

https://reviewboard.mozilla.org/r/246960/#review253276

lgtm, except for that one nit.

::: security/manager/ssl/nsPKCS11Slot.cpp:196
(Diff revision 1)
> +// must map to the localized version when we display it to the user.
> +static nsresult
> +NormalizeModuleNameOut(const char* moduleNameIn, nsACString& moduleNameOut)
> +{
> +  // Easy case: this isn't the builtin roots module.
> +  if (strcmp(kRootModuleName, moduleNameIn) != 0) {

I don't like `strcmp`, how about `strncmp`? :)

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8980775 [details]
bug 1464520 - hard-code the builtin roots module name to avoid a dependency on l10n in nsNSSComponent

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

Comment 4

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8980775 [details]
bug 1464520 - hard-code the builtin roots module name to avoid a dependency on l10n in nsNSSComponent

https://reviewboard.mozilla.org/r/246960/#review253710

LGTM w/ nit addressed. Nice cleanup!

::: security/manager/ssl/nsPKCS11Slot.cpp:196
(Diff revision 2)
> +// must map to the localized version when we display it to the user.
> +static nsresult
> +NormalizeModuleNameOut(const char* moduleNameIn, nsACString& moduleNameOut)
> +{
> +  // Easy case: this isn't the builtin roots module.
> +  if (strlen(moduleNameIn) != kRootModuleNameLen ||

`strnlen(moduleNameIn, kRootModuleNameLen) != kRootModuleNameLen ||` maybe?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8980775 [details]
bug 1464520 - hard-code the builtin roots module name to avoid a dependency on l10n in nsNSSComponent

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

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8980775 [details]
bug 1464520 - hard-code the builtin roots module name to avoid a dependency on l10n in nsNSSComponent

https://reviewboard.mozilla.org/r/246960/#review253710

> `strnlen(moduleNameIn, kRootModuleNameLen) != kRootModuleNameLen ||` maybe?

strnlen(moduleNameIn, kRootModuleNameLen + 1) as discussed on irc.

Comment 7

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8980775 [details]
bug 1464520 - hard-code the builtin roots module name to avoid a dependency on l10n in nsNSSComponent

https://reviewboard.mozilla.org/r/246960/#review253918

shipt it!

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thanks! Here's try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9a2aeb90d65968670879c51bf6fbea58e61bfae

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9132bdb0177
hard-code the builtin roots module name to avoid a dependency on l10n in nsNSSComponent r=fkiefer,jcj

Comment 10

[Andreea Pavel [:apavel]]

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