Closed Bug 1446948 Opened Last year Closed Last year

Unsigned langpacks are not displayed in “Languages”

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox59 --- unaffected
firefox60 blocking verified
firefox61 --- verified

People

(Reporter: cbadescu, Assigned: aswan)

References

Details

Attachments

(3 files)

[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.
If you set `xpinstall.signatures.required` to false, they display.
Assignee: nobody → aswan
Priority: -- → P1
Attachment #8961522 - Flags: review?(kmaglione+bmo)
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?
Attachment #8961522 - Flags: review?(kmaglione+bmo) → review+
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.
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e418952dd353
Fix presentation of unsigned langpacks in about:addons r=kmag
https://hg.mozilla.org/mozilla-central/rev/e418952dd353
Status: NEW → RESOLVED
Closed: Last year
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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
Attachment #8961522 - Flags: approval-mozilla-beta?
Attached image 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.
Status: RESOLVED → VERIFIED
Comment on attachment 8961522 [details]
Bug 1446948 Fix presentation of unsigned langpacks in about:addons

fix recent regression in about:addons, beta60+
Attachment #8961522 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This issue is verified as fixed on Firefox 60.0b7 (20180326164103) under Windows 7 64-bit and Mac OS 10.13.2.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.