498379 - Checking that we aren't updating to an older version of the application fails in certain cases

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Dave Townsend [:mossop]]

When we startup we look for updates to apply. Since bug 313057 we check that the update isn't for an older version of the application already installed. The code that write the update.version includes a trailing newline in the file. THe code that reads the update.version fails to ignore it, so we start doing funky version comparisons line "3.5b99" against "3.5\n". Due to the fun of the version comparator "3.5\n" < "3.5b99" so we delete the update.

Comment 1

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

Is there any way to fix this problem for people on 3.5b99? If I understand what you're writing, there isn't.

Comment 2

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

This is a STOP SHIP issue for 3.0.12, IMO, and will likely require a RC1build2.

Comment 3

[[e-mail] (:bhearsum)]

(In reply to comment #1)
> Is there any way to fix this problem for people on 3.5b99? If I understand what
> you're writing, there isn't.

I've got an idea about how to work around this, I'm about to send mail to r-d.

Comment 4

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

Oh, we haven't shipped 3.0.12 yet, nor code frozen, so yay.

Also, ss pointed out that if we ship RC1build2 as a major update, we might be able to get away without needing to ship a 3.5b100 that contains a fix for this issue, right?

Comment 5

[Dave Townsend [:mossop]]

Fix in progress

Comment 6

[[e-mail] (:bhearsum)]

(In reply to comment #4)
> Oh, we haven't shipped 3.0.12 yet, nor code frozen, so yay.
> 
> Also, ss pointed out that if we ship RC1build2 as a major update, we might be
> able to get away without needing to ship a 3.5b100 that contains a fix for this
> issue, right?

It would be tough to ship a 3.5b100 at this point, anyways. We should do 3.5rc1build2 and 3.5rc2 IMHO.

I can test doing build2 as a major update.

Comment 7

[[e-mail] (:bhearsum)]

(In reply to comment #4)
> Also, ss pointed out that if we ship RC1build2 as a major update, we might be
> able to get away without needing to ship a 3.5b100 that contains a fix for this
> issue, right?

I just tested this - updates are still broken if they're done as a major update.

Comment 8

[[e-mail] (:bhearsum)]

(Sorry, bugzilla made me break the blocking flags)

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This strips off any trailing newline that we read from update.version. We could alternately just not write the newline in the first place but I think this way works better in case people are manually editing things in the updates dir. We could do both of course, your call Rob. I don't think we have a way to test this code?

Comment 10

[[e-mail] (:bhearsum)]

At Mossop's suggestion I just tried using 'extv=3.5rc1' in the snippets, and that made the update work.

I don't know what the implications are for extensions if we actually ship with those, though.

Comment 11

[Dave Townsend [:mossop]]

Rob, there is some inconsistency between how bug 313057 does its version comparison and how bug 485624 does it. The former uses update.extensionVersion (which is what is written to update.version) and compares that to the application version on startup. The latter compares update.version to the application version during running.

We should I think make this consistent, maybe even while we were here. If we made them both use update.extensionVersion then it would actually allow build to go back to using any version name they liked in update.version which would resolve bug 496917 even more nicely. But I'm a little fuzzy on what the semantics of update.version and update.extensionVersion are meant to be, can you clarify?

Comment 12

[Milos Dinic [:Milos]]

If you need some spare tester, ping me. Sorry if this count`s as off topic.

Comment 13

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #4)
> Oh, we haven't shipped 3.0.12 yet, nor code frozen, so yay.

Is this a problem for the just-shipped FF3.0.11?

Comment 14

[[e-mail] (:bhearsum)]

(In reply to comment #13)
> (In reply to comment #4)
> > Oh, we haven't shipped 3.0.12 yet, nor code frozen, so yay.
> 
> Is this a problem for the just-shipped FF3.0.11?

AIUI, it will only cause problems for affected 3.x[abrc] builds that have the bug in them.

"3.0.11" < "3.0.12\n" will be True despite the newline. And since this bug (AFAIK) doesn't exist in any 3.0[abrc] products we should be fine there.

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #4)
> > > Oh, we haven't shipped 3.0.12 yet, nor code frozen, so yay.
> > 
> > Is this a problem for the just-shipped FF3.0.11?

The code that is the problem was not shipped in 3.0.11. It has however been landed ready to ship in 3.0.12 so we should fix this before 3.0.12 ships

Comment 16

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #4)
> > > > Oh, we haven't shipped 3.0.12 yet, nor code frozen, so yay.
> > > 
> > > Is this a problem for the just-shipped FF3.0.11?
> 
> The code that is the problem was not shipped in 3.0.11. It has however been
> landed ready to ship in 3.0.12 so we should fix this before 3.0.12 ships

Thanks for the confirm, mossap, that helps.

Comment 17

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

(In reply to comment #10)
> At Mossop's suggestion I just tried using 'extv=3.5rc1' in the snippets, and
> that made the update work.

So this is a manual edit to the snippets? How will it appear to users?

> I don't know what the implications are for extensions if we actually ship with
> those, though.

Can we get someone to test / confirm what happens here? I'd like to think those called 3.5.* will still be compatible, but that last "." scares me.

Comment 18

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

(In reply to comment #11)
> Rob, there is some inconsistency between how bug 313057 does its version
> comparison and how bug 485624 does it. The former uses update.extensionVersion
> (which is what is written to update.version) and compares that to the
> application version on startup. The latter compares update.version to the
> application version during running.
> 
> We should I think make this consistent, maybe even while we were here. If we
> made them both use update.extensionVersion then it would actually allow build
> to go back to using any version name they liked in update.version which would
> resolve bug 496917 even more nicely. But I'm a little fuzzy on what the
> semantics of update.version and update.extensionVersion are meant to be, can
> you clarify?
I mistakenly used update.version instead of update.extensionVersion in bug 485624 which caused bug 496917.

extensionVersion was added so arbitrary string could override version so extensionVersion would always contain a version string... the years haven't been kind to the update xml.

Comment 19

[Dave Townsend [:mossop]]

Landed on m-c: http://hg.mozilla.org/mozilla-central/rev/1224a2fdf826

Comment 20

[Dave Townsend [:mossop]]

Landed on branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1aa4492403b2

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, how can we verify the fix? We need any build after this checkin, or?

Comment 22

[Dave Townsend [:mossop]]

(In reply to comment #21)
> Dave, how can we verify the fix? We need any build after this checkin, or?

Well this is a bit hacky, but it should give you a result:

Get a 3.5pre tinderbox build (building now)
Set app.update.url.override to https://aus2.mozilla.org/update/3/Firefox/3.5b99/20090605144508/Darwin_Universal-gcc3/en-US/betatest/Darwin%209.7.0/default/default/update.xml to make AUS think it is 3.5b99 on the betatest channel.
Do an update check
It should download the partial, let you restart, fail to apply the partial (because it is against the wrong build), download the complete, let you restart and then you'll magically have the version that the betatest channel is currently offering (3.5rc1build1 right now)

Comment 23

[Dave Townsend [:mossop]]

Pushed to the relbranch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bfada7b09c1b

Comment 24

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

Comment on attachment 383294 [details] [diff] [review]
patch rev 1

Should get this for 1.9.0 as well

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 383294 [details] [diff] [review]
patch rev 1

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment 26

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

Checked in for 1.9.0.12

Checking in mozilla/toolkit/xre/nsUpdateDriver.cpp;
/cvsroot/mozilla/toolkit/xre/nsUpdateDriver.cpp,v  <--  nsUpdateDriver.cpp
new revision: 1.28; previous revision: 1.27
done

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Juan, can we verify this fix with a current or the latest updates? Looks like everything works as expected, right?

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Is there a way to verify this for 1.9.0 (as opposed to 1.9.1) since this in Firefox 3.0.12?