312670 - Software update does not set updates.xml correctly after successful install

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Split from bug 310362. This will deal with fixing the UpdateManager to 
accurately report install success (so that the update history will display 
correctly).

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: patch

This patch correctly writes the 'statusText' attribute in updates.xml when a
patch is successfully installed. It's not very elegant, but it was the best of
all the hacks I tried.

The basic problem is that we were changing the statusText of the activeUpdate,
but then we were turning around and deleting the activeUpdate (because the
install was successful) so the new statusText was getting lost. This patch
works by iterating through the updates.xml database directly and correcting the
statusText for the appropriate update. It uses the same matching criteria as
UpdateManager's _addUpdate function.

Ideally there would be more connection between the activeUpdate and the
internal array of updates that is maintained by the UpdateManager... Right now,
changing an attribute on the activeUpdate causes no change to the internal
array. Is this by-design behavior?

Comment 2

[Darin Fisher]

Comment on attachment 199780 [details] [diff] [review]
patch

>+        // Dig through the update history to find the patch that was just
>+        // installed and update its metadata.
>+        for (var i = 0; i < um.updateCount; ++i) {
>+          var umUpdate = um.getUpdateAt(i);
>+          if (umUpdate) {
>+            if (umUpdate.version == update.version &&
>+                umUpdate.buildID == update.buildID) {
>+              umUpdate.statusText = update.statusText;
>+              break;
>+            }
>+          }
>+        }
>+

Do we have to worry about the case where the update is not found?  Also,
are updates being published with BuildIDs now?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Wow, I completely forgot about what might happen if the update wasn't found... 
I'll look into it.

About the buildID, see bug 310362, comment 10. Chase says that updates will 
ship with the buildID tag probably before the next release.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: patch

The case where our active update isn't found in the updates.xml file should be
pretty hard to reach. However, this patch guarantees that the active update
will *always* be in the updates.xml file. Thanks for pointing that out, Darin.

Comment 5

[Darin Fisher]

This patch sort of depends on AUS sending us a BuildID.  The patch looks good to
me, but I have just a small tweak that I'd like to make.  Posting a revised
patch shortly.

Comment 6

[Darin Fisher]

Attachment: v1.1 patch

Comment 7

[Darin Fisher]

Reassigning this bug to myself for easier tracking.

Comment 8

[Darin Fisher]

fixed-on-trunk

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Some folks have reported that this bug still exitst in the trunk build (see 
bug 310362). I'll check into it later today.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: screenshot of update history

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051022
Firefox/1.6a1

WORKSFORME

Comment 11

[Asa Dotzler [:asa]]

We need to get this into the branch before tonight's deadline.

Comment 12

[Darin Fisher]

fixed1.8

Comment 13

[Weston Carloss]

Updating from 2005102503 (first nightly) to 2005102510 (respin) still results in an "install pending" status.  I'm using Windows XP, but I know it also occurs on Linux.  I believe this bug should be reopened.

Comment 14

[Vignir G.]

Attachment: Update history still not right

I can confirm the previous comment. And i think this bug should be reopened!

Comment 15

[Darin Fisher]

bent: can you take another look at this?

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

If this "feature" is going to be so buggy we could just remove it. In retrospect I don't really see a use for a user-visible update history... people just need to know what version they're on now and they get that from the About box. Also, the updates.xml file shows a full history anyway for debugging purposes. bent - what do you think?

Comment 17

[Chase Phillips]

(In reply to comment #16)
> I don't really see a use for a user-visible update history...

We may run into an app or an update problem in the field that is dependent on a particular set of updates being run in sequence.  If that were to happen and the update history is unavailable, we wouldn't be able to use the app's update history to tell us how it got into its current state.  That's an important debugging capability, IMO.

Whether it's user-visible or not is not as important, but I suspect this bug is damaging the user-invisible portions of the update history, too.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The problem definitely still exists in the nightlies, but I can't reproduce it with a fresh (late last night) debug build... I'm on it. Should have something in an hour or so. 

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ok, the problem is coming from somewhere else now. I added some dump statements and got this:

*** UpdateService:_postUpdateProcessing: umUpdate.version = 1.5
*** UpdateService:_postUpdateProcessing: update.version = undefined
*** UpdateService:_postUpdateProcessing: umUpdate.buildID = 2005102422
*** UpdateService:_postUpdateProcessing: update.buildID = undefined

The update variable is set from the active-update.xml file. So somehow we're breaking down between reading the file and setting those attributes.

I'll keep looking... Maybe this is blowback from bug 311613, patch v2?

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, I'm pretty sure that's it. We used to have that static properties list for all update vars, but now they're all independent. There's no code to read active-update.xml at all! Patch coming up.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: update history patch v2.0

Ok, so ignore my last comment. I'll explain this in a sec.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

You should remove the fixed1.8 keyword if this isn't properly fixed on the branch.

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ok, so I was totally wrong in comment #20. What's actually happening is that UpdateManager is clearing _activeUpdate in the the public getter for activeUpdate. It's doing this because the serviceURL prop of the update no longer matches the default serviceURL for the Checker component because it's buildID has changed! This check was added to catch the case where a user changes the update channel, but it's screwing us in this case. Patch v2 is just a workaround for the case in which we're sure we have an update that just succeeded and has therefore changed the default updateURL.

And thanks for fixing the flags Gavin.

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The reason that it kept working for me (and I assume most folks testing these kinds of patches) is that the default serviceURL is set to whatever is in app.update.url.override (if it exists). 

Comment 25

[Darin Fisher]

The correct solution here is to fix the activeUpdate getter to not destroy the updates dir and active-update.xml file when the serviceURL changes.  Instead, it should only check to see if the channel changed.  Given the release schedule, though, I think it's reasonable to take the latest patch as a workaround since I have too many uncertainties about what changing the activeUpdate getter might also do.

Comment 26

[Darin Fisher]

Comment on attachment 200817 [details] [diff] [review]
update history patch v2.0

looks good, i'm testing this now.

Comment 27

[Scott MacGregor]

Comment on attachment 200817 [details] [diff] [review]
update history patch v2.0

a=me and chase

Darin is reviewing and testing it right now.

Comment 28

[Scott MacGregor]

a=asa as well on the 10/25 patch

Comment 29

[Darin Fisher]

Comment on attachment 200817 [details] [diff] [review]
update history patch v2.0

looks good.  passed my tests.

Comment 30

[Darin Fisher]

fixed-on-trunk, fixed1.8

Comment 31

[Darin Fisher]

I filed bug 313830 about fixing this properly after FF 1.5