760356 - Only show the add-on compatibility UI when actually necessary

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Since D2C landed there are a number of cases where we show the compatibility UI but it wouldn't actually be necessary.

I think there are a few classes of add-ons:

Normal add-ons with no binary components, they are going to be compatible unless the blacklist says not.

Add-ons with binary components will either be compatible or not based on the install.rdf. It shouldn't be possible to bump that compatible in update.rdf because the binary component will still be bad.

Add-ons that opt out of d2c.

Unless I'm forgetting something the only case where we would need to show this UI these days is when either our D2C blacklist is old or the user has add-ons of the latter type that seem incompatible without an update ping. I bet the prevalence of the latter type is extremely small, and assuming most users use Firefox reasonably often the blacklist should be quite up to date, so making those changes would probably leave us almost never showing this UI.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

As bought up in bug 787699, can skip over disabled addons too. 

And *possibly* blocklisted addons that were already disabled by the blocklist before the update, since not fixing them isn't a sudden loss of functionality at the time of app update.

Comment 4

[Florian Bender]

I filed Bug 903769 prematurely. 

However, this bug can be turned into a meta bug, as there are a few different tasks to do here, AFAIUC.

Comment 6

[Tony Mechelynck [:tonymec]]

Hmmm… The following used to be (and, ATM, still is) a way to force SeaMonkey to check if there wasn't a new version of ChatZilla, Venkman, DOMi and/or DebugQA included in {installdir}/distribution/extensions/ with a newly downloaded Trunk or Aurora nightly, and if yes, install it in the current profile:

1. In prefs.js (or in about:config before closing the previous nightly), change the value of extensions.lastAppVersion to make it end in a0 rather than a1 or a2, thus faking an "earlier" app version.
2. (Only after that) install and run the new nightly.

Not as a support question (the above hack is hardly a "normal" procedure), just for my own edification: if and when this bug gets FIXED, I guess the above procedure won't work anymore?

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP Part 1 - v1

Old work in progress patch. Been ages since I've been able to look at this. Can't remember exactly what state it's in. Also, for some reason I have a part-1 and a part-2... and I have no idea what's in either. Go me!

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP Part 2 - v1

Comment 9

[:Irving Reid (No longer working on Firefox)]

How much do we care about the "Select add-ons" UI (content/extensions/selectAddons.xul and friends)? It was added in bug 596343 to give users a once-only chance to purge undesired add-ons from existing profiles.

If we want to keep it, we'll need to refactor some of your WIP to show it even when we aren't disabling add-ons. If we don't, we probably want a separate bug to rip it out as a blocker for this bug...

Comment 10

[:Irving Reid (No longer working on Firefox)]

I could easily add a telemetry probe to count how often we show selectAddons.xul, but I'd want Dave or Blair to tell me it was worth the effort...

Comment 11

[Dave Townsend [:mossop]]

