1294811 - Move certificate shimming and AddonManager restarts to shared helper module

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Robert Helmer [:rhelmer]]

Currently we have to test some things over in `toolkit/mozapps/extensions/tests` because that's the only place that certificate shimming works.

Things like bug 1279012 (which is mostly about the webextension API) could potentially be done in `toolkit/components/extensions` instead (we'd also need a way to restart AddonManager, incidentally.)

Comment 1

[Kris Maglione [:kmag]]

This has been coming up a lot lately. It should probably be done before bug 1294287, since it would make a lot of things simpler.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1294811: Move AddonManager test helpers to a shared test module.

Most of the AddonTestUtils code is simply moved from head_addons.js, but I
did significantly refactor some of the especially crufty parts.

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

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8780883 [details]
Bug 1294811: Move AddonManager test helpers to a shared test module.

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

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8780883 [details]
Bug 1294811: Move AddonManager test helpers to a shared test module.

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

Comment 5

[Robert Helmer [:rhelmer]]

Comment on attachment 8780883 [details]
Bug 1294811: Move AddonManager test helpers to a shared test module.

https://reviewboard.mozilla.org/r/71452/#review69456

It's difficult to thoroughly review a patch this large that moves so much existing code, but I really like the approach and I think it'll be a huge improvement. Noticed a few things but they were in the existing code, I see you have cleaned things up as you went so included these.

I don't see any problem from visual inspection so r+, but make sure you do a full try run :)

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:101
(Diff revision 3)
> + * @param {string} str
> + *        The string to escape.
> + * @return {string} The escaped string.
> + */
> +function escapeXML(str) {
> +  let replacements = {"&": "&amp;", '"': "&quot;", "<": "&lt;", ">": "&gt;"};

Should we replace for `&apos;` too?

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:130
(Diff revision 3)
> +
> +  return result.join("");
> +}
> +
> +
> +class AddonsList {

Nice to see classes

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:183
(Diff revision 3)
> +    var xpiPath = dir.clone();
> +    xpiPath.append(id + ".xpi");
> +
> +    return this[type].some(file => {
> +      if (!file.exists())
> +        throw new Error("Non-existant path found in extensions.ini: " + file.path)

s/existant/existent/

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:328
(Diff revision 3)
> +   * @param {Promise} promise
> +   *        The promise to wait on.
> +   * @returns {*} The promise's resolution value.
> +   * @throws The promise's rejection value, if it rejects.
> +   */
> +  awaitPromise(promise) {

I really like the `await` nomenclature vs. `loopUntil`

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:776
(Diff revision 3)
> +   */
> +  createTempXPIFile(files) {
> +    var file = this.tempDir.clone();
> +    file.append("foo.xpi");
> +    do {
> +      file.leafName = Math.floor(Math.random() * 1000000) + ".xpi";

I wonder why this doesn't just use a UUID.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8780883 [details]
Bug 1294811: Move AddonManager test helpers to a shared test module.

https://reviewboard.mozilla.org/r/71452/#review69456

Yeah, sorry about that.

I at least tried to keep the layout of `head_addons.js` a similar as possible, so it was obvious what changed there. I thought about `hg cp`ing `head_addons.js` to `AddonTestUtils.jsm` so it would be diff-able, but I figured that as long as we were doing this kind of huge refactor, it would be a good opportunity to untangle some of the years of accumulated cruft. So in the end, I don't think the diff would have been that helpful.

> Should we replace for `'` too?

Yeah, leaving it out seems like a footgun waiting to happen.

> I wonder why this doesn't just use a UUID.

Good idea.

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1294811: Fix external tests that import head_addons.js.

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

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8780883 [details]
Bug 1294811: Move AddonManager test helpers to a shared test module.

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

Comment 9

[Robert Helmer [:rhelmer]]

Comment on attachment 8782122 [details]
Bug 1294811: Fix external tests that import head_addons.js.

https://reviewboard.mozilla.org/r/72362/#review70328

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23923c9bee18945e943151b7fb77f1f17febe0c5
Bug 1294811: Move AddonManager test helpers to a shared test module. r=rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9313a026f954a5a0553f8226adc7fa7b6de22cf
Bug 1294811: Fix external tests that import head_addons.js. r=rhelmer

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23923c9bee18
https://hg.mozilla.org/mozilla-central/rev/e9313a026f95