1446948 - Unsigned langpacks are not displayed in “Languages”

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[CosminB]

[Affected versions]:
- Firefox 61.0a1 (20180318220108)
- Firefox 60.0b4 (20180315232954)


[Affected platforms]:
- Win 7 64-bit
- Mac OS X 10.13.2

[Steps to reproduce]:
1.Set extensions.langpacks.signatures.required to false.
2.Install some unsigned langpacks.
3.Observe “Languages” from about:addons.

[Expected results]:
- The langpacks are displayed.

[Actual results]:
- The langpacks are not displayed but you can see in the extensions folder from the profile, that they are installed.

Comment 1

[CosminB]

Attachment: Unsigned langpacks are not displayed in “Languages”.gif

Comment 2

[Justin Wood (:Callek)]

If you set `xpinstall.signatures.required` to false, they display.

Comment 3

[Andrew Swan [:aswan]]

Attachment: Bug 1446948 Fix presentation of unsigned langpacks in about:addons

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

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8961522 [details]
Bug 1446948 Fix presentation of unsigned langpacks in about:addons

https://reviewboard.mozilla.org/r/230302/#review235904

::: toolkit/mozapps/extensions/content/extensions.js:235
(Diff revision 1)
> -  return AddonSettings.REQUIRE_SIGNING && !isCorrectlySigned(addon);
> +  let signingRequired = (addon.type == "locale") ?
> +                        AddonSettings.LANGPACKS_REQUIRE_SIGNING :
> +                        AddonSettings.REQUIRE_SIGNING;
> +  return signingRequired && !isCorrectlySigned(addon);

This is fine, I guess, but can we maybe add a `requiresSigning` property to the Addon instead, and re-use the mustSign logic in XPIProvider?

::: toolkit/mozapps/extensions/test/browser/browser_langpack_signing.js:8
(Diff revision 1)
> +// the Languages tab based on the langpack signing preference.
> +add_task(async function() {
> +  const PREF = "extensions.langpacks.signatures.required";
> +
> +  await SpecialPowers.pushPrefEnv({
> +    set: [[PREF, false]]

Nit: Trailling comma, please.

::: toolkit/mozapps/extensions/test/browser/browser_langpack_signing.js:80
(Diff revision 1)
> +  await SpecialPowers.pushPrefEnv({
> +    set: [[PREF, true]]
> +  });

This would really be better as two `withPrefEnv`s... but that apparently does not exist. It would probably worth adding... But can you at least pop the previous pref env before you push the new one?

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8961522 [details]
Bug 1446948 Fix presentation of unsigned langpacks in about:addons

https://reviewboard.mozilla.org/r/230302/#review235904

> This is fine, I guess, but can we maybe add a `requiresSigning` property to the Addon instead, and re-use the mustSign logic in XPIProvider?

I've been resisting making any big changes to how this stuff is propagated from the backing objects in the hope that we'll write something new from the ground up for Medley and then just burn all this code to the ground.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e418952dd353
Fix presentation of unsigned langpacks in about:addons r=kmag

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8961522 [details]
Bug 1446948 Fix presentation of unsigned langpacks in about:addons

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1444487

[User impact if declined]:
language packs don't show up in about:addons

[Is this code covered by automated tests?]:
yes

[Has the fix been verified in Nightly?]:
manually be the path author, yes

[Needs manual test from QE? If yes, steps to reproduce]: 
additional manual testing would be good, but can probably be lumped in with ongoing testing happening for bug 1444487 generally

[List of other uplifts needed for the feature/fix]:
this depends on 144487 of course, that has already been uploaded to beta

[Is the change risky?]:
not very

[Why is the change risky/not risky?]:
the change is relatively small, the new behavior is covered by a new test and there are a lot of other existing tests that should catch any other regressions.

[String changes made/needed]:
none

Comment 9

[CosminB]

Attachment: Bug1446948.gif

This issue is verified as fixed on Firefox 61.0a1 (20180322234402) under Windows 7 64-bit and Ubuntu 14.04 32-bit.

The unsigned langpacks are displayed in the “Languages” when extensions.langpacks.signatures.required is set to false.

Please see the attached video.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8961522 [details]
Bug 1446948 Fix presentation of unsigned langpacks in about:addons

fix recent regression in about:addons, beta60+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1b2c48ee942a

Comment 12

[CosminB]

This issue is verified as fixed on Firefox 60.0b7 (20180326164103) under Windows 7 64-bit and Mac OS 10.13.2.