Closed Bug 790096 Opened 8 years ago Closed 8 years ago

Please create a hotfix add-on to address the update bustage on Firefox 15

Categories

(Firefox :: General, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: ehsan, Assigned: MattN)

References

Details

Attachments

(1 file, 2 obsolete files)

See bug 789958.

Here is the summary of my conversaion with MattN on #fx-team: http://pastebin.mozilla.org/1814739

This hotfix add-on is only applicable to Linux and Mac OS X, the bug does not exist on Windows.
A possible alternative?

1) Fix updater code, release as 15.0.2
2) When generating updates for 15.0.0 & 15.0.1 to 15.0.2, hand-tweak update manifest to ensure updater is treated as a complete update and not a partial (ie, no binary diffs, just a file replace)
3) Create hotfix addon that contains the 15.0.2 updater, and replaces your 15.0 / 15.0.1 updater with it.
4) Allow 15.0.2 update process to happen naturally (perhaps restarting it if it already failed?)

(I was thinking about how it would be nice if we had a way to update _just_ the updater, for situations like this. And then realized we basically could.)
(In reply to comment #1)
> A possible alternative?
> 
> 1) Fix updater code, release as 15.0.2
> 2) When generating updates for 15.0.0 & 15.0.1 to 15.0.2, hand-tweak update
> manifest to ensure updater is treated as a complete update and not a partial
> (ie, no binary diffs, just a file replace)
> 3) Create hotfix addon that contains the 15.0.2 updater, and replaces your 15.0
> / 15.0.1 updater with it.
> 4) Allow 15.0.2 update process to happen naturally (perhaps restarting it if it
> already failed?)
> 
> (I was thinking about how it would be nice if we had a way to update _just_ the
> updater, for situations like this. And then realized we basically could.)

The original bug affects both partial and complete updates the same way (and indeed, right after the partial update fails, we fall back into downloading and applying the full update, which will also fail.)

However, the idea of releasing the hotfix add-on to replace the updater binary with a potential 15.0.2 binary which fixes this issue seems reasonable after some initial thought.
Robert, please see comments 1 and 2.
The hotfix add-on won't always have privileges to update the updater binary. I also think using the hotfix add-on to disable bgupdates for the affected platforms and then updating has less moving parts that could go wrong. It is an alternative that we could use if necessary but I don't see additional value in the alternative.
Still some TODOs to address but this is most of the way there.

Some questions:
1) Which Firefox versions are we targeting to flip the pref to false?

2) Isn't there a directory service entry for the application install directory? I didn't see it when I looked quickly but I could have sworn there was one.

3) This patch doesn't handle flipping the pref back to true on a fixed build. I'm guessing we will just use Services.vc to compare the version against a known fixed one.  Will we know the fixed version before releasing this or are we going to use two hotfixes (one to flip to false and another to flip to true when fixed)?

Tested quickly locally on FC 17 and OS X 10.7.4
Test build: https://people.mozilla.com/~mnoorenberghe/hotfix-v20120910.01-wip1.xpi

Uncomment two debug lines in check_app_permissions.sh and then disable+enable for more debug info.
Attachment #659977 - Flags: feedback?(ehsan)
Attachment #659977 - Flags: feedback?(dtownsend+bugmail)
Attachment #659977 - Flags: feedback?(dolske)
Comment on attachment 659977 [details] [diff] [review]
WIP Doesn't handle all error cases and only flips stage pref to false

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

::: v20120910.01/check_app_permissions.sh
@@ +22,5 @@
> +# find $1 -not -uid $uid -or -not \( $groupQuery \) >> /tmp/hotfix.txt
> +
> +# Find all files that are not owned by the user or are for a group which the
> +# user is not part of. If the result is empty, return 0, otherwise return 1.
> +if [ -z "`find $1 -not -uid $uid -or -not \( $groupQuery \)`" ]; then

Just realized I didn't include "-type f" like Ehsan said on IRC. Can someone confirm I should only check files and not directories?
Comment on attachment 659977 [details] [diff] [review]
WIP Doesn't handle all error cases and only flips stage pref to false

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

So, I gave this a bit more thought, and I think I now agree with Robert in that we should just disable background updates for OS X and Linux on the affected versions.  The affected versions will be 15.0 and 15.0.1 in the release channel.  I don't know which versions in other channels we want this on.  That is a call to be made on the release drivers mailing list, and I'll post about it right now (CCing you.)

Sorry for changing strategies here, but I think we should opt for as much simplicity in what the hotfix add-on does as possible.
Attachment #659977 - Flags: feedback?(ehsan) → feedback-
(In reply to Ehsan Akhgari [:ehsan] from comment #7)

> So, I gave this a bit more thought, and I think I now agree with Robert in
> that we should just disable background updates for OS X and Linux on the
> affected versions.

So, basically the hot fix is just a simple pref-flip for everyone on those two platforms, for 15.0+? Works for me.
(In reply to comment #8)
> (In reply to Ehsan Akhgari [:ehsan] from comment #7)
> 
> > So, I gave this a bit more thought, and I think I now agree with Robert in
> > that we should just disable background updates for OS X and Linux on the
> > affected versions.
> 
> So, basically the hot fix is just a simple pref-flip for everyone on those two
> platforms, for 15.0+?

Precisely, with the extra caveat that we need to handle version numbers across all branches.
Assigning myself as QA Contact. I'll test this if/when we decide to move forward.
QA Contact: anthony.s.hughes
My understanding is that this won't be released until after 16.0 is on the release channel and therefore this hotfix would have to be combined with bug 774509 as they both target OS X. The hotfix also doesn't set the stage pref to false if it doesn't exist since that would be either an old build without this feature or one that has the new pref (assuming we'll remove the old pref when we add the new one).

I'd like feedback on this standalone version before combining hotfixes.

https://people.mozilla.com/~mnoorenberghe/hotfix-v20120910.01-wip2.xpi
Attachment #659977 - Attachment is obsolete: true
Attachment #659977 - Flags: feedback?(dtownsend+bugmail)
Attachment #659977 - Flags: feedback?(dolske)
Attachment #660266 - Flags: feedback?(ehsan)
Attachment #660266 - Flags: feedback?(dolske)
Comment on attachment 660266 [details] [diff] [review]
Approach 2 (standalone) - flip the pref, set a hotfix pref, and uninstall

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

Looks good with the below comments addressed.

::: README
@@ +16,5 @@
>                 uninstalls itself immediately.
>  v20120430.01 - Bug 741004 - A hotfix rolled out to 10.0 - 12.* Windows users to
>                 communicate the de-support of Windows 2000 and XP <= SP1 in Firefox 13.
> +v20120910.01 - Bug 790096 - Disable staged updates for OS X and Linux for
> +               15.0 - 18.0a1. The pref will be changed in fixed builds.

This comment should probably be updated.

::: v20120910.01/bootstrap.js
@@ +9,5 @@
> +
> +const APP_UPDATE_STAGE_ENABLED_PREF = "app.update.stage.enabled";
> +// Preference to track that we flipped the above pref so we can flip it back
> +// when the updater is fixed.
> +const HOTFIX_APPLIED_PREF = "extensions.firefox-hotfix.20120910";

We don't need to flip the pref back on any more.  We'll just rename the pref on the fixed versions.  So, you can remove this and the related code below.

@@ +55,5 @@
> + */
> +function shouldHotfixApp() {
> +  // Ensure that this is the correct version in case compatability checking is overridden.
> +  if (Services.vc.compare(Services.appinfo.version, "15.0") < 0 ||
> +      Services.vc.compare(Services.appinfo.version, "18.0a1") > 0) {

Does this also include 15.0a1 and 15.0b1?  It probably should it if doesn't.
Attachment #660266 - Flags: feedback?(ehsan) → feedback+
Addressed the two code comments.

(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> ::: README
> > +v20120910.01 - Bug 790096 - Disable staged updates for OS X and Linux for
> > +               15.0 - 18.0a1. The pref will be changed in fixed builds.
> 
> This comment should probably be updated.

Doesn't it reflect the current plan?  Do you want it to be more verbose? I tweaked it slightly in this version.

Ehsan, since you're a Firefox module peer, your review is sufficient. I'd like to check this hotfix code into the hotfix repo and then make a separate hotfix version which combines this with bug 774509.
Attachment #660266 - Attachment is obsolete: true
Attachment #660266 - Flags: feedback?(dolske)
Attachment #660292 - Flags: review?(ehsan)
(In reply to Matthew N. [:MattN] from comment #13)
> Created attachment 660292 [details] [diff] [review]
> Standalone v2 - Flip pref if true and uninstall
> 
> Addressed the two code comments.
> 
> (In reply to Ehsan Akhgari [:ehsan] from comment #12)
> > ::: README
> > > +v20120910.01 - Bug 790096 - Disable staged updates for OS X and Linux for
> > > +               15.0 - 18.0a1. The pref will be changed in fixed builds.
> > 
> > This comment should probably be updated.
> 
> Doesn't it reflect the current plan?  Do you want it to be more verbose? I
> tweaked it slightly in this version.

No, the current plan is to rename the pref in fixed builds.
Comment on attachment 660292 [details] [diff] [review]
Standalone v2 - Flip pref if true and uninstall

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

LGTM, but I have never reviewed the code for the hotfix add-on before, and I'm not sure if I was looking for everything that I should be watching out for.  Can you please ask someone else to have a second look here as well?  Thanks!

::: README
@@ +16,5 @@
>                 uninstalls itself immediately.
>  v20120430.01 - Bug 741004 - A hotfix rolled out to 10.0 - 12.* Windows users to
>                 communicate the de-support of Windows 2000 and XP <= SP1 in Firefox 13.
> +v20120910.01 - Bug 790096 - Disable staged updates on OS X and Linux for
> +               15.0a1 - 18.0a1. The flipped pref name will be renamed in fixed builds.

Oh, you changed this.  Looks good!
Attachment #660292 - Flags: review?(ehsan) → review+
Comment on attachment 660292 [details] [diff] [review]
Standalone v2 - Flip pref if true and uninstall

Mossop/dolske/others, mind taking a look at this.  It's a pretty simple hotfix.

(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> No, the current plan is to rename the pref in fixed builds.

That's what I was trying to say but I can see how it was a bit ambiguous so I fixed it. Changing the pref was referring to changing the pref name, not flipping it. :)
Attachment #660292 - Flags: review?(dtownsend+bugmail)
Attachment #660292 - Flags: review?(dolske)
Attachment #660292 - Flags: review?(dtownsend+bugmail) → review+
Pushed: https://hg.mozilla.org/users/dtownsend_mozilla.com/hotfixes/rev/4b7995fd09f6

Test build:
https://people.mozilla.com/~mnoorenberghe/hotfixes/hotfix-v20120910.01-v2.xpi

Filed bug 790821 to combine hotfixes for OS X.  Linux can probably use this standalone version.
Status: NEW → RESOLVED
Closed: 8 years ago
Hardware: x86 → All
Resolution: --- → FIXED
Matthew, can you please confirm what this hotfix does before I test it? If I understand correctly, this will flip the old background update pref to "false" on an "affected" build. Those builds being:
 * Firefox 15.0a1/a2/b*
 * Firefox 15.0/15.0.1
 * Firefox 16.0a1/a2/b*
 * Firefox 17.0a1/a2
 * Firefox 18.0a1

Is this correct?
Oh, and this only applies to builds on Mac and Linux, right? It will not do anything if installed in a Windows build?

Also, it only applies to "normal" builds, right? It will not do anything if installed in an ESR build?
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #18)
> Matthew, can you please confirm what this hotfix does before I test it? If I
> understand correctly, this will flip the old background update pref to
> "false" on an "affected" build. Those builds being:
>  * Firefox 15.0a1/a2/b*
>  * Firefox 15.0/15.0.1
>  * Firefox 16.0a1/a2/b*
>  * Firefox 17.0a1/a2
>  * Firefox 18.0a1
> 
> Is this correct?

Yes, except it doesn't target the affected builds using the specific version number ranges for each major version.  Instead, it installs in all builds from 15.0a1 to 18.* and checks if app.update.stage.enabled exists and is true. If so, it flips the pref to false and uninstalls,  Otherwise, it just uninstalls itself after doing nothing.

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #19)
> Oh, and this only applies to builds on Mac and Linux, right? It will not do
> anything if installed in a Windows build?

Yes, the targetApplication is set to Linux and Darwin so it should not even install on other operating systems.

> Also, it only applies to "normal" builds, right? It will not do anything if
> installed in an ESR build?

Correct. If it gets installed in an ESR build (for the affected version range above), it will uninstall itself immediately.  Is this correct?

I'm just going repeat here again the known issue that the hotfix may temporarily show in the add-ons manager after uninstallation if you have it open to the extensions tab while it uninstalls.  Re-opening the AOM or toggling the category on the left should update the list of extensions.
I've just sent an email to Softvision instructing them to test this hotfix overnight based on a testplan Matthew and I worked together over the last hour or so. I will report back tomorrow morning with the results of that testing.

For reference, you can see the test plan here:
https://etherpad.mozilla.org/background-update-hotfix
Marking this verified fixed. All results are a PASS. I won't go into the details here, please read the etherpad if interested. For posterity, I've archived the last etherpad revision here:
https://etherpad.mozilla.org/ep/pad/view/background-update-hotfix/RY3JRug9BE
Status: RESOLVED → VERIFIED
Attachment #660292 - Flags: review?(dolske) → review+
I've retested this on Mac and Linux using Fx15 and the updated hotfix from bug 803596, and the hotfix is still addressing this scenario.
You need to log in before you can comment on or make changes to this bug.