311613 - UI doesn't update when going from partial-> full update

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Marcia Knous [:marcia]]

During tonight's software update testing, Asa and I saw the following - 
STR:

1.  Download and install FFBeta1 with a fresh profile
2.  Force a partial update by hange the channel update pref and modifying the
firefox.js file
3.  Launch Firefox. It finds the partial update and tries to apply it and
fails.es it. When it goes to get the full update, the UI stays frozen showing a
complete partial update downloaded.  The dialog closes and the full update is
applied, but not a good user experience overall.

Comment 1

[Marcia Knous [:marcia]]

nominating for rc1

Comment 2

[Asa Dotzler [:asa]]

This also appears on Windows though for only a moment before the new full
download progress replaces it.

Comment 3

[Asa Dotzler [:asa]]

Ben, we need your help here. Can you dig into this?

Comment 4

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

I've tried to reproduce this multiple times on WindowsXP Pro (SP2) and haven't 
been able to do it with either b1 or b2. Each time the partial downloads and 
succeeds. Are there any other steps to reproduce this one?

Comment 5

[Darin Fisher]

In the past, we had a few bad partial updates that resulted in everyone getting
complete updates.  Those few times, I recall seeing this bug.  If the partial
update was 500k, then I would see the progress meter at 100% until more than
500k worth of complete update got downloaded.  I suspect this means that some
field in active-update.xml is not set correctly when we go to try to download
the complete update.

Comment 6

[Tracy Walker [:tracy]]

