Last Comment Bug 760356 - Only show the add-on compatibility UI when actually necessary
: Only show the add-on compatibility UI when actually necessary
Status: VERIFIED FIXED
p=0 s=it-32c-31a-30b.2 [qa-]
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
-- normal with 3 votes (vote)
: mozilla32
Assigned To: :Irving Reid (No longer working on Firefox)
: Catalin Varga [QA][:VarCat]
: Andy McKay [:andym]
Mentors:
: 787699 849473 903769 (view as bug list)
Depends on: 903769 1010449 1015892
Blocks: 890100 921558 960597 996125
  Show dependency treegraph
 
Reported: 2012-05-31 21:21 PDT by Dave Townsend [:mossop]
Modified: 2014-05-26 03:03 PDT (History)
21 users (show)
mmucci: firefox‑backlog+
irving: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WiP Part 1 - v1 (24.85 KB, patch)
2014-01-14 13:40 PST, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details | Diff | Splinter Review
WiP Part 2 - v1 (1.08 KB, patch)
2014-01-14 13:41 PST, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details | Diff | Splinter Review
WIP: Only interrupt startup for addon compat check if necessary (195.08 KB, patch)
2014-04-30 20:28 PDT, :Irving Reid (No longer working on Firefox)
blair: feedback+
Details | Diff | Splinter Review
part 1 - Modify AddonRepo.repopulateCache() to always repopulate all add-ons (21.77 KB, patch)
2014-05-12 09:56 PDT, :Irving Reid (No longer working on Firefox)
blair: review-
Details | Diff | Splinter Review
Part 2 - Only show the update check UI when necessary (198.44 KB, patch)
2014-05-12 09:59 PDT, :Irving Reid (No longer working on Firefox)
blair: review+
Details | Diff | Splinter Review
part 1 - Modify AddonRepo.repopulateCache() to always repopulate all add-ons (comments addressed) (17.75 KB, patch)
2014-05-14 10:26 PDT, :Irving Reid (No longer working on Firefox)
blair: review+
Details | Diff | Splinter Review
Part 2 - Only show the update check UI when necessary (updated for part 1 changes) (194.90 KB, patch)
2014-05-14 10:29 PDT, :Irving Reid (No longer working on Firefox)
blair: review+
Details | Diff | Splinter Review
Part 2 - Only show the update check UI when necessary (telemetry split out) (193.39 KB, patch)
2014-05-19 17:07 PDT, :Irving Reid (No longer working on Firefox)
blair: review+
Details | Diff | Splinter Review

Description User image Dave Townsend [:mossop] 2012-05-31 21:21:10 PDT
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 1 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2012-09-10 07:48:16 PDT
*** Bug 787699 has been marked as a duplicate of this bug. ***
Comment 2 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2012-09-10 07:51:55 PDT
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 3 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2013-03-10 01:03:24 PST
*** Bug 849473 has been marked as a duplicate of this bug. ***
Comment 4 User image Florian Bender 2013-08-10 09:53:46 PDT
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 5 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2013-08-12 02:03:44 PDT
*** Bug 903769 has been marked as a duplicate of this bug. ***
Comment 6 User image Tony Mechelynck [:tonymec]. (NEEDINFO me if you want my attention) 2013-11-08 17:42:40 PST
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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2014-01-14 13:40:58 PST
Created attachment 8360021 [details] [diff] [review]
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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2014-01-14 13:41:25 PST
Created attachment 8360022 [details] [diff] [review]
WiP Part 2 - v1
Comment 9 User image :Irving Reid (No longer working on Firefox) 2014-04-10 14:26:46 PDT
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 User image :Irving Reid (No longer working on Firefox) 2014-04-11 05:48:57 PDT
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 User image Dave Townsend [:mossop] 2014-04-11 08:17:38 PDT
(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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2014-04-13 03:02:27 PDT
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 User image :Irving Reid (No longer working on Firefox) 2014-04-30 20:28:39 PDT
Created attachment 8415753 [details] [diff] [review]
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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2014-04-30 21:03:00 PDT
(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 User image :Irving Reid (No longer working on Firefox) 2014-05-02 12:02:00 PDT
Why *do* we exclude the hotfix from the metadata ping?
Comment 16 User image :Irving Reid (No longer working on Firefox) 2014-05-12 09:56:59 PDT
Created attachment 8421092 [details] [diff] [review]
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 User image :Irving Reid (No longer working on Firefox) 2014-05-12 09:59:55 PDT
Created attachment 8421099 [details] [diff] [review]
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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2014-05-12 17:02:28 PDT
(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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2014-05-12 17:51:17 PDT
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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2014-05-12 18:19:30 PDT
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 User image :Irving Reid (No longer working on Firefox) 2014-05-14 10:26:52 PDT
Created attachment 8422562 [details] [diff] [review]
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 User image :Irving Reid (No longer working on Firefox) 2014-05-14 10:29:50 PDT
Created attachment 8422567 [details] [diff] [review]
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 User image :Irving Reid (No longer working on Firefox) 2014-05-14 10:30:09 PDT
https://tbpl.mozilla.org/?tree=Try&rev=4867f8bcec12
Comment 24 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2014-05-14 20:46:54 PDT
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 User image :Irving Reid (No longer working on Firefox) 2014-05-19 17:07:05 PDT
Created attachment 8425150 [details] [diff] [review]
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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 2014-05-20 02:28:13 PDT
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 29 User image juan becerra [:juanb] 2014-05-21 08:58:39 PDT
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-05-21 09:44:33 PDT
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 User image :Irving Reid (No longer working on Firefox) 2014-05-22 05:34:03 PDT
Manual testing would involve a fairly complex set up; based on an IRC conversation with Blair, we're comfortable with the automated tests.

Note You need to log in before you can comment on or make changes to this bug.