(In reply to :Irving Reid from comment #10)
> I could easily add a telemetry probe to count how often we show
> selectAddons.xul, but I'd want Dave or Blair to tell me it was worth the
> effort...

I don't want to pre-empt Blair here. My take is that the number of users seeing this UI should be miniscule so we should probably drop the code to show it for now. There have been persistent suggestions that we show it again but if that happens we'll no doubt be making some changes to it and how we show it anyway.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ugh, that thing is a PITA to maintain these days, given what minuscule (or borrow Dave's term) benefit we get from it these days. So +1 to what Dave said in comment 11. But I hate having code lying around that's unmaintained/unsupported - leads to all sorts of trouble. So I'd rather just remove it from the tree entirely. If we end up wanting to resurrect it based on the old code, we can delve into VCS history.

Comment 13

[:Irving Reid (No longer working on Firefox)]

Attachment: WIP: Only interrupt startup for addon compat check if necessary

This started from a merge of Blair's two patches, updated for bit rot, and then I fixed all the existing tests to reflect the new behaviour expected.

After that, I added a set of tests to make sure we do or don't show the dialog correctly based on the saved time of last metadata update, and added telemetry probes for how many add-ons are updated (or failed, or deliberately not updated) by the start-up dialog.

The main open question I have is: The AddonRepository database gets discarded and re-loaded any time a client requests metadata from AMO, but that metadata isn't necessarily for all add-ons; the APIs allow a caller to ask for a subset. I think we should only record a "metadata updated" event when we've requested info for *all* add-ons, not a subset. I'll need to check around a bit, but the most likely place is to record that is probably in the background update check, though we'll also want it to happen if bug 890100 gets add-on metadata during the app update, as I expect it will.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Irving Reid from comment #13)
> The main open question I have is: The AddonRepository database gets
> discarded and re-loaded any time a client requests metadata from AMO, but
> that metadata isn't necessarily for all add-ons; the APIs allow a caller to
> ask for a subset. I think we should only record a "metadata updated" event
> when we've requested info for *all* add-ons, not a subset. I'll need to
> check around a bit, but the most likely place is to record that is probably
> in the background update check, though we'll also want it to happen if bug
> 890100 gets add-on metadata during the app update, as I expect it will.

Maybe we should turn that on it's head: Is there any case where we actually use this/need to use this (need to only ask for a subset)? Or is it just an unneeded footgun?

Off the top of my head, I can only think of one useful case where we use a subset - and that's to exclude the hotfix add-on, which we intend to always do. That can just be done within AddonRepository itself.

Comment 15

[:Irving Reid (No longer working on Firefox)]

Why *do* we exclude the hotfix from the metadata ping?

Comment 16

[:Irving Reid (No longer working on Firefox)]

Attachment: part 1 - Modify AddonRepo.repopulateCache() to always repopulate all add-ons

Based on the discussion up to comment 14, here's a patch that removes the passed-in list of addon IDs to AddonRepository.repopulateCache. Because existing clients of this API always want to have a list of all add-ons aside from the hotfix, and repopulateCache happens to build such a list, I resolve the promise returned by repopulateCache with the list of add-ons.

Comment 17

[:Irving Reid (No longer working on Firefox)]

Attachment: Part 2 - Only show the update check UI when necessary

Updated to use the new AddonRepo.repopulateCache() API and a bit more test cleanup. Try run of both patches at https://tbpl.mozilla.org/?tree=Try&rev=25f89874dfc2

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Irving Reid from comment #15)
> Why *do* we exclude the hotfix from the metadata ping?


From Dave in bug 694068 comment 5:
> (In reply to Blair McBride (:Unfocused) from comment #3)
> > Is it useful for AddonRepository to get & cache metadata for hotfixes? Will
> > there ever be any useful metadata for hotfixes?
> 
> Probably not much use nor any harm I think.

And Dave now points out that changing it now would have some impact on metrics - so simplest road is to just keep the status quo, IMO.

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8421092 [details] [diff] [review]
part 1 - Modify AddonRepo.repopulateCache() to always repopulate all add-ons

Review of attachment 8421092 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +1163,3 @@
>          // Repopulate repository cache first, to ensure compatibility overrides
>          // are up to date before checking for addon updates.
> +        let allAddons = yield AddonRepository.backgroundUpdateCheck();

Hm, I don't like the strong coupling here. From the point of view of AddonRepository.backgroundUpdateCheck(), it doesn't make sense for it to be returning all add-ons, because that's not what it's operating on (it'll filter some out, and eventually we should make it so it only fetches data for specific types).

Calling this.getAllAddons() a second time is cheap these days, so I don't think there's a downside of explicitly fetching all add-ons here.

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm
@@ +632,3 @@
>  
> +    // Filter the hotfix out of our list of add-ons
> +    let allAddons = [a for (a of allAddons) if (a.id != hotfixID)];

Should just use AddonManager.hotfixID

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js
@@ +26,5 @@
>  Components.utils.import("resource://gre/modules/Promise.jsm");
>  Components.utils.import("resource://gre/modules/Task.jsm");
>  Components.utils.import("resource://gre/modules/osfile.jsm");
>  
> +// Services.prefs.setBoolPref("toolkit.osfile.log", true);

Leftover?

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8421099 [details] [diff] [review]
Part 2 - Only show the update check UI when necessary

Review of attachment 8421099 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js
@@ +27,5 @@
>  Components.utils.import("resource://gre/modules/Task.jsm");
>  Components.utils.import("resource://gre/modules/osfile.jsm");
>  
>  // Services.prefs.setBoolPref("toolkit.osfile.log", true);
> +Services.prefs.setBoolPref("toolkit.osfile.log", false);

Erm?

Comment 21

[:Irving Reid (No longer working on Firefox)]

Attachment: part 1 - Modify AddonRepo.repopulateCache() to always repopulate all add-ons (comments addressed)

(In reply to Blair McBride [:Unfocused] from comment #19)
> Comment on attachment 8421092 [details] [diff] [review]
> part 1 - Modify AddonRepo.repopulateCache() to always repopulate all add-ons
> 
> Review of attachment 8421092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/extensions/AddonManager.jsm
> @@ +1163,3 @@
> >          // Repopulate repository cache first, to ensure compatibility overrides
> >          // are up to date before checking for addon updates.
> > +        let allAddons = yield AddonRepository.backgroundUpdateCheck();
> 
> Hm, I don't like the strong coupling here. From the point of view of
> AddonRepository.backgroundUpdateCheck(), it doesn't make sense for it to be
> returning all add-ons, because that's not what it's operating on (it'll
> filter some out, and eventually we should make it so it only fetches data
> for specific types).
> Calling this.getAllAddons() a second time is cheap these days, so I don't
> think there's a downside of explicitly fetching all add-ons here.

Yes, this felt a bit smelly while I was writing it. Going back to fetching the add-ons separately made the patch a lot smaller...

> Should just use AddonManager.hotfixID

I was relocating code that didn't use that, but yes.

> ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js
> @@ +26,5 @@
> > +// Services.prefs.setBoolPref("toolkit.osfile.log", true);
> 
> Leftover?

Yes, reverted.

Comment 22

[:Irving Reid (No longer working on Firefox)]

Attachment: Part 2 - Only show the update check UI when necessary (updated for part 1 changes)

This needed significant changes to track the AddonRepo.repopulateCache change to part 1 that Blair requested in comment 19.

Comment 23

[:Irving Reid (No longer working on Firefox)]

https://tbpl.mozilla.org/?tree=Try&rev=4867f8bcec12

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8422567 [details] [diff] [review]
Part 2 - Only show the update check UI when necessary (updated for part 1 changes)

Review of attachment 8422567 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it! Finally! 

This bug has been a PITA... Thanks for pushing it through to completion, Irving :)

Comment 25

[:Irving Reid (No longer working on Firefox)]

Attachment: Part 2 - Only show the update check UI when necessary (telemetry split out)

Splitting the telemetry out into Bug 1010449 changed enough lines in this patch that I'd appreciate a quick re-review.

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8425150 [details] [diff] [review]
Part 2 - Only show the update check UI when necessary (telemetry split out)

Review of attachment 8425150 [details] [diff] [review]:
-----------------------------------------------------------------

You're melting my poor brain with these revisions ;)

Comment 27

[:Irving Reid (No longer working on Firefox)]

https://hg.mozilla.org/integration/fx-team/rev/60de42b54f91
https://hg.mozilla.org/integration/fx-team/rev/a1c18a7b7d39

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60de42b54f91
https://hg.mozilla.org/mozilla-central/rev/a1c18a7b7d39

Comment 29

[juan becerra [:juanb]]

Qa+'ing for a spot check, given this landed with tests, and cc'ing Henrik for a heads-up should our Mozmill tests need revision.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Irvine, is the test coverage we have enough here? Mind updating the in-testsuite flag appropriately? Thanks.

In regards of Mozmill we do not have a test, which specifically checks that.

Comment 31

[:Irving Reid (No longer working on Firefox)]

Manual testing would involve a fairly complex set up; based on an IRC conversation with Blair, we're comfortable with the automated tests.