1395892 - Some special mozilla extensions are still showing "Legacy" badge in their details page

Status: VERIFIED FIXED

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

Description

[Kestrel]

Attachment: shield-legacy-badge.png

Affected extensions encountered:
Shield Opt-out Nothing Test Study
ADB Helper
Valence

These extensions are not tagged as "Legacy" in the main listing but are in their details page.

Comment 1

[Andrew Swan [:aswan]]

:Osmose, are shield studies not getting signed with "Mozilla Extensions"?

Comment 2

[Osmose [:osmose, :mkelly]]

The Opt-out Nothing Test Study was signed with the Mozilla Extensions key by me using the instructions on Mana. The XPI used is still available at https://net-mozaws-prod-us-west-2-normandy.s3.amazonaws.com/extensions/shield-optout-nothing-study-signed.xpi.

I can confirm having observed the behavior of the Legacy badge on the details page but not on the listing with that XPI.

Comment 3

[Kestrel]

Attachment: adbhelper-valence-legacy.png

ADB Helper and Valence are still showing Legacy tag in details page.

Comment 4

[Kestrel]

Reopening since this has not been fixed and does not seem to be a problem with signing per Comment 2.

Comment 5

[Andy McKay]

Sole, are ADB Helper and Valence signed with "Mozilla Extensions" yet?

Comment 6

[Soledad Penades [:sole] [:spenades]]

Alex is the signer here, moving the needinfo to him! I'll ping him in irc too just in case

Comment 7

[Alexandre Poirot [:ochameau]]

Yes, they are.

But I think that's a bug in about:addons.
"Legacy" badge isn't displayed in the add-on list, but is still displayed when opening addon details.
I saw that behavior while testing the first signed add-ons.

so +1 on comment 0 and comment 2!

Comment 8

[Andrew Swan [:aswan]]

Ouch, this is a stupid typo in code that apparently isn't covered by unit tests :(

I'll put a patch up but I'm not sure what we should do, the legacy badges were really meant for users of pre-57 releases and 56 is headed to release very soon...

Comment 9

[Andrew Swan [:aswan]]

Attachment: Bug 1395892 Fix legacy badge in details view

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

Comment 10

[Andy McKay]

Comment on attachment 8909531 [details]
Bug 1395892 Fix legacy badge in details view

https://reviewboard.mozilla.org/r/180996/#review186226

lgtm

Comment 11

[Andy McKay]

Comment on attachment 8909531 [details]
Bug 1395892 Fix legacy badge in details view

https://reviewboard.mozilla.org/r/180998/#review186228

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acdf05e3a341
Fix legacy badge in details view r=andym

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8909531 [details]
Bug 1395892 Fix legacy badge in details view

(Note this request is meant for 56, I was told to use -release for that.  If this should be approval-request-beta instead please let me know or just adjust appropriately)

Approval Request Comment
[Feature/Bug causing the regression]:
This wasn't a regression, it apparently never actually worked properly.

[User impact if declined]:
"privileged" extensions that are not listed in the default exceptions list (http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/browser/app/profile/firefox.js#92) will have a bogus "LEGACY" badge next to them in the details page in about:addons.  This includes devtools extensions (Valence, ADB Helper) and Shield studies.

[Is this code covered by automated tests?]:
Partially.  The scenario in this bug isn't tested, but related functionality is tested in toolkit/mozapps/extensions/test/browser/browser_details.js so I'm fairly confident this patch doesn't introduce any unrelated regressions.

[Has the fix been verified in Nightly?]:
It was manually tested before landing but it hasn't actually landed yet.

[Needs manual test from QE? If yes, steps to reproduce]: 
STR: install one the extensions listed above, go to about:addons, click on "More" next to the extension to see the details page, there should not be a yellow "LEGACY" badge.

[List of other uplifts needed for the feature/fix]:
none

[Is the change risky?]:
no

[Why is the change risky/not risky?]:
The change is trivial.  The existing code was reading a non-existent property, here he just fix the property name.

[String changes made/needed]:
none

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei, can your team verify the fix in nightly? We can still uplift this to m-r for the 56 RC, but I'd like some testing to make sure it works as expected. thanks!

Comment 16

[Vasilica Tamas, QA [:vtamas]]

Attachment: Animation.gif

This issue seems to be fixed on Firefox 57.0a1 (20170919220202) under Windows 10 64-bit and Mac OS X 10.12.3 while using ADB Helper and Valence add-ons. “Legacy” badge is no longer displayed in add-ons details tab.
Screenshots:
  - https://drive.google.com/file/d/0B-KIyWoO4a-lVEtmNTdtdm52Yzg/view?usp=sharing
  - https://drive.google.com/file/d/0B-KIyWoO4a-lcWI4OU9ES1dQS28/view?usp=sharing

But, I was unable to verify this bug using Shield-optout-nothing-study add-on from Comment 2 because it breaks the Add-ons Manager. See attached screencast.
Any thoughts about this?

Comment 17

[Osmose [:osmose, :mkelly]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #16)
> Created attachment 8910290 [details]
> Animation.gif
> 
> This issue seems to be fixed on Firefox 57.0a1 (20170919220202) under
> Windows 10 64-bit and Mac OS X 10.12.3 while using ADB Helper and Valence
> add-ons. “Legacy” badge is no longer displayed in add-ons details tab.
> Screenshots:
>   -
> https://drive.google.com/file/d/0B-KIyWoO4a-lVEtmNTdtdm52Yzg/view?usp=sharing
>   -
> https://drive.google.com/file/d/0B-KIyWoO4a-lcWI4OU9ES1dQS28/view?usp=sharing
> 
> But, I was unable to verify this bug using Shield-optout-nothing-study
> add-on from Comment 2 because it breaks the Add-ons Manager. See attached
> screencast.
> Any thoughts about this?

Looks like a bug with that specific study, and not with this patch. I've filed bug 1401744 to cover that.

https://net-mozaws-prod-us-west-2-normandy.s3.amazonaws.com/extensions/safe-browsing-v4-crash-rate-test-study-signed.xpi is an alternative study signed with the same cert that can be used for testing instead. I tried it on Nightly and no longer see the Legacy badge on the details page.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Nice catch Vasilica! 
OK, then I'm considering we can take this patch for 56.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8909531 [details]
Bug 1395892 Fix legacy badge in details view

Fix verified on Nightly, the followup bug seems unrelated. 
So let's take this patch for the 56 RC2 build.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/d57a23a8d5dc

Comment 21

[Vasilica Tamas, QA [:vtamas]]

Attachment: Animation.gif

I confirm that this issue is fixed on Firefox 57.0a1 (20170921100141) and Firefox 56.0 (20170921234614) under Windows 10 64-bit and Mac OS X 10.12.3. “Legacy” badge is no longer displayed in add-on details tab for "Mozilla Extensions" signed add-ons.

Comment 22

[Worcester12345]

(In reply to Andrew Swan [:aswan] from comment #8)
> Ouch, this is a stupid typo in code that apparently isn't covered by unit
> tests :(
> 
> I'll put a patch up but I'm not sure what we should do, the legacy badges
> were really meant for users of pre-57 releases and 56 is headed to release
> very soon...

There is also a place where it says "Compatible with multiprocess." or "Not compatible with multiprocess."

Then the third (or is it the same) thing is whether or not it is a Webextension.

I suggest the about:addons page be broken into columns, with all the different compatibilities either checked or a hollow or filled circle to indicate where they fall in the matrix of possibilities. This matrix could also list author, website, email, version, etc., which is all missing from this view.