496917 - Can't include an arbitrary version name in the version field of the update xml

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[[e-mail] (:bhearsum)]

However, 3.1b3 -> 3.5b99 does work.

I've also tried temporarily removing the 3.5b4 -> 3.5b99 partial update to make sure a complete was served - that didn't help.

Comment 1

[Tracy Walker [:tracy]]

This is a blocker

Comment 2

[[e-mail] (:bhearsum)]

Mossop did some testing and says he thinks it's a code problem, I'm going to continue looking at a possible cause in the snippets/aus/etc, but we should look at possible code problems too.

Here's the pushlog query for everything between 3.1b3 and 3.5b4:
http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=FIREFOX_3_1b3_RELEASE&tochange=FIREFOX_3_5b4_RELEASE

There's not many update related changesets, here's the ones I found:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b05bd6c2858
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dc680328af08
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/300fca604d16
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2176123cd7c0
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/25e053fc15b3
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/99a0415dbd3a
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a48dcceb93b8
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/55f67feda326


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2176123cd7c0 is the most substantial change, afaict.

Comment 3

[[e-mail] (:bhearsum)]

Some additional information
3.1b3 update url: https://aus2.mozilla.org/update/3/Firefox/3.1b3/20090305152042/WINNT_x86-msvc/en-US/betatest/Windows_NT%205.1/default/default/update.xml?force=1
3.5b4 update url: https://aus2.mozilla.org/update/3/Firefox/3.5b4/20090423204732/WINNT_x86-msvc/en-US/betatest/Windows_NT%205.1/default/default/update.xml?force=1
Dump from the console with app.update.log.all = true:
*** AUS:SVC UpdateService:canUpdate - testing /Users/bhearsum/Desktop/Firefox.app/Contents/MacOS/update.test
*** AUS:SVC UpdateService:canUpdate - testing /Users/bhearsum/Desktop/Firefox.app/Contents/MacOS/updates/0/update.test
*** AUS:SVC UpdateService:canUpdate - able to update
*** AUS:SVC General:readStatusFile - Reading Status File: /Users/bhearsum/Desktop/Firefox.app/Contents/MacOS/updates/0/update.status
*** AUS:SVC UpdateService:_postUpdateProcessing - no status, no update
*** AUS:SVC General:getLocale - getting locale from file: /Users/bhearsum/Desktop/Firefox.app/Contents/MacOS/updater.ini, locale: en-US
*** AUS:SVC Checker:getUpdateURL - update URL: https://aus2.mozilla.org/update/3/Firefox/3.5b4/20090423191946/Darwin_Universal-gcc3/en-US/betatest/Darwin%209.7.0/default/default/update.xml?force=1
*** AUS:SVC Checker:checkForUpdates - sending request to: https://aus2.mozilla.org/update/3/Firefox/3.5b4/20090423191946/Darwin_Universal-gcc3/en-US/betatest/Darwin%209.7.0/default/default/update.xml?force=1
*** AUS:SVC Checker:onLoad - request completed downloading document
*** AUS:SVC Checker:getUpdateURL - update URL: https://aus2.mozilla.org/update/3/Firefox/3.5b4/20090423191946/Darwin_Universal-gcc3/en-US/betatest/Darwin%209.7.0/default/default/update.xml?force=1
*** AUS:SVC Checker:onLoad - number of updates available: 1
*** AUS:SVC UpdateService:downloadUpdate - removing update for previous application version 3.5 Beta 99 (build 2)
*** AUS:SVC UpdateService:addDownloadListener - no downloader!

Comment 4

[Dave Townsend [:mossop]]

I think the problem now is actually in the update XML, not the code. The version attribute is currently "3.5 Beta 99 (build 2)", but I believe we expect this to be a real application version, so when we look at the available updates it appears that the update we are downloading is for an older version than that already installed (3.5b4). This will upset the changes in bug 485624.

Comment 5

[Dave Townsend [:mossop]]

That said we probably shouldn't be left in this crappy state when this happens. Means we are probably not checking that the update offered is less than the currently installed version when we do an update check.