(In reply to comment #2)
> This also appears on Windows though for only a moment before the new full
> download progress replaces it.

I saw the same behavior on Linux updating from 1012 -> 1013

Comment 7

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

(In reply to comment #0)
> 2.  Force a partial update by hange the channel update pref and modifying the
> firefox.js file

What does "modifying the firefox.js file" mean?

Are you using a different update URL? I tried changing the channel to "nightly"
in both places and I can't see this. I believe you, but I can't reproduce. I'll
try a couple of other things and report back. 

Comment 8

[Asa Dotzler [:asa]]

Ben, we just add a blank line to the firefox.js file so it will force a full
update rather than a partial. That way you get the partial, it fails to apply
and triggers a full update. 

Comment 9

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

Ah, I finally figured out how to reproduce this one.

Folks, adding blank lines to firefox.js didn't cause the partial updates to 
fail for me. I was, however, able to simulate the bad UI experience you 
described by doing the following:

1. Hack the channel file so that you're on the nightly channel
2. Download a partial update and, when asked, say that you'll install it later
3. Close firefox
4. Go into the updates/0 folder and hack update.status from 'pending' 
to 'download-failed'
5. Restart firefox

Something that I think is *really* bad is that if you were to make 
update.status say 'failed' instead of 'download-failed' you get *zero* 
notifications... I'll file another bug on that in a bit.

Anyway, once you do the five steps above the UI shows a complete download of 
the partial patch for a while. After about 1mb has been downloaded, however, 
the UI suddenly refreshes itself and shows the progress of the full patch 
download.

I'm doing all of this from my favorite coffee shop (with a relatively slow 
internet connection), so I'm betting that the reason you guys never see 
anything is that your download completes much sooner than mine. 

Comment 10

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

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051017 
Firefox/1.4.1

Actually, in the latest nightly setting the status to 'failed' gives the same 
result as 'download-failed'... The UI behaves as previously described.

Comment 11

[Asa Dotzler [:asa]]

Darin, I think you mentioned yesterday that you might know what's going on here? 

Comment 12

[Chase Phillips]

Josh, given that this affects Mac heavily, could you take a look, too?

Comment 13

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

Attachment: patch

This patch tightens up the UI while downloading an update. The basic cause of
this bug is that the update manager is saving the 'status' and 'progress'
properties of the patch object to disk. That is, these two properties are
recorded in active-update.xml. Furthermore (and I think this is the *real* bug)
the 'status' and 'progress' values for the partial patch are getting duplicated
onto the entry for the complete patch.

The situation Marcia reported happens like this: The downloader pulls the
partial patch from AUS. Every time it gets another chunk it updates the
'progress' and 'status' values for *both* the partial and complete patches.
These (duplicated) values get written to active-update.xml and persist. Once
the partial patch finishes downloading then firefox restarts. If for any reason
the partial fails to apply then firefox will present you with the error dialog
and offer to download the complete patch. At this point the active-update.xml
file has already been parsed and the bogus 'progress' and 'status' values have
been loaded as properties of the complete patch. The UI then sets itself up
according to those values and you finally see the bug.

My approach to fixing this one was simple at first: I just tweaked the patch
class so that it no longer serializes 'status' or 'progress' properties. I know
this isn't the way this bug should be fixed, but given the short time frame
until the close for rc1 I figured it was worth a shot. Anyway, I don't think
that those values are ever used/loaded by any other components, and the
incremental downloader doesn't need them to do its job at all. Simply pulling
those values almost resolved this bug.

Unfortunately the UI doesn't really initialize itself well starting from the
patch-error page. I also had to rework the UI notifications a bit to make it
more responsive and accurate. I think everything is working pretty well now.

So the next step... Obviously writing 'progress' and 'status' values for both
partial and complete patches at once is a bad thing. I took the quickest way
around that. If I have enough time I'll see if I can't fix this in a better
way, but I don't know if I can do it before rc1. The rest of the UI changes are
probably going to be necessary in any case because the UI just doesn't pass the
sniff test when you've already had a patch failure.

Review from anybody welcome.

Comment 14

[Asa Dotzler [:asa]]

rob and mike, can you help us with reviews here?

Comment 15

[Darin Fisher]

I wonder why we can't just solve this bug more simply by not copying the
progress and status fields onto the 'complete' update object.  Investigating...

Comment 16

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

Darin, to simulate that:

1. Download a partial update.
2. Select 'Later' when asked to install it.
3. Modify update.status to say 'failed'.
4. Modify active-update.xml to remove the 'progress' and 'status' elements.
5. Restart firefox.

(Or, just take the changes in the patch made to nsUpdateService.js.in and do 
steps 1-3)

Comment 17

[Darin Fisher]

This bug is simply caused by the fact that each UpdatePatch shares a common
'static' properties map!!  The "_properties" map is stored on
UpdatePatch.prototype, so the bug is simply a misuse of JS.  We can fix this
with a one line change to allocate a separate map for each UpdatePatch instance :-(

We have the same bug on the Update object as well, and possibly on other objects.

Damn!  This same bug bit us in EM code as well :-(

Patch coming up....

Comment 18

[Darin Fisher]

Attachment: v2 patch [fixed-on-trunk, fixed1.8]

Here's the patch that I think will solve the bug.

Comment 19

[Darin Fisher]

To test this patch out, I downloaded a partial update, then I closed the browser
without restarting.  Then, I loaded the generated active-updates.xml file into
another browser window, and I found that the "progress" and "status" fields were
only set for the partial update and not the complete update as well.  This
should fix the bug.  Please help me test this if you can.  Thanks!

Comment 20

[Darin Fisher]

I verified after running through a failed partial that the progress meter for
the ensuing complete update download looked as it should.

Comment 21

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

The progress meter with the patch behaved as it should for me as well when
downloading a complete update with a partial that failed.

Darin - in relation to it affecting the EM are you referring to the multiple
update check bug which has been fixed or a bug that is still present?

Comment 22

[Darin Fisher]

No, I was referring to a bug that was encountered when I was working on
implementing support for installing extensions via the Windows registry.

Comment 23

[Asa Dotzler [:asa]]

ben, and rob, can you guys review this so we can get it landed ASAP.

Comment 24

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

This fix explains a lot! I kept getting weird failures when messing with 
get/setProp and I couldn't figure it out (that's why I abandoned properties in 
patch v1). At least now I know that I'm not crazy! Thanks Darin!

With v2 the active-update.xml keeps the complete patch separate from the 
partial nicely. In the UI, the status text stays at the default 'Connecting to 
the update server...' just as it should.

However, all of the other UI problems still exist. For instance:

I still see a big delay between when the update begins downloading and when 
the status text changes to something like '1.0 mb downloaded...' (It 
says 'connecting' sometimes for a while and then suddenly jumps to '3.6 mb 
downloaded', so it must have downloaded a bunch without notifying me).

When I pause a download, hide it, and then resume it I see several things: the 
status/progress don't update for a while, and the throbber/progress bar are 
inactive (Initially, they both 'spin' while connecting).

Also, when I pause, sometimes I get the wrong text displayed (I'll attach a 
screenshot in a sec).

These UI glitches are all addressed in patch v1. Could we maybe merge these 
two?

And I gather that this bug is more pronounced on Macs (comments 1 - 3). Any 
mac users up for testing?

Comment 25

[Darin Fisher]

OK, hmm... I haven't been seeing those glitches on my machine.

Ben: You're testing on Windows, right?  What part of the v1 patch fixes the
glitches?

Comment 26

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

Yes, I'm using windows, and everything except the changes to 
nsUpdateService.js.in are there to fix those UI bugs.

Comment 27

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

Attachment: screenshot of incorrect text

As promised, here's the screenshot of the bad text. It should say 'xxx
downloaded so far'. Before these patches the status text would revert to the
value of the 'status' element in the active-update.xml file.

Comment 28

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

Comment on attachment 200410 [details] [diff] [review]
v2 patch [fixed-on-trunk, fixed1.8]

r=me in any case: we need this patch. 

Now let's just get the UI figured out!

Comment 29

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

whitebard -> qawanted

See if we can't pull in more testers

Comment 30

[Mike Schroepfer]

Ben - thanks for the help on this and the really detail and detailed
explainations of the issues.  What testing help specifically do you need on the
v1 patch.   Should we at least land v2 in the meantime?

Comment 31

[Darin Fisher]

schrep: we should land the v2 patch at least, but it sounds like we're going to
want more than just that.  i'm going to try to reproduce this locally so that i
can then try out Ben's patch.

Comment 32

[Darin Fisher]

v2 patch is fixed-on-trunk since drivers usually want patches on the trunk for a
while before approving the change for the branch.

Comment 33

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

Attachment: UI fixes only (v1.1)

This patch addresses the UI problems only, meant to be applied in addition to
Darin's v2.

schrep: I'd love someone with a mac to check a pre- and post-patched (v1.1 AND
v2) build to see how things work. The UI is a little buggy on windows, but it
sounds like the mac version looks worse.

Comment 34

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

And yes, I think v2 should definitely be landed.

Comment 35

[Darin Fisher]

Ben: can you walk me through the UI changes, explaining why each change is being
made.  Or, at least give me a high-level overview of what the core problems are
and how these changes address those problems.  The change to _setStatus look
interesting... was it simply the case that we were spending a lot of CPU time
updating the UI to some old state that was causing the glitches?

Comment 36

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

Darin: I can surely do that, but now that properties are working I'd like to 
tweak the patch a little (I totally quit relying on properties because I kept 
getting weird behavior). I posted v1.1 just to highlight the UI glitches that 
should be fixed. 

Here's what gets fixed in v1.1:

1) There are two things that update the UI now: the downloader and the 
pause/resume button. Because the downloader is asynchronous, each mechanism 
checks and ensures that the UI is doing the appropriate thing when it receives 
an event. For instance, onStartRequest only messes with the UI if the 
pause/resume says we are not paused. Another example: clicking resume 
immediately kicks the UI back into download mode without waiting for the 
onStartRequest to fire. There are several other fixes of this nature in the 
patch. This asynchronicity is the main cause of the remaining UI bugs 
described in comment 24.

2) The status text doesn't get cached anymore. It's either 'Connecting', a 
paused 'x downloaded so far' phrase, or it's built directly from onProgress.

