1245376 - Hello Beta appears in Add-on Available Updates

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[custom.firefox.lady [:tawn]]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160202004008

Steps to reproduce:

1. Have automatic Add-on Updates turned off
2. Add-ons Manager > Check for Updates



Actual results:

Firefox Hello Beta0.2.0 appears as an available update in the Add-ons Mgr. even though it is not listed in the extension's category.


Expected results:

I would expect that either Hello appears in the Add-ons Mgr. extensions category, or that its updates also be hidden.

Comment 1

[Ian Bicking (:ianbicking)]

Dave: what is the expected behavior here?  (Note we have the addon published to https://addons.mozilla.org/en-US/firefox/addon/firefox-hello-beta/ )

Comment 2

[Dave Townsend [:mossop]]

That shouldn't be happening, we should exclude system add-ons from the manual update check as we do from the automatic update check.

Comment 3

[Robert Helmer [:rhelmer]]

Attachment: Screen Shot 2016-02-09 at 12.59.27 AM.png

I can reproduce this on Mac Nightly 47.0a1 (2016-02-07).

STR:

1) load about:addons
2) disable "Update Add-ons Automatically"
3) install older add-on version such as https://addons.mozilla.org/firefox/downloads/file/384462/ublock_origin-1.5.4-sm+tb+an+fx.xpi?src=version-history
4) "Check for Updates"

"Available Updates" appears in about:addons sidebar, and shows Hello (see screenshot). There is a working "Disable" button, but no actual update appears to be available or install.

Comment 4

[Robert Helmer [:rhelmer]]

Note that if you press "Disable", complete manual updates, and switch off of the "Available Updates" entry so that it is dismissed, then it's not possible to re-enable Hello via the UI.

From a quick look I think it's coming from here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#3569

Should AddonManager.getAllInstalls() be returning system add-ons, or should callers be filtering these out?

Comment 5

[Dave Townsend [:mossop]]