Comment 6

[[e-mail] (:bhearsum)]

(In reply to comment #4)
> I think the problem now is actually in the update XML, not the code. The
> version attribute is currently "3.5 Beta 99 (build 2)", but I believe we expect
> this to be a real application version, so when we look at the available updates
> it appears that the update we are downloading is for an older version than that
> already installed (3.5b4). This will upset the changes in bug 485624.

I just confirmed this behaviour by temporarily changing the OS X 3.5b4 snippets to use version=3.5b99. We can do this for the purposes of getting 3.5b99 out but we'll have to do the same thing for the 3.5b4 and 3.5b99 snippets on at least the beta and betatests channels when we do rc1 - since neither will work with version=3.5 RC 1

Comment 7

[Dave Townsend [:mossop]]

What we should be doing is moving to have AUS output the nice name for the version in the update xml in the name attribute (bug 359082 I believe) and keep the version attribute for the real app version since we need to rely on knowing that. Unfortunately the UI is slightly broken and won't use the nice name currently, though it is a very easy fix.

Comment 8

[Dave Townsend [:mossop]]

Attachment: fix update name display

If we want to continue to be able to display arbitrary version names to the user in the update dialog then we have basically two options that I can see:

We could back out bug 485624 which is the cause of the brokenness we see right now. It isn't really a broken patch, it is doing what it is intended, we are just throwing bogus data into the update system with the current method of showing pretty version names.

We could change AUS to include the pretty version as the name attribute in the XML (not bug 359082 at all), and we would also need to patch the client with the attachment. This isn't ideal either, the patch only affects the main display of the version, there are a couple of other places where it would still be displayed as the real version number, but fixing those looks to be a string change.

Comment 9

[Dave Townsend [:mossop]]

Attachment: ignore old update versions

Additionally, if we keep bug 485624 then we ought to consider taking this patch which makes us ignore old versions in the available update list and causes the broken UI we see with that.

Comment 10

[Dave Townsend [:mossop]]

We ought to block until we decide what the appropriate course of action is here I think.

Comment 11

[[e-mail] (:bhearsum)]

Will this support the release candidate case? In that case, we'll have appv=3.5rc1, extv=3.5, and the internal version number will be '3.5'.

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #11)
> Will this support the release candidate case? In that case, we'll have
> appv=3.5rc1, extv=3.5, and the internal version number will be '3.5'.

So, RC's work fine, if you set the version field to be what it really is, "3.5". In that case for the first RC any user of a beta or lower would see "Firefox 3.5" offered for install and would install it. Later when RC2 is available (also with a version of "3.5" in the XML), all users of RC1 or betas or lower would see it and get to install it, still named "Firefox 3.5".

It gets hairy if you try to actually use "3.5rc1" in the version field so the update dialog displays it as "Firefox 3.5rc1". That would work for the first RC. But if/when the second came along, the application running would be 3.5, it would see the available update as 3.5rc2 which is lower than 3.5 so we hit the failure state.

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #12)
> So, RC's work fine, if you set the version field to be what it really is,

Sounds fine to me.  Let's stay on the cowpaths here if we can; that's a major motivation for doing b99 instead of pre.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

By my understanding of comment 12 and comment 13, this is no longer a blocking issue, so minusing. Renom if I've misinterpreted.

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #14)
> By my understanding of comment 12 and comment 13, this is no longer a blocking
> issue, so minusing. Renom if I've misinterpreted.

If you want to be able to do the sort of version strings in the update dialog that we have been using for security updates up till now ("3.0.12 (build 1)" f.e.) then we need to fix this in the client before release. If you don't want to be able to do that anymore then this is basically a wontfix.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #15)
> (In reply to comment #14)
> > By my understanding of comment 12 and comment 13, this is no longer a blocking
> > issue, so minusing. Renom if I've misinterpreted.
> 
> If you want to be able to do the sort of version strings in the update dialog
> that we have been using for security updates up till now ("3.0.12 (build 1)"
> f.e.) then we need to fix this in the client before release. If you don't want
> to be able to do that anymore then this is basically a wontfix.

That's a false dichotomy. I don't think this issue blocks the final release. Right now it affects users on the beta channel, and while it sucks, I don't think we should keep Firefox out of the hands of the primary audience to fix it.

That said, I'd totally take a fix if one comes along before the other blockers are finished :)

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 382170 [details] [diff] [review]
fix update name display

Well this is the simple fix that would allow us to display a custom name for the update in the UI

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 382171 [details] [diff] [review]
ignore old update versions

And this makes us ignore updates that we would eventually fail to install anyway

Comment 20

[[e-mail] (:bhearsum)]

Dave, do we need any AUS changes to support these patches? What does 'appv' need to look like in the snippets?

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #20)
> Dave, do we need any AUS changes to support these patches? What does 'appv'
> need to look like in the snippets?

Yes, in order to display a custom name in the update dialog the xml will need to contain a name="" attribute on the update element. Unfortunately that needs to be the full update name i.e. "Firefox 3.5" rather than just "3.5" which probably complicates AUS a little. Let's see what Rob thinks of the patch before we start digging too deep in there.

Comment 22

[[e-mail] (:bhearsum)]

(In reply to comment #21)
> Let's see what Rob thinks of the patch
> before we start digging too deep in there.

Okay. I want to note that this will require both AUS and automation tooling changes - we should think hard about whether we want to do something like that this late in the cycle.

Comment 23

[Dave Townsend [:mossop]]

(In reply to comment #22)
> (In reply to comment #21)
> > Let's see what Rob thinks of the patch
> > before we start digging too deep in there.
> 
> Okay. I want to note that this will require both AUS and automation tooling
> changes - we should think hard about whether we want to do something like that
> this late in the cycle.

If we take the client patch everything will continue to behave as it does at the moment. It will merely give us the option to make AUS and automation changes to use custom version names, which we can choose to do at any time after the client has been patched, not necessarily during the current release cycle.

Comment 24

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

Comment on attachment 382171 [details] [diff] [review]
ignore old update versions

Could you create a unit test for this as well?

Comment 25

[Dave Townsend [:mossop]]

Attachment: testcases

This adds testcases to make sure we don't select updates for older application versions and we do select updates for the current application version.

Comment 26

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

Comment on attachment 382487 [details] [diff] [review]
testcases

Thanks!

Comment 27

[Dave Townsend [:mossop]]

Pushed all three as http://hg.mozilla.org/mozilla-central/rev/825c6fc9f69d.

Comment 28

[Dave Townsend [:mossop]]

Attachment: follow-up patch

Per bug 498379 comment 18, let's fix this so we use extensionVersion for the version comparisons so we can use the plain version for our own evil reasons again.

Comment 29

[Dave Townsend [:mossop]]

Landed the follow-up: http://hg.mozilla.org/mozilla-central/rev/d18a8082a617

This is now resolved on trunk.

Comment 30

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

Fixed for 1.9.0.12 by the landing of the followup patch in bug 485624

Comment 31

[Samuel Sidler (old account; do not CC)]

We still need this on 1.9.1, no? It only landed on the release branch, not on the 1.9.1 branch?

Comment 32

[Dave Townsend [:mossop]]

Attachment: rollup patch

This is a combined patch of all the above patches that were landed on trunk. I think it serves us to take this for 1.9.1.1 to keep ourselves in sync as much as possible. This code has been on trunk now for a while with no noticeable problems.

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 387148 [details] [diff] [review]
rollup patch

a1912=beltzner

Comment 34

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

Pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9fd8f0e6e6f5

Comment 35

[[e-mail] (:bhearsum)]

Juan asked me to comment here on verification of this in 3.5.2:
We don't really have a way to do any verification because we have no updates available for 3.5.2 builds. I might be able to hack up something manually if it's really important to verify...but we aren't using the features this enables anymore (now that we're out of the beta period), so it doesn't seem greatly useful.