Hello Beta appears in Add-on Available Updates

VERIFIED FIXED in Firefox 46

Status

()

defect
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: tawn, Assigned: rhelmer, Mentored)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox45 wontfix, firefox46 fixed, firefox47 fixed, firefox48 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(3 attachments, 1 obsolete attachment)

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.
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/ )
Flags: needinfo?(dtownsend)
That shouldn't be happening, we should exclude system add-ons from the manual update check as we do from the automatic update check.
Component: General → Add-ons Manager
Flags: needinfo?(dtownsend)
Product: Hello (Loop) → Toolkit
Whiteboard: [good first bug]
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.
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?
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
(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.
(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.
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/
Attachment #8719070 - Flags: feedback?(dtownsend)
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.
Attachment #8719070 - Flags: review+
(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.
Attachment #8719070 - Flags: review+
Attachment #8719070 - Flags: feedback?(dtownsend)
Attachment #8719070 - Flags: feedback+
Mentor: kmaglione+bmo
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?
Flags: needinfo?(rhelmer)
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(rhelmer)
Assignee: nobody → rhelmer
Status: NEW → ASSIGNED
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(stayopenmenu)
Flags: needinfo?(stayopenmenu)
Note in terms of impact that we're seeing user consternation about this specific problem
Attachment #8719070 - Attachment is obsolete: true
Attachment #8724981 - Attachment description: MozReview Request: Bug 1245376 - do not advertise updates for locked install locations → MozReview Request: Bug 1245376 - do not advertise updates for locked install locations r?mossop
Attachment #8724981 - Flags: review?(dtownsend)
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/
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 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.
Attachment #8724981 - Flags: review?(dtownsend)
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/
Attachment #8724981 - Flags: review?(dtownsend)
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
Attachment #8724981 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/979162c452db
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
Flags: needinfo?(rhelmer)
Flags: needinfo?(dtownsend)
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
Flags: needinfo?(rhelmer)
Flags: needinfo?(dtownsend)
Attachment #8724981 - Flags: approval-mozilla-beta?
Attachment #8724981 - Flags: approval-mozilla-aurora?
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+
Attachment #8724981 - Flags: approval-mozilla-beta?
Attachment #8724981 - Flags: approval-mozilla-beta+
Attachment #8724981 - Flags: approval-mozilla-aurora?
Attachment #8724981 - Flags: approval-mozilla-aurora+
custom.firefox.lady, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(stayopenmenu)
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)
Flags: needinfo?(stayopenmenu)
(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. :)
Status: RESOLVED → VERIFIED
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?
(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?
Flags: needinfo?(rhelmer)
Posted file 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
Flags: needinfo?(rhelmer)
Attachment #8792574 - Flags: approval-mozilla-esr45?
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?
Flags: needinfo?(standard8)
Flags: needinfo?(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.
Flags: needinfo?(rhelmer)
I was hoping to get the Hello removal out to a few more folks. However, given where things are, this is fine to skip.
Flags: needinfo?(standard8)
Comment on attachment 8792574 [details]
ESR uplift request

Just like Liz said
Attachment #8792574 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45-
You need to log in before you can comment on or make changes to this bug.