Closed Bug 772484 Opened 12 years ago Closed 10 years ago

extension check dialog is annoying and can effectively hang the Firefox process

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jaas, Assigned: Irving)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Snappy:P1] c=startup_misc u= p=)

Attachments

(1 file, 3 obsolete files)

Sometimes when I launch Firefox I get an extension check dialog. That seems bad enough. But it's worse if I happen to be offline once the check starts because I can't cancel the check or even quit the browser. This is effectively a hung process and I have to quit the browser.
I meant to say that I have to force-quit the browser. I can't quit via any normal means.
Summary: extension check dialog is annoying and effectively hangs the Firefox process → extension check dialog is annoying and can effectively hang the Firefox process
I ran into this again on a flight. Started digging through source code and I found the problem.

toolkit/mozapps/extensions/content/update.js:

108   onWizardCancel: function ()
109   {
110     if (!gInteruptable) {
111       gPendingClose = true;
112       this._setUpButton("back", null, true);
113       this._setUpButton("next", null, true);
114       this._setUpButton("cancel", null, true);
115       return false;
116     }

This is the function that executes when you click the cancel button in extension check. This bug happens when gInteruptable == false. As you can see, in that situation the buttons become unavailable and the cancel is not allowed. This takes away all UI options while disallowing cancel, because you can't quite or close the window any other way (at least on OS X). Now the user is stuck with no options until gInteruptible == true.

Here is the code in the same file that manages gInteruptible:

166       gInteruptable = false;
167       AddonRepository.repopulateCache(ids, function() {
168         AddonManagerPrivate.updateAddonRepositoryData(function() {
169           gInteruptable = true;
170           if (gPendingClose) {
171             window.close();
172             return;
173           }

The problem is that the code between setting gInteruptible to false and then true can take a *long* time to run. It does blocking network i/o, so in the worst case you get a network timeout and the user just has to sit there. That could be 30 to 90 seconds, maybe more in my experience. This is why the problem is worst when you're connected via WiFi but on a network that won't work (captive portal that isn't set up).

This is basically a lame-network bug, but higher-level than Necko. It's effectively a full hang of Firefox for a very long time on startup.
Component: General → Add-ons Manager
Product: Firefox → Toolkit
Whiteboard: [Snappy]
This was also a big problem in the past when we were having server problems and AMO couldn't handle the update check load.
Whiteboard: [Snappy] → [Snappy:P1]
So, some background here:

The uninterpretable part was added with the compatible-by-default stuff - its fetching compatibility overrides (which go into the addon repository database), then applying any necessary compatibility changes to the addons in the main extensions database. 

Because of the way AddonRepository is currently designed, there's no way to cancel the network request/insertion into it's DB - so exiting out of the dialog means the request will still eventually succeed, and the data inserted into the repository database. That's a huge problem, because with the dialog gone the callback no longer gets called and the main database doesn't get updated - so there's an inconsistency with parts of the Add-ons Manager thinking an addon is compatible, and others thinking it's not. That alone was a serious enough bug to need this kind of awful hack with the dialog.

On top of that, since the compatibility overrides list was new, we *needed* to ensure it was populated *before* first run for compatible-by-default to be safe when we first introduced it. I think that's far less important now days for the majority of cases, where that data will already exist (just maybe slightly out of date). For new profiles, that's still important - at least until we hook up refreshing this data in the 3rd party install confirmation dialog. The pain point is still upgrading from a version from before compatible-by-default existed, but that should be very rare now.

The awful hack was only meant to be temporary, but I ran out of available time :( The long-term proper solution involves fixing AddonRepository so requests can be properly canceled (the API is generally causing us all sorts of issues - it needs a redesign). 

Just looking at the code now, I guess we could introduce a new API into AddonRepository that does both repopulateCache()/updateAddonRepositoryData() - that would ensure that process remains atomic, while letting the dialog be safely closed. Would mean that it doesn't guarantee the overrides list is updated before first startup, but as mentioned above that's less important now.
Blocks: start-faster
Whiteboard: [Snappy:P1] → [Snappy:P1] c=startup_misc u= p=
Is this going to overlap with changes for bug 760356 (only show the UI when necessary)?

I've left most of my working thoughts as comments in the patch. I've done rough testing by setting extensions.update.url to a black-hole host name so that I have time to close the window before requests time out. The requests do keep running, but I haven't figured out how to test an actual add-on upgrade happening after window close yet.

The two main open questions I have right now are:

1) Do we want to continue downloading and applying add-on upgrades in the background when the user cancels the dialog?

2) Do we want to show UI telling the user we've disabled add-ons that are not compatible with the new version?
Assignee: nobody → irving
Status: NEW → ASSIGNED
Attachment #809379 - Flags: feedback?(bmcbride)
(In reply to :Irving Reid from comment #5)
> Is this going to overlap with changes for bug 760356 (only show the UI when
> necessary)?

Nah, should be fine.

> 1) Do we want to continue downloading and applying add-on upgrades in the
> background when the user cancels the dialog?

Yes.


> 2) Do we want to show UI telling the user we've disabled add-ons that are
> not compatible with the new version?

Yes. Well, kinda. See next comment.
Comment on attachment 809379 [details] [diff] [review]
WIP let addon compatibility check continue in background when window is closed

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

::: toolkit/mozapps/extensions/content/update.js
@@ +104,5 @@
>    onWizardCancel: function gUpdateWizard_onWizardCancel()
>    {
> +    dump("*** Window cancelled\n");
> +    if (gVersionInfoPage.fillingCache) {
> +      // XXX should we show a "these addons are not compatible" UI?

Yes, but not once you've hit "Cancel" - that'd just add the need to click another button.

Would be better to have the dialog show the list of newly incompatible add-ons in the screen where it's checking for/downloading updates for them. Probably best leave that part for a followup bug, as that's the one thing that will depend on bug 760356.

@@ +110,5 @@
>      }
>  
>      if (gInstallingPage.installing) {
> +      // XXX We could let these continue in the background, but they might end up
> +      // needing a restart

That's fine - if they need a restart, it'll just be like any automatic background update. And some will be restartless, so updating those immediately will mean they can be re-enabled sooner than they normally would have been.

@@ +160,5 @@
>        // individual addon updates.
>        let ids = [addon.id for each (addon in gUpdateWizard.addons)];
>  
> +      AddonRepository.repopulateCache(ids, function gVersionInfoPage_repopulateCache() {
> +        AddonManagerPrivate.updateAddonRepositoryData(

Hm, I definitely remember having issues when the window was closed while waiting for repopulateCache() to do it's thing - since the window's scope was forcibly destroyed. Are you not seeing that now? If not, do you know why?

Because if repopulateCahce() succeeds, but updateAddonRepositoryData doesn't get called, all sorts of things get out of sync and bad things happen. Which, really sounds like footgun, even ignoring the scope issue I had. So it may be best to make repopulateCache() call updateAddonRepositoryData() internally, before calling the supplied callback.

@@ +168,5 @@
> +            if (window.closed) {
> +              dump("Addon check completed after dialog closed\n");
> +              // We could return here and just live with the enables / disables
> +              // caused by updateAddonRepositoryData, or we can continue on and
> +              // fetch available updates in the background

May as well update them in the background (if automatic background updates are enabled) - since we know they need updating, better to do that sooner rather than later.

@@ +187,5 @@
>      });
>  
> +    if (window.closed) {
> +      dump("Addons completed in closed window\n");
> +      Services.obs.notifyObservers(null, "addons-background-update-complete", null);

Why is this needed?

@@ +215,5 @@
>      ++this._completeCount;
>  
> +    // XXX This lets add-on upgrades complete in the background if the dialog is closed
> +    // but doesn't do anything about those that need restart
> +    if (!window.closed) {

Hm, why do anything in this method if the window has closed?
Attachment #809379 - Flags: feedback?(bmcbride) → feedback+
Depends on: 925389
Depends on: 965788
Depends on: 966374
This tests a few more steps in the dialog, and cancels / cleans up pending add-on updates if the dialog is closed while update requests are in progress.
Attachment #809379 - Attachment is obsolete: true
Attachment #8372489 - Flags: feedback?(bmcbride)
Comment on attachment 8372489 [details] [diff] [review]
WIP allow user to close compat check popup, now with more tests

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

Thinking about this dialog some more, I rather wonder why we even present a UI for selecting which add-ons to update - when the default is to always auto-update. We should only show that selection UI if any of the add-ons aren't set to auto-update, which should streamline this dialog. Filed bug 979277.

::: toolkit/mozapps/extensions/content/update.js
@@ +106,5 @@
>  
>    onWizardCancel: function gUpdateWizard_onWizardCancel()
>    {
> +    if (gVersionInfoPage.fillingCache) {
> +      dump("Dialog closed while waiting for updated compatibility information\n");

Would be nice if this used Log.jsm - logging some of this to file could be useful for troubleshooting/diagnosing/debugging.

@@ +121,5 @@
> +
> +    // cancel any AddonInstalls we created
> +    for (let [, install] of gUpdateWizard.addonInstalls) {
> +      dump("XXX Cancelling install for " + install.name + "\n");
> +      install.cancel();

Hm, per comment 6, shouldn't these be allowed to continue in the background?

@@ +216,4 @@
>      if (gUpdateWizard.addons.length > 0) {
>        // There are still incompatible addons, inform the user.
> +      if (!gUpdateWizard.xpinstallEnabled && gUpdateWizard.xpinstallLocked) {
> +        document.documentElement.currentPage = document.getElementById("adminDisabled");

If you've added this check here, it doesn't need to be in gUpdatePage.onPageShow() (should be unreachable there now)

@@ +472,5 @@
>      if (this._installs.length == this._currentInstall) {
>        this._installing = false;
> +      if (window.closed) {
> +        // XXX We might want to make noise if there are install errors after the
> +        // dialog is closed

Better not to. We'll automatically re-try it sometime anyway. We want the path of least interruption.

@@ +489,5 @@
>    /////////////////////////////////////////////////////////////////////////////
>    // InstallListener
>    onDownloadStarted: function gInstallingPage_onDownloadStarted(aInstall) {
> +    if (window.closed) {
> +      // XXX cancel any remaining installs?

No, leave everything installing in the background. Restartless installs will keep installing fine, non-restartless installs will at the very least get upgraded potentially sooner than they normally would have.

::: toolkit/mozapps/extensions/test/browser/browser_cancelCompatCheck.js
@@ +224,5 @@
> +
> +// Tests that the right add-ons show up in the mismatch dialog and updates can
> +// be installed
> +// This is a task-based rewrite of the first test in browser_bug557956.js
> +// add_task(

Is this meant to be disabled?
Attachment #8372489 - Flags: feedback?(bmcbride) → feedback+
https://tbpl.mozilla.org/?tree=Try&rev=2be0e9ad2695

(In reply to Blair McBride [:Unfocused] from comment #9)
> Comment on attachment 8372489 [details] [diff] [review]
> WIP allow user to close compat check popup, now with more tests
> 
> Review of attachment 8372489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thinking about this dialog some more, I rather wonder why we even present a
> UI for selecting which add-ons to update - when the default is to always
> auto-update. We should only show that selection UI if any of the add-ons
> aren't set to auto-update, which should streamline this dialog. Filed bug
> 979277.
> 
> ::: toolkit/mozapps/extensions/content/update.js
> @@ +106,5 @@
> >  
> >    onWizardCancel: function gUpdateWizard_onWizardCancel()
> >    {
> > +    if (gVersionInfoPage.fillingCache) {
> > +      dump("Dialog closed while waiting for updated compatibility information\n");
> 
> Would be nice if this used Log.jsm - logging some of this to file could be
> useful for troubleshooting/diagnosing/debugging.

Converted. I really need to get bug 966674 fixed...

> @@ +121,5 @@
> > +
> > +    // cancel any AddonInstalls we created
> > +    for (let [, install] of gUpdateWizard.addonInstalls) {
> > +      dump("XXX Cancelling install for " + install.name + "\n");
> > +      install.cancel();
> 
> Hm, per comment 6, shouldn't these be allowed to continue in the background?
...
> @@ +489,5 @@
> >    /////////////////////////////////////////////////////////////////////////////
> >    // InstallListener
> >    onDownloadStarted: function gInstallingPage_onDownloadStarted(aInstall) {
> > +    if (window.closed) {
> > +      // XXX cancel any remaining installs?
> 
> No, leave everything installing in the background. Restartless installs will
> keep installing fine, non-restartless installs will at the very least get
> upgraded potentially sooner than they normally would have.

The user has explicitly said "cancel" after we said we were going to update add-ons, so in one sense we're violating the user's wishes to keep going. On the other hand, the user (probably) has given us permission to auto-update add-ons. I decided to compromise and extended a few more wizard pages (and modified test cases) to allow installs to keep running in the background, but only for add-ons marked to auto update.

I didn't do all of the pages because bug 760356 and bug 960597 are likely to rearrange the flow a bit (we could probably eliminate two pages with bug 960597).

> ::: toolkit/mozapps/extensions/test/browser/browser_cancelCompatCheck.js
> @@ +224,5 @@
> > +
> > +// Tests that the right add-ons show up in the mismatch dialog and updates can
> > +// be installed
> > +// This is a task-based rewrite of the first test in browser_bug557956.js
> > +// add_task(
> 
> Is this meant to be disabled?

I expanded the comment, but yes - it's completely redundant to the existing test in browser_bug557956.js, I was using it as a guide for the step by step process to walk through the dialog.
Attachment #8372489 - Attachment is obsolete: true
Attachment #8389145 - Flags: review?(bmcbride)
Comment on attachment 8389145 [details] [diff] [review]
Allow user to close compatibility check popup while checks are in progress

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

\o/

::: toolkit/mozapps/extensions/content/update.js
@@ +19,5 @@
>  Components.utils.import("resource://gre/modules/AddonManager.jsm");
>  Components.utils.import("resource://gre/modules/addons/AddonRepository.jsm");
>  
> +Components.utils.import("resource://gre/modules/Log.jsm");
> +let logger = Log.repository.getLogger("addons.update");

addons.update-dialog

As it makes it more distinct.

@@ +248,5 @@
> +      logger.debug("VersionInfo update " + this._completeCount + " of " + this._totalCount +
> +           " failed for " + aAddon.id + ": " + status);
> +      gUpdateWizard.errorItems.push(aAddon);
> +    }
> +    else

Nit: Add {}

@@ +439,5 @@
> +    if (!gUpdateWizard.xpinstallEnabled) {
> +      return;
> +    }
> +
> +    logger.debug("Start installs for " + [i.existingAddon.id for (i of aInstallList)].join(", "));

Nit: You use join() here but toSource() elsewhere.

::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm
@@ +44,5 @@
>  var gRDF = Cc["@mozilla.org/rdf/rdf-service;1"].
>             getService(Ci.nsIRDFService);
>  
>  Cu.import("resource://gre/modules/Log.jsm");
> +const LOGGER_ID = "addons.checker";

addons.update-checker

As I can imagine actually introducing some kind of add-on checker tool.
Attachment #8389145 - Flags: review?(bmcbride) → review+
Carrying forward Blair's r+ from comment 11.
Attachment #8389145 - Attachment is obsolete: true
Attachment #8391203 - Flags: review+
Attachment #8391203 - Flags: checkin?
Comment on attachment 8391203 [details] [diff] [review]
Allow user to close compatibility check popup, nits cleaned up

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7761b7514f7

In the future, please just use the checkin-needed keyword. It works better with our bug marking tools :)
Attachment #8391203 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/b7761b7514f7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: