1037029 - Update hotfix may be installed in disabled state

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Gregory Szorc [:gps]]

Catalin discovered that the update hotfix is installed in disabled mode on OS X and Linux and never gets uninstalled. I can reproduce.

STR:

1) New profile on Linux or Mac
2) Hook up extension updating to addons-dev.allizom.org
3) Trigger some extension updates

Results:

1) We get the SSL cert hotfix on first trigger
1a) If your Firefox version is really old, we'll get another cert hotfix on 2nd trigger
2) We get the update hotfix on next trigger
3) Hotfix is installed, but disabled

This doesn't make much sense because bootstrap.js's install() explicitly sets userDisabled = false [1].

I dug into things and the add-on's appDisabled is true!

During the review stages of bug 1014194, <em:targetPlatform>WINNT</em:targetPlatform> was added to the install.rdf. I reckon this is what's causing appDisabled to be true.

A handful of existing hotfixes set <targetPlatform>, so there's precedent for it.

Looking at the implementation of other hotfixes, they each perform an "is compatible" check in bootstrap.js:install() and do an uninstall if not. This code runs even if appDisabled is set.

The update hotfix, by contrast, performs its "is compatible" check in update.jsm, which runs after startup(). startup() never runs because appDisabled=true.

The easy fix is to remove <targetPlatform> from install.rdf and let the hotfix code post startup() do the uninstall. This code works fine on Linux and Mac.

I knew better than this. I should have pushed back on the review comment that asked to add <targetPlatform> :/

[1] https://hg.mozilla.org/releases/firefox-hotfixes/file/fc3c622aff83/v20140527.01/bootstrap.js#l29

Comment 1

[Gregory Szorc [:gps]]

I reckon this would impact <targetApplication><maxVersion> as well. If we don't release another hotfix for a while, Firefox 33+ users could have this hotfix perma-installed in a disabled state until a new hotfix is released.

I /think/ we want to remove the upper bound on <maxVersion>. That, or we move some of the "is compatible" checks back into bootstrap.js:install().

Comment 2

[Gregory Szorc [:gps]]

Attachment: Uninstall hotfix if appDisabled is set

I have no clue if this is proper add-on behavior because I don't know if
bootstrap.js:install() will ever be called with appDisabled=true during
the process of upgrade, etc.

I'm not sure how I can test this locally since manual xpi installs
result in a user notification that the add-on is not compatible and
bootstrap.js:install() is never even called.

This patch scares me a little. That's why I want someone familiar with
AddonManager behavior to review it.

Comment 3

[Benjamin Smedberg]

Comment on attachment 8453902 [details] [diff] [review]
Uninstall hotfix if appDisabled is set

It's pretty surprising that install() would be called at all if an addon is appDisabled. But since we know that's what's actually happening, this makes sense. The only other realistic option is to remove most of the version checks and do all the version checking internally in the addon.

Can you test by setting up a mock addon update service?

I'm not going to clear Blair's review, but I think we should go ahead and get this landed and staged.

Comment 4

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/firefox-hotfixes/rev/db1fa91affeb

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8453902 [details] [diff] [review]
Uninstall hotfix if appDisabled is set

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

Yes, install() is always called, even during an update (with reason=ADDON_UPGRADE).

::: v20140527.01/bootstrap.js
@@ +41,5 @@
> +  // We have additional compatibility checks after startup(), inside
> +  // update.jsm. However, if appDisabled is set, then startup() and these
> +  // checks will never run. We assume that appDisabled means we would fail
> +  // these checks anyway.
> +  if (data.appDisabled) {

.... however, appDisabled is not a property of the data param. You need to check that on the addon object. Thankfully, there's no race condition with startup() if appDisabled=true.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Properly check if addon is appDisabled

Patch against what already landed.

Comment 7

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/firefox-hotfixes/rev/2cde2496d01d

Comment 8

[Paul Silaghi, QA [:pauly]]

Tested on FF 11, 28 OS X 10.9, FF 10, 27 Ubuntu 13.04.
I confirm the hotfix is successfully uninstalled in the end, but during the installation I see it's still in the disable state (having the Addons Manager open while triggering the update ping).
Also note that on a manual install of the .xpi, "addon is not compatible" error message displayed, both on Mac and Linux.
I'm marking this bug as verified. Reopen if you consider any of the above as issues.