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.

nominating for rc1

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

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

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?

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.

(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

(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.

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.

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.

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.

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

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

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.

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

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...

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)

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....

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

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!

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

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?

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

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

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?

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?

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

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 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!

whitebard -> qawanted See if we can't pull in more testers

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?

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.

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

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.

And yes, I think v2 should definitely be landed.

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?

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.

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.

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

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 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;

Sorry for the extra crud in the last comment!

(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.

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

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.

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

fixed-on-trunk

Comment on attachment 200686 [details] [diff] [review] UI fixes only (v1.3) - for checkin thanks for attaching a new patch with that review comment.

fixed1.8