(In reply to Robert Helmer [:rhelmer] from comment #4)
> Note that if you press "Disable", complete manual updates, and switch off of
> the "Available Updates" entry so that it is dismissed, then it's not
> possible to re-enable Hello via the UI.
> 
> From a quick look I think it's coming from here:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> content/extensions.js#3569
> 
> Should AddonManager.getAllInstalls() be returning system add-ons, or should
> callers be filtering these out?

Filtering sounds nice but that would mean breaking about:support and telemetry so I'd like to avoid that. Instead I think we should make the code that actually finds new updates skip over the cases where the install location is locked (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#6443) and also the front-end manual update check and the available updates page should skip add-ons that don't have PERM_CAN_UPGRADE in the permissions field.

Comment 6

[Worcester12345]

(In reply to custom.firefox.lady from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.1; rv:46.0) Gecko/20100101 Firefox/46.0
> Build ID: 20160202004008
> 
> Steps to reproduce:
> 
> 1. Have automatic Add-on Updates turned off
> 2. Add-ons Manager > Check for Updates
> 
> 
> 
> Actual results:
> 
> Firefox Hello Beta0.2.0 appears as an available update in the Add-ons Mgr.
> even though it is not listed in the extension's category.
> 
> 
> Expected results:
> 
> I would expect that either Hello appears in the Add-ons Mgr. extensions
> category, or that its updates also be hidden.

I would prefer to not have this at all. Sure, offer it as a suggestion when looking for Addons, but that is it. This is like the Thunderbird "Test Pilot" thing.

Comment 7

[Tony Mechelynck [:tonymec]]

(In reply to Worcester12345 from comment #6)
> I would prefer to not have this at all. Sure, offer it as a suggestion when
> looking for Addons, but that is it. This is like the Thunderbird "Test
> Pilot" thing.

I would prefer to see everything installed, even system add-ons, but I'm an ux-control fanatic (often at the expense of ux-minimalism even) and YMMV. Happily there is https://addons.mozilla.org/addon/disable-hello-pocket-reader/ (which lets the user enable or disable them one by one) for people like me.

Comment 8

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: Bug 1245376 - do not show hidden add-ons on manual update page f?mossop

The public method hidden() is checking that the installLocation is not
for system or temporary add-ons - I don't see anywhere that locked install
locations are exposed in the public API, do you think this is sufficient for
this bug or should locked install locations be exposed?

I think that `extensions.js` is already skipping update checks for add-ons
without PERM_CAN_UPGRADE in e.g. https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1049 but correct me if I am mistaken.

The trickier part here is adding a test - providers are mocked in the
mochitests, I expect we just need to mock this as well, thought I'd ask for
feedback on the above while I work this out.

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

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 8719070 [details]
MozReview Request: Bug 1245376 - do not show hidden add-ons on manual update page f?mossop

https://reviewboard.mozilla.org/r/34867/#review31721

Looks good. What I was trying to say was that we should make the internal code that actually does the update check just always say there are no updates available for add-ons in locked locations. That is here: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#6448. That way if we have bad callers for the API they don't see unusable updates.

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Dave Townsend [:mossop] from comment #9)
> Comment on attachment 8719070 [details]
> MozReview Request: Bug 1245376 - do not show hidden add-ons on manual update
> page f?mossop
> 
> https://reviewboard.mozilla.org/r/34867/#review31721
> 
> Looks good. What I was trying to say was that we should make the internal
> code that actually does the update check just always say there are no
> updates available for add-ons in locked locations. That is here:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#6448. That way if we have bad callers for the API
> they don't see unusable updates.

Ah, thanks for clarifying. Yes I think this makes more sense than patching all callers, I see others in DXR and it would be better not to worry about these.

Comment 11

[Mark Banner (:standard8)]

Dave/Rob, I think we're seeing a few people hitting this and we're only on beta, should we be pushing it into releases a bit faster?

Comment 12

[Robert Helmer [:rhelmer]]

(In reply to Mark Banner (:standard8) from comment #11)
> Dave/Rob, I think we're seeing a few people hitting this and we're only on
> beta, should we be pushing it into releases a bit faster?

comment 4 makes me worry about this one, someone could accidentally disable Hello and not be able to re-enable via the UI.

I can take this if nobody steps up to get it today (I noticed we have [good first bug] and a mentor set) - if someone does appear soon I'd be happy to help with reviewing too.

Comment 13

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #10)
> (In reply to Dave Townsend [:mossop] from comment #9)
> > Comment on attachment 8719070 [details]
> > MozReview Request: Bug 1245376 - do not show hidden add-ons on manual update
> > page f?mossop
> > 
> > https://reviewboard.mozilla.org/r/34867/#review31721
> > 
> > Looks good. What I was trying to say was that we should make the internal
> > code that actually does the update check just always say there are no
> > updates available for add-ons in locked locations. That is here:
> > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> > internal/XPIProvider.jsm#6448. That way if we have bad callers for the API
> > they don't see unusable updates.
> 
> Ah, thanks for clarifying. Yes I think this makes more sense than patching
> all callers, I see others in DXR and it would be better not to worry about
> these.

Hm, skipping sending the onUpdateAvailable event doesn't seem to be enough, Hello still shows up.

One note about reproducing this - the Add-on Update Check timer must have fired, you can force it to with https://addons.mozilla.org/en-US/firefox/addon/timer-fire

Another odd thing is that Pocket isn't showing up here too. I'll dig into this a bit more.

Comment 14

[Ian Bicking (:ianbicking)]

Note in terms of impact that we're seeing user consternation about this specific problem

Comment 15

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: Bug 1245376 - do not advertise updates for locked install locations r?mossop

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

Comment 16

[Robert Helmer [:rhelmer]]

Comment on attachment 8724981 [details]
MozReview Request: Bug 1245376 - do not advertise updates for locked install locations r?mossop

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

Comment 17

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/37243/#review33813

::: toolkit/mozapps/extensions/test/xpcshell/test_update.js:1380
(Diff revision 1)
> +      onCompatibilityUpdateAvailable: function() {

Hm, should I test that these still fire? I think you mentioned in IRC that we still get compatibility info for addons in locked locations.

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 8724981 [details]
MozReview Request: Bug 1245376 - do not advertise updates for locked install locations r?mossop

https://reviewboard.mozilla.org/r/37243/#review34185

You still need to call sendUpdateAvailableMessages so the final events are sent to listeners.

Comment 19

[Robert Helmer [:rhelmer]]

Comment on attachment 8724981 [details]
MozReview Request: Bug 1245376 - do not advertise updates for locked install locations r?mossop

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

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8724981 [details]
MozReview Request: Bug 1245376 - do not advertise updates for locked install locations r?mossop

https://reviewboard.mozilla.org/r/37243/#review34777

Comment 21

[Robert Helmer [:rhelmer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8988d0afbfb5

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/979162c452db

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/979162c452db

Comment 24

[Worcester12345]

Confirming this ridiculous add-on does not show up any longer. 

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 ID:20160303134406 CSet: b6609650a911cbc5267fe8c34f6a8071599d6205

Thank you.

Comment 25

[Mark Banner (:standard8)]

Rob, Dave, can we get this uplifted to 46/47 please? I want to offer newer versions on AMO compatible with them, but I'm a bit concerned users will pick them up accidentally with this out there.

Comment 26

[Dave Townsend [:mossop]]

Comment on attachment 8724981 [details]
MozReview Request: Bug 1245376 - do not advertise updates for locked install locations r?mossop

Approval Request Comment
[Feature/regressing bug #]: System add-ons
[User impact if declined]: Users may see misleading messages about updates for system add-ons
[Describe test coverage new/current, TreeHerder]: On m-c with good automated tests
[Risks and why]: Low, this is a very simple fix in code with good tests
[String/UUID change made/needed]: None

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8724981 [details]
MozReview Request: Bug 1245376 - do not advertise updates for locked install locations r?mossop

Seems like a fix that is needed in support of system add-ons like Hello. Aurora47+, Beta46+

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

custom.firefox.lady, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 29

[custom.firefox.lady [:tawn]]

Confirming issue appears to be fixed on 48.0a1 2016-03-16
Hello Beta not showing in available updates even after using Timer Fire (as mentioned in comment #13)

Comment 30

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee1c8ed1e4b6

Comment 31

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4d07b9c0fcc9

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to custom.firefox.lady from comment #29)
> Confirming issue appears to be fixed on 48.0a1 2016-03-16
> Hello Beta not showing in available updates even after using Timer Fire (as
> mentioned in comment #13)

Awesome! Thank you so much for a prompt verification. :)

Comment 33

[Kim]

Hi,
I just had the same issue and unfortunately I clicked on agree to update Hello Beta. Is there a possibility to cancel the update through about:config (I haven't restarted FF yet)? I am using FF ESR 45.3, when will it be fixed with ESR version?

Comment 34

[Mark Banner (:standard8)]

(In reply to Kim from comment #33)
> Hi,
> I just had the same issue and unfortunately I clicked on agree to update
> Hello Beta. Is there a possibility to cancel the update through about:config
> (I haven't restarted FF yet)? I am using FF ESR 45.3, when will it be fixed
> with ESR version?

Hmm, I can't actually reproduce this at the moment using ESR. I have, however, changed the min version of the latest Hello add-on to 46.0a1. Note that the latest version should uninstall itself anyway, though obviously we don't want this recurring.

Rob, do you think we could uplift this patch to ESR so that we can then enable the latest version of the Hello Beta add-on for 45 and hence drop any users who have it accidentally installed?

Comment 35

[Robert Helmer [:rhelmer]]

Attachment: ESR uplift request

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Users may see misleading messages about updates for system add-ons
Fix Landed on Version: 46
Risk to taking this patch (and alternatives if risky): Low, this is a very simple fix in code with good tests.
String or UUID changes made by this patch: None

Comment 36

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

We don't generally uplift patches to ESR if they aren't for critical security issues. 
Has something changed recently to make this start happening for ESR users?

Comment 37

[Robert Helmer [:rhelmer]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #36)
> We don't generally uplift patches to ESR if they aren't for critical
> security issues. 
> Has something changed recently to make this start happening for ESR users?

I don't think this is particularly severe but I'll let Mark answer.

Comment 38

[Mark Banner (:standard8)]

I was hoping to get the Hello removal out to a few more folks. However, given where things are, this is fine to skip.

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8792574 [details]
ESR uplift request

Just like Liz said