690819 - Nightly with Mozilla Labs: Lab Kit never stops displaying "Updating add-ons"

Status: RESOLVED FIXED

(Toolkit :: Blocklist Policy Requests, defect)

Description

[B.J. Herbison]

With Firefox Nightly 10.0a1 (2011-09-30) on Windows XP:
1) Create a new profile.
2) Add Add-on Compatibility Reporter 0.9.2.
3) Add Mozilla Labs: Lab Kit 3.
4) In the Add-ons Manager click gear/Check for Updates.
5) Take a long lunch.

Expected: A display of "No updates found" next to the gear.
Actual: "Updating add-ons" is still displayed.

Also observed on Firefox 7.0 and 7.0.1 (without a clean profile) and Aurora 7.0a2 (mentioned in July on https://addons.mozilla.org/en-US/firefox/addon/mozilla-labs-lab-kit/).

Comment 1

[Dimas]

The same here in Firefox 8 beta 1, since Firefox 7.0 beta

Comment 2

[wamsaya]

Same here with firefox 7.01 stables release

Comment 3

[Dave Townsend [:mossop]]

Labs kit is overriding the module used to check for add-on updates with its own, broken version.

Comment 4

[Ed Lee :Mardak]

I've dropped the maxversion to 6.

I would assume this is where the update could be breaking?

  Cu.import("resource://gre/modules/AddonUpdateChecker.jsm");
  let orig = AddonUpdateChecker.checkForUpdates;
  AddonUpdateChecker.checkForUpdates = function() {
    // Don't block the original call and check for updates after it finishes
    Utils.delay(function() checkForUpdates());
    return orig.apply(this, arguments);
  };

Comment 5

[Ed Lee :Mardak]

Ah, it's actually because services-sync refactored various modules at some point.

Warning: WARN addons.manager: Exception calling callback: TypeError: Svc.IO is undefined

Comment 6

[Dave Townsend [:mossop]]

Yeah. Some example messages (the latter being the breakage specific to this bug):

*** WARN addons.manager: Exception calling callback: TypeError: Svc.IO is undefined
*** WARN addons.manager: Exception calling callback: TypeError: Utils.delay is not a function

Comment 7

[Dimas]

Without know technical details... is it possible to protect the Firefox code against issues like that? I mean not only fix the Lab Kit error... if it can broke that module other addons could too, isn't it?

Comment 8

[Dave Townsend [:mossop]]

(In reply to Dimas from comment #7)
> Without know technical details... is it possible to protect the Firefox code
> against issues like that? I mean not only fix the Lab Kit error... if it can
> broke that module other addons could too, isn't it?

The problem is that we let add-ons do anything they want. The only protection against that would be to severely curtail the capabilities available to add-ons which would vastly restrict the types of add-ons you can make. Very popular add-ons like NoScript for example would probably no longer be possible.

Comment 9

[Dave Townsend [:mossop]]

(In reply to Edward Lee :Mardak from comment #5)
> Ah, it's actually because services-sync refactored various modules at some
> point.

So what is the story here, did you mistakenly bump compatibility or something? I'd suggest that you should do a blog post about this since you've broken add-on updates for any users with no way to fix the problem other than uninstall/manually updating this add-on. It's also possible (and I'd like you to check this to make sure it isn't) that you'll have broken automatic app updates for those users too, since app update does an add-on update check in most cases.

I'd really rather you didn't override the add-on update components like this.

Comment 10

[Ed Lee :Mardak]

The maxVersion in the install.rdf is set to <maxVersion>4.0.*</maxVersion> but I don't recall changing the maxVersion. I did get an email a while back:

Good news! Our automated tests did not detect any compatibility issues with your add-on Mozilla Labs: Lab Kit and Firefox 6. We've updated your add-on's compatibility to work with Firefox 6.* so that our Aurora users can begin using your add-on. Firefox 6 beta is expected in just a few weeks.

Comment 11

[Dave Townsend [:mossop]]

Before you dropped the maxVersion to 6 on AMO what was it set to? I've just checked and this does break app updates in the case where the new app version would cause one of the installed add-ons to become incompatible.

What are the stats like for this add-on? If we have too many users on this we might have to consider a blocklist here.

Comment 12

[Ed Lee :Mardak]

(In reply to Dave Townsend (:Mossop) from comment #11)
> Before you dropped the maxVersion to 6 on AMO what was it set to?
I believe it was 7.

> What are the stats like for this add-on?
The ADU showing today is 750.

Comment 13

[Justin Scott [:fligtar]]

Given that add-on and app updates are broken, I think we need to blocklist. Is there a fixed version that can be put out, such that we only need to block lower version numbers and only in Firefox 7?

Also, I'm interested in learning more about the changes to services modules which escaped compatibility flags.

Comment 14

[Philipp von Weitershausen [:philikon]]

(In reply to Justin Scott [:fligtar] from comment #13)
> Also, I'm interested in learning more about the changes to services modules
> which escaped compatibility flags.

The relevant changes happened in resource://services-sync/util.js (services/sync/modules/util.js) which contains a bunch of helpers for Sync and, when the same codebase also services Firefox 3.5/3.6 users, lazy XPCOM service getters and other things that were introduced in Firefox 4+. This is not considered a public API module like Services.jsm, NetUtil.js, etc., so we liberally pruned all the code that was duplicating helpers that are now in Firefox 4+ in bug 648364 (which landed in Firefox 6).

Comment 15

[Ed Lee :Mardak]

There isn't a fixed version yet. I can make a version that does what the services module refactored:

from: Utils.delay(callback)
to: Utils.nextTick(callback)
to: Services.tm.currentThread.dispatch(callback, DISPATCH_NORMAL)

To not rely on the services utils.

But Mossop would like an alternate way that does not override the add-on update component. It seems like a listener/observer can be set somehow when creating UpdateChecker from XPIProvider. Or maybe somehow hooking into AddonManagerPrivate.callInstallListeners?

Comment 16

[Philipp von Weitershausen [:philikon]]

(In reply to Edward Lee :Mardak from comment #15)
> There isn't a fixed version yet. I can make a version that does what the
> services module refactored:
> 
> from: Utils.delay(callback)
> to: Utils.nextTick(callback)
> to: Services.tm.currentThread.dispatch(callback, DISPATCH_NORMAL)
> 
> To not rely on the services utils.

Yes, please do not use services-sync/util.js! We may break you again at any time in the future, and IIRC we already did several times in the past.

Comment 17

[Dave Townsend [:mossop]]

Attachment: blocklist screenshot

I've just verifying that blocklisting still works with one minor issue. The add-on uses a rather large image (web hosted too though I had thought we disallowed that) for its icon, so the blocklisting UI looks a little messed up.

Comment 18

[Dave Townsend [:mossop]]

(In reply to Edward Lee :Mardak from comment #15)
> But Mossop would like an alternate way that does not override the add-on
> update component. It seems like a listener/observer can be set somehow when
> creating UpdateChecker from XPIProvider. Or maybe somehow hooking into
> AddonManagerPrivate.callInstallListeners?

What is the goal for overriding the update checker?

Comment 19

[Ed Lee :Mardak]

(In reply to Dave Townsend (:Mossop) from comment #18)
> What is the goal for overriding the update checker?
It was to also do a Lab Kit add-on update check whenever the user manually checked or add-on updates checked in the background.

Comment 20

[Dave Townsend [:mossop]]

(In reply to Edward Lee :Mardak from comment #19)
> (In reply to Dave Townsend (:Mossop) from comment #18)
> > What is the goal for overriding the update checker?
> It was to also do a Lab Kit add-on update check whenever the user manually
> checked or add-on updates checked in the background.

Is there a reason this has to happen at the same time as the background update? If so you could just watch for when the "app.update.lastUpdateTime.addon-background-update-timer" pref changes. Alternatively just register a once a day timer of your own. For when the user checks for updates you could just add an event listener to the button. That seems a whole lot safer than replacing components.

In situations where you absolutely must replace components it is safer to call the real component first before doing your own thing, that way if your own code fails in some way it doesn't break too much.

Comment 21

[Ed Lee :Mardak]

(In reply to Dave Townsend (:Mossop) from comment #20)
> Alternatively just register a once a day timer of your own.
It does have its own timer as well. This code path was to allow users to force a check when they clicked "Check for Updates" for add-ons.

> just add an event listener to the button
So listen for a tab to show about:addons and hook in there?

> In situations where you absolutely must replace components it is safer to
> call the real component first before doing your own thing, that way if your
> own code fails in some way it doesn't break too much.
Sure. The code here was trying to preserve any return value if needed:

    Utils.delay(function() checkForUpdates());
    return orig.apply(this, arguments);

But sure, it could have done "let ret = orig.apply..;"

Comment 22

[Dave Townsend [:mossop]]

(In reply to Edward Lee :Mardak from comment #21)
> (In reply to Dave Townsend (:Mossop) from comment #20)
> > Alternatively just register a once a day timer of your own.
> It does have its own timer as well. This code path was to allow users to
> force a check when they clicked "Check for Updates" for add-ons.
> 
> > just add an event listener to the button
> So listen for a tab to show about:addons and hook in there?

Yep, chrome-document-global-created is a useful tool for this.

Comment 23

[Justin Scott [:fligtar]]

Mardak, what's your ETA for a fixed version so I can plan accordingly?

Also note that there are 3000 users with this add-on -- the 700 users are only the ones who *aren't* affected since they are still pinging for updates and counting in our stats.

Comment 24

[Ed Lee :Mardak]

(In reply to Justin Scott [:fligtar] from comment #23)
> Mardak, what's your ETA for a fixed version so I can plan accordingly?
There's the simple fix to just correct the one Utils.delay. But for a full fix to stop using services utils/resource/Svc/Preferences and hooking into about:addons is a bit more work.

I'm seeing if we still want to keep Lab Kit around.

Comment 25

[Ed Lee :Mardak]

(In reply to Justin Scott [:fligtar] from comment #23)
> Mardak, what's your ETA for a fixed version so I can plan accordingly?
How soon/late is it acceptable to turn on the blocklist? We're going to put out a blog post and mailing list message about discontinuing Lab Kit.

The idea being that hopefully users can see the message and act by removing the add-on from Firefox 7 before they're greeted with attachment 565300 [details].

The basic flow being: Lab Kit is being discontinued. For those using Lab Kit on Firefox 7, please uninstall it and restart your browser. If not, you will be notified that Lab Kit is blocked in X days.

Comment 26

[Justin Scott [:fligtar]]

There aren't any security updates to Firefox 7 and I don't know of any serious vulnerability fixes in other add-ons that have gone out recently, so maybe we would block on Tuesday?

As far as the block dialog, if it's a remote image can't we just change the size server-side?

Comment 27

[Dave Townsend [:mossop]]

(In reply to Justin Scott [:fligtar] from comment #26)
> As far as the block dialog, if it's a remote image can't we just change the
> size server-side?

We can, though based on the URL of it it looks likely to be used on the website somewhere too. I filed bug 692617 on fixing the UI to cope with large images as we do elsewhere.

Comment 28

[Ed Lee :Mardak]

(In reply to Justin Scott [:fligtar] from comment #26)
> maybe we would block on Tuesday?
Okay. We'll try to get the blog post out by tomorrow.

> As far as the block dialog, if it's a remote image can't we just change the
> size server-side?
What size should it be? It seems like the image is part of the theme and hardcoded in places I can't edit. I'll try to set up a redirect to point to another file that I can upload..

http://mozillalabs.com/wp-content/themes/labs_project/img/labkit-header.png

Comment 29

[Dave Townsend [:mossop]]

32x32 should look fine (that is what the UI was designed around)

Comment 30

[Ed Lee :Mardak]

I tried changing the icon with a redirect, but that doesn't seem to work.

We pushed out a notice to the Labs blog and the mailing list:
http://mozillalabs.com/blog/2011/10/lab-kit-is-retiring/

We can close this bug tomorrow when we push out the blocklist?

Comment 31

[Justin Scott [:fligtar]]

OK, any last minute objections to blocking today?

Comment 32

[Justin Scott [:fligtar]]

Before we block, Dave, do you have any ideas on why the image redirection didn't work?

Comment 33

[Ed Lee :Mardak]

It's on the mozillalabs.com wordpress setup. Somehow these images are part of the theme, so I'm unable to change them. I tried to hack around it by getting wordpress to redirect, but I can't even get it to work normally in a browser or curl.

Comment 34

[Justin Scott [:fligtar]]

Ok. I've gone ahead and blocked in production and disabled the add-on on AMO.