3) Duplicate calls to _setStatus are ignored (not a huge save, but it takes 
care of doing unnecessary work caused by rounding the amount of data received 
in the status string).

Anyway, just to reiterate: v1.1 is not something I would like to see landed. 
It needs a little bit of tweaking so that it again uses properties. I'll get 
to that later this afternoon, I think.

Comment 37

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

Attachment: UI fixes only (v1.2)

Ok, here's what I think we should do to fix this bug. It only works after applying darin's v2 patch. The patch looks a little complicated because I changed the order of some calls a little to increase readability of the patched code.

Comment 38

[Darin Fisher]

Ok, thanks for the details and the revised patch.  I'll review this today.

Comment 39

[Darin Fisher]

Comment on attachment 200532 [details] [diff] [review]
UI fixes only (v1.2)

beng is back, and said that he will be looking at this.

Comment 40

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

Comment on attachment 200532 [details] [diff] [review]
UI fixes only (v1.2)

>+    var patch = gUpdates.update.selectedPatch;
>+    patch.QueryInterface(Components.interfaces.nsIWritablePropertyBag);

Why move this outside the else, away from the only code that uses it? 

This tests well for me - no more wedged progress meter, so r=ben

>     if (this._paused)
>       updates.downloadUpdate(gUpdates.update, false);
>     else {
>-      var patch = gUpdates.update.selectedPatch;
>-      patch.QueryInterface(Components.interfaces.nsIWritablePropertyBag);
>-      patch.setProperty("status", 
>+      patch.setProperty("status",
>         gUpdates.strings.getFormattedString("pausedStatus", 
>-          [this.statusFormatter.progress]));
>+        [this.statusFormatter.progress]));
>       updates.pauseDownload();
>     }
>     this._paused = !this._paused;
>@@ -1192,7 +1186,17 @@
>     request.QueryInterface(nsIIncrementalDownload);
>     LOG("UI:DownloadingPage", "onStartRequest: " + request.URI.spec);
>     
>-    this._downloadThrobber.setAttribute("state", "loading");
>+    // This !paused test is necessary because onStartRequest may fire after
>+    // the download was paused (for those speedy clickers...)
>+    if (this._paused)
>+      return;
>+      
>+    if (!(this._downloadThrobber.hasAttribute("state") &&
>+          (this._downloadThrobber.getAttribute("state") == "loading")))
>+      this._downloadThrobber.setAttribute("state", "loading");
>+    if (this._downloadProgress.mode != "undetermined")
>+      this._downloadProgress.mode = "undetermined";
>+    this._setStatus(this._label_downloadStatus);
>   },
>   
>   /** 
>@@ -1208,23 +1212,32 @@
>    */
>   onProgress: function(request, context, progress, maxProgress) {
>     request.QueryInterface(nsIIncrementalDownload);
>-    LOG("UI:DownloadingPage.onProgress", request.URI.spec + ", " + progress + 
>+    LOG("UI:DownloadingPage.onProgress", " " + request.URI.spec + ", " + progress + 
>         "/" + maxProgress);
> 
>+    var name = gUpdates.strings.getFormattedString("downloadingPrefix", [gUpdates.update.name]);
>+    var status = this.statusFormatter.formatStatus(progress, maxProgress);
>+    var progress = Math.round(100 * (progress/maxProgress));
>+
>     var p = gUpdates.update.selectedPatch;
>     p.QueryInterface(Components.interfaces.nsIWritablePropertyBag);
>-    p.setProperty("progress", Math.round(100 * (progress/maxProgress)));
>-    p.setProperty("status", 
>-      this.statusFormatter.formatStatus(progress, maxProgress));
>+    p.setProperty("progress", progress);
>+    p.setProperty("status", status);
>+    
>+    // This !paused test is necessary because onProgress may fire after
>+    // the download was paused (for those speedy clickers...)
>+    if (this._paused)
>+      return;
> 
>-    this._downloadProgress.mode = "normal";
>-    this._downloadProgress.value = parseInt(p.getProperty("progress"));
>+    if (!(this._downloadThrobber.hasAttribute("state") &&
>+         (this._downloadThrobber.getAttribute("state") == "loading")))
>+      this._downloadThrobber.setAttribute("state", "loading");
>+    if (this._downloadProgress.mode != "normal")
>+      this._downloadProgress.mode = "normal";
>+    this._downloadProgress.value = progress;
>     this._pauseButton.disabled = false;
>-    var name = gUpdates.strings.getFormattedString("downloadingPrefix", [gUpdates.update.name]);
>     this._downloadName.value = name;
>-    var status = p.getProperty("status");
>-    if (status)
>-      this._setStatus(status);
>+    this._setStatus(status);
>   },
>   
>   /** 
>@@ -1259,7 +1272,10 @@
>     LOG("UI:DownloadingPage", "onStopRequest: " + request.URI.spec + 
>         ", status = " + status);
>     
>-    this._downloadThrobber.removeAttribute("state");
>+    if (this._downloadThrobber.hasAttribute("state"))
>+      this._downloadThrobber.removeAttribute("state");
>+    if (this._downloadProgress.mode != "normal")
>+      this._downloadProgress.mode = "normal";
> 
>     var u = gUpdates.update;
>     const NS_BINDING_ABORTED = 0x804b0002;

Comment 41

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

Sorry for the extra crud in the last comment!

Comment 42

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

(In reply to comment #40)
> >+    var patch = gUpdates.update.selectedPatch;
> >+    patch.QueryInterface(Components.interfaces.nsIWritablePropertyBag);
> Why move this outside the else, away from the only code that uses it? 
> This tests well for me - no more wedged progress meter, so r=ben

Oops, that was probably left outside the else because I must have tried using it in the other case. It can definitely go back in.

Comment 43

[Mike Schroepfer]

Are you guys ready to land or is this still in process?

Comment 44

[Mike Schroepfer]

Comment on attachment 200532 [details] [diff] [review]
UI fixes only (v1.2)

Preemptive approval - if you attach a new patch please re-request approval.  Otherwise land away.

Comment 45

[Darin Fisher]

Attachment: UI fixes only (v1.3) - for checkin

OK, this is the version that has the QI call in the better place.  I've landed this change on the trunk.

Comment 46

[Darin Fisher]

fixed-on-trunk

Comment 47

[Scott MacGregor]

Comment on attachment 200686 [details] [diff] [review]
UI fixes only (v1.3) - for checkin

thanks for attaching a new patch with that review comment.

Comment 48

[Darin Fisher]

fixed1.8