727398 - Mozilla Firefox Hotfix is listed in Add-ons Manager even though it shouldn't.

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Vlad [QA]]

Attachment: Screenshot with the add-on

This is happening on all platforms.

Steps to reproduce:
1. Start Firefox 11 beta with a new, clean profile.
2. Change extensions.update.url to addons-dev.allizom.org and set the extensions.update.interval pref to 10
3. Restart Firefox for the changes to take effect and open Add-ons Manager
4. Browse for a while with the Add-ons Manager open.

Expected results:
The default add-on should update (ex:Feedback)

Actual results:
In the time that the Feedback add-on is updating, a Mozilla Hotfix update appears and it's disabled with no working buttons (see screenshot)
*Note: the add-on disappears when restarting the add-ons manager or the browser.

Comment 1

[Dave Townsend [:mossop]]

Blair, would you have time to take a look at this? I think the download entry for the add-on is getting stuck somehow

Comment 2

[hannu.nyman]

Attachment: screenshot of hotfix download failure

Alternative symptom with an old Firefox profile: the Hotfix is shown there, and when manually updating add-ons with "Check for Updates" from the Add-ons Manager, Firefox Beta 11 tries to update also the Hotfix and fails.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0 ID:20120208012847

Comment 3

[Dave Townsend [:mossop]]

(In reply to hannu.nyman from comment #2)
> Created attachment 597448 [details]
> screenshot of hotfix download failure
> 
> Alternative symptom with an old Firefox profile: the Hotfix is shown there,
> and when manually updating add-ons with "Check for Updates" from the Add-ons
> Manager, Firefox Beta 11 tries to update also the Hotfix and fails.
> 
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0
> ID:20120208012847

This is a different bug, we shouldn't be checking for hotfix updates in response to that button at all, or at least if we do then we need to add special logic to handle it.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Seems to fail removing the file from /trash/ when uninstalling (it uninstalls itself immediately on startup). Which is fine, but interesting.

But it then throws in AddonWrapper.hasResource(). Since that's being called after uninstall (after the file is moved to /trash/), the source bundle no longer exists, so calling nsIFile.isDirectory() on that throws.

Not sure why removing that file is failing, or why hasResource is being called after uninstall. I suspect it's just a timing issue. Will investigate further tomorrow.

Comment 5

[Dave Townsend [:mossop]]

My guess would be the zip file is locked open accessing the icon in there.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

UI was breaking for two reasons:
* Addon.hasResource() was being called after uninstall and throwing an exception, but everything assumes that function will never throw
* startup() in bootstrap.js is called before onInstalled/onInstallEnded are sent. Since the addon uninstalls itself in startup(), onUninstalled could be sent before onInstalled/onInstallEnded.

I have a working patch - just gotta write some tests.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Comment 8

[Dave Townsend [:mossop]]

This API change concerns me a little. The intention was that after onInstalled is called the add-on is running. I guess that's not such a big deal but I wonder if we still need a way to get that information (to save you having to wait for a pref to be set in the tests f.e.).

There are some 370 add-ons on AMO that call addAddonListener. Thankfully the majority of them are Brand Thunder based so it might be interesting to verify that nothing breaks there.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, I pondered that. But I think the only reasons that would matter would be:
* An addon depends on another addon (rare, and listening for onInstalled would be wrong anyway)
* An addon is handling the install of itself wrong (using onInstalled instead of the bootstrap install function).

I went through every addon on AMO that listens for onInstalled. Most of those cases are an empty function (useful, huh?). The other cases, I couldn't see anything that could break (just looking at the code).

I did wonder about firing onEnabled, which seems semantically correct. Especially given that non-restartless extensions can be installed disabled (by disabling when onInstalling fires). I was kinda scared by the number of tests that could potentially break, but looking at them, it may not be that many after all.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 599482 [details] [diff] [review]
Patch v1

Let's go with this. Could you blog about the change, maybe also mention it to jorge and we'll see what kind of response we get?

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Good idea.

http://theunfocused.net/2012/03/06/addons-manager-api-change/

(Also emailed Jorge.)

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I had meant to land this before the merge, but then got sick. This this something we want on Fx13?


https://hg.mozilla.org/integration/fx-team/rev/0bcbe43a0245

Comment 13

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #12)
> I had meant to land this before the merge, but then got sick. This this
> something we want on Fx13?
> 
> 
> https://hg.mozilla.org/integration/fx-team/rev/0bcbe43a0245

I don't think we need to push it out that fast, it's a small UI issue that only happens if the user has the add-ons manager open at the time of install.

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/0bcbe43a0245

Comment 15

[Alex Keybl [:akeybl]]

(In reply to Dave Townsend (:Mossop) from comment #13)
> I don't think we need to push it out that fast, it's a small UI issue that
> only happens if the user has the add-ons manager open at the time of install.

Agreed - we do have plans to use the hotfix in FF13 to enable an about:home pref mid-cycle (related to the Mozilla Marketplace), but we'll likely just flip the pref and have it uninstall itself immediately afterwards.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Alex Keybl [:akeybl] from comment #15)
> Agreed - we do have plans to use the hotfix in FF13 to enable an about:home
> pref mid-cycle (related to the Mozilla Marketplace), but we'll likely just
> flip the pref and have it uninstall itself immediately afterwards.

If the Add-ons Manager is open when that happens, that will trigger this (minor) bug. But as an easy work-around just delay uninstalling by, say, 1 second.