1391703 - Introduce common JSM for security/manager/tools/ scripts

Status: REOPENED

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

Description

[:Cykesiopka]

PSM has various xpcshell scripts under the security/manager/tools/ folder. At the moment, these scripts:
  1. Duplicate code.
  2. Aren't testable.

We should introduce a common, testable JSM that these scripts can use.

Comment 1

[:Cykesiopka]

Attachment: Bug 1391703 - Introduce common JSM for security/manager/tools/ scripts.

PSM has various xpcshell scripts under the security/manager/tools/ folder. At
the moment, these scripts:
  1. Duplicate code.
  2. Aren't testable.

This patch introduces a common, unit tested JSM that these scripts can use.

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

Comment 2

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

Comment on attachment 8899222 [details]
Bug 1391703 - Introduce common JSM for security/manager/tools/ scripts.

https://reviewboard.mozilla.org/r/170484/#review176454

This is a great improvement in our situation - thanks!
I guess we don't really have a way to test if these changes will work in the automation infrastructure, but even if things do break there, it won't break the build (famous last words), so I think we can go ahead with this and just keep an eye on automation when it runs with this.

::: security/manager/moz.build:11
(Diff revision 1)
>  
>  with Files("**"):
>      BUG_COMPONENT = ("Core", "Security: PSM")
>  
> -DIRS += ['ssl', 'locales']
> +DIRS += [
> +  'locales',

nit: 4-space indentation here, if I understand correctly

::: security/manager/tools/tests/test_psmtoolutils.js:47
(Diff revision 1)
> +});
> +
> +add_task(function testDownloadFile_BadStatusCode() {
> +  let url = `${gHttpServerPrePath}/BadStatusCode`;
> +  throws(() => PSMToolUtils.downloadFile(url, true),
> +         /problem downloading.*status/,

Should we also add the expected status code (404)?

Comment 3

[:Cykesiopka]

Comment on attachment 8899222 [details]
Bug 1391703 - Introduce common JSM for security/manager/tools/ scripts.

https://reviewboard.mozilla.org/r/170484/#review176454

Thanks for the review.
Yeah, it's unfortunate we can't really test this on automation, but oh well.

> nit: 4-space indentation here, if I understand correctly

Yes, you're correct. Fixed.

> Should we also add the expected status code (404)?

Sounds good.

Comment 4

[:Cykesiopka]

Comment on attachment 8899222 [details]
Bug 1391703 - Introduce common JSM for security/manager/tools/ scripts.

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

Comment 5

[:Cykesiopka]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/121e4d470c11
Introduce common JSM for security/manager/tools/ scripts. r=keeler

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/121e4d470c11

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/56188620cce0
Backed out changeset 121e4d470c11 for breaking periodic HSTS/HPKP updates.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment 8 was at Cykesiopka's request. FWIW, the automated updates still didn't run on m-c today :\

Comment 10

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

Do we still want to do this?

Comment 11

[:Cykesiopka]

(In reply to Dana Keeler [:keeler] (use needinfo) from comment #10)
> Do we still want to do this?

Yes, but sadly landing this will break automation until Bug 1398829 is fixed.
I'll try and take another stab at fixing that bug soon, but for now P3 I guess.

Comment 12

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

Oh, right, now I remember. No worries anyway - we were just doing a bit of triage on P1/psm-assigned bugs.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.