536547 - 3.5.6 is downloading the same version for an update

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[James Andrewartha]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6 (.NET CLR 3.5.30729)

OK, here we go again. 3.5.6 installed via WPKG, I start it up and it immediately starts downloading 3.5.6. I'm not sure whether it'd started downloading when I had 3.5.5 installed, but I don't think so this time.

Reproducible: Sometimes

Steps to Reproduce:
1. Install Firefox via WPKG
Actual Results:  
Firefox tries to update to same version

Expected Results:  
Detected it was already updated.

Program Files\Mozilla Firefox\install.log has 3.5.5 as the app version and is dated 2009-11-09. Local Settings\Mozilla\Firefox\Mozilla Firefox\updates\0\update.status says downloading.

Comment 1

[James Andrewartha]

Attachment: C:\Program Files\Mozilla Firefox\install.log

Comment 2

[James Andrewartha]

Attachment: Local Settings\Application Data\Mozilla\Firefox\Mozilla Firefox\active-update.xml

Comment 3

[James Andrewartha]

updates.xml is just. <updates xmlns="http://www.mozilla.org/2005/app-update"/>

Show Update History says "Firefox 3.5.6 (20091201220228) Security Update
Installed on: Wednesday 16 December 2009 10:15:26 PM
Status: The Update could not be installed (patch apply failed)"

(it'd be nice if you could copy and paste from this window)

Looking at the Samba logs, I was the user on this machine at that time, so perhaps it had detected the update then and started downloading it.

Comment 4

[Justin Dolske [:Dolske]]

(In reply to comment #0)

> OK, here we go again. 3.5.6 installed via WPKG

WPKG? What's that?

Comment 5

[Robert Strong (they/them - no direct email)]

http://wpkg.org/

What appears to be happening is the download starts for a user, the new version is provided by WPKG, the download continues on next startup for the user, it fails to apply.

When updates are going to be provided by something other than Firefox the distribution should really disable app update. I'll see if I can come up with instructions for this.

Comment 6

[James Andrewartha]

Can you make it a flag to the installer? I don't really want to mess around with setting preferences. Even so, I thought this was fixed with bug 485624.

I've reproduced it with 3.5.6, I started the 3.5.7 download, quit before it finished and then had WPKG install 3.5.7, then it was still downloading when I started up 3.5.7. The problem isn't from squid caching it, as it's an https URL. There's only one connection to aus2.mozilla.org in the squid log too.

Actually, looking at https://bugzilla.mozilla.org/attachment.cgi?id=381256&action=diff#mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in_sec5 you only check if an older version is being downloaded, not if the same version is being downloaded.

Comment 7

[Robert Strong (they/them - no direct email)]

How would a flag to the updater solve this?

bug 485624 comment #20 and elsewhere hint or state that the fix there covers trying to update to an older version and not the same version. We don't check for the same version since that would break nightly updates which are for the same version.

When using your own distribution method you really shouldn't have the in app distribution method enabled.

Comment 8

[Robert Strong (they/them - no direct email)]

I guess you were asking about a command line option for the installer to disable app update. I definitely don't want to provide this option since a user not using a separate method for distribution might try it out and then not receive updates.

I'll look into also using the buildID to cancel updating when the app version is the same which should suffice if it is doable.

Comment 9

[James Andrewartha]

Yeah, passing an option to the installer is what I meant. How about an option in the .ini file, is that unobvious enough that random users won't accidentally use it?

The buildID would be preferred by some WPKG users, who do let users with admin rights upgrade Firefox immediately for protection, rather than having to wait for a reboot (which is when WPKG normally runs) http://wpkg.org/Talk:Firefox

Comment 10

[Robert Strong (they/them - no direct email)]

There are already methods for administratively setting and locking a preference when distributing an application and I don't want to one off things as well as add duplicate methods to accomplish the same end result which would then require supporting multiple code paths for supporting the same end result.

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: patch with test rev1

Fairly straight forward but there are still cases where nightly users can end up in this situation though I'm not terribly concerned with that particular case.

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 421009 [details] [diff] [review]
patch with test rev1

I think I'll reschedule the next update for this case and I'll resubmit a new patch.

Comment 13

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

I'll request review after I verify the changes I've made

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 421009 [details] [diff] [review]
patch with test rev1

I'm going to go with the first patch for now and add the rescheduling of the check in another bug since I might want to backport this functionality and not the reschedule.

Comment 15

[Robert Strong (they/them - no direct email)]

Filed bug 539106 for rescheduling of the timer

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 421009 [details] [diff] [review]
patch with test rev1

>     if (update.extensionVersion &&
>-        vc.compare(update.extensionVersion, ai.version) < 0) {
>+        gVC.compare(update.extensionVersion, gApp.version) < 0 ||
>+        update.buildID && update.buildID == gApp.appBuildID) {

Would this mean that if we managed to release a Firefox 3.6.x build with the same timestamp (buildid) as a 3.5.x update, that users wouldn't be able to update? Or would major updates always go through a different code path?

Still, that seems quite unlikely to happen, would be caught in release testing, and is easily fixed by a respin. :-)

Comment 18

[Robert Strong (they/them - no direct email)]

(In reply to comment #17)
> (From update of attachment 421009 [details] [diff] [review])
> >     if (update.extensionVersion &&
> >-        vc.compare(update.extensionVersion, ai.version) < 0) {
> >+        gVC.compare(update.extensionVersion, gApp.version) < 0 ||
> >+        update.buildID && update.buildID == gApp.appBuildID) {
> 
> Would this mean that if we managed to release a Firefox 3.6.x build with the
> same timestamp (buildid) as a 3.5.x update, that users wouldn't be able to
> update? Or would major updates always go through a different code path?
> 
> Still, that seems quite unlikely to happen, would be caught in release testing,
> and is easily fixed by a respin. :-)
You are correct and I changed it to compare both the app version and build ID to prevent this.

Comment 19

[Robert Strong (they/them - no direct email)]

Attachment: patch for checkin

carrying forward r+

Comment 20

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/46267f0a4b3e

Comment 21

[Robert Strong (they/them - no direct email)]

shed followup to fix log message
-          "current build ID: " + gApp.buildID + "\n" +
+          "current build ID: " + gApp.appBuildID + "\n" +

Comment 22

[Robert Strong (they/them - no direct email)]

Attachment: patch for 1.9.2

Comment 23

[Robert Strong (they/them - no direct email)]

Comment on attachment 430185 [details] [diff] [review]
patch for 1.9.2

Changing request to 1.9.2.3... drivers, this is a well baked patch that prevents continuing a download for the same Firefox version when another user or process upgrades the browser to that version.

Comment 24

[Robert Strong (they/them - no direct email)]

Also has xpcshell tests

Comment 25

[Robert Strong (they/them - no direct email)]

Comment on attachment 430185 [details] [diff] [review]
patch for 1.9.2

Canceling approval request.

This patch is included in a rollup patch for 1.9.2 in bug 530872. No estimate yet as to when or for which release it will land

Comment 26

[Robert Strong (they/them - no direct email)]

Comment on attachment 430185 [details] [diff] [review]
patch for 1.9.2

Simple patch with a test that I'd like to get in Firefox 3.6.5.

Comment 27

[LegNeato]

Comment on attachment 430185 [details] [diff] [review]
patch for 1.9.2

I want this fix, but 3.6.6 can't take the risk. We will not be taking this for 3.6.6. Moving approval request forward.

If you disagree, send me an email.

Comment 28

[Robert Strong (they/them - no direct email)]

Not going to make a fuss... just going to point out that this patch has baked for a week shy of 5 months and has tests. Doesn't get much safer than that. ;)

Comment 29

[Robert Strong (they/them - no direct email)]

Comment on attachment 430185 [details] [diff] [review]
patch for 1.9.2

I'm going to add this to a rollup patch in Bug 576939 for 1.9.2

Comment 30

[Robert Strong (they/them - no direct email)]

Fixed on 1.9.2 for Firefox 3.6.9 by bug 576939.