1420210 - Background update is interrupted by About Nightly window

Status: RESOLVED DUPLICATE of bug 1426487

(Toolkit :: Application Update, defect, P1)

Description

[Bogdan Maris, Desktop QA]

[Affected versions]:
- Nightly 59.0a1

[Unaffected versions]:
- Beta 58.0b3 and newer
- Release 56-57

[Affected platforms]:
- macOS 10.13
- Ubuntu 16.04 32bit

[Unaffected platforms]:
- Windows 10 64bit

[Steps to reproduce]:
1. Start old Nightly
2. Open About Nightly window
- Update failed. Download the latest version appears.
- Here are the logs until this step: https://pastebin.com/UN7Wbcvf
3. Close About Nightly window
4. Reopen About Nightly
- Download is in progress and a restart is required to update Nightly)
- Here are the logs from step 3 to 4: https://pastebin.com/YaqiUmpH (note that not all the logs are here because downloading Fx spams a lot of logs and Browser Console is not able to record it all).
5. Restart Nightly
- Nightly successfully updated to latest version.

[Expected result]:
- Update is not interrupted by opening About Nightly window and is successfully applied.

[Actual result]:
- See inline STR.

[Regression range]:
- Not sure if this is a regression or not. I could try and find out but it will take some time because it would require to test manually. Let me know if it's worth investing time in this.

[Additional notes]:
- Here is a link to a video with the issue, it was too large to upload it to bugzilla. (https://drive.google.com/open?id=1ElOkdsTNRTzlsXcL9HZLZgtFJBOfKynJ)

Comment 1

[Molly Howell (she/her) (no longer active)]

In the video, opening the about box caused the already-running background download to cancel, and then started a new download. That probably shouldn't happen, but it's not the real bug; the download should have just peacefully resumed. It didn't do that because the patch size check failed; our onProgress handler makes the assumption at [0] that if it's resuming a download, the maxProgress it was given excludes the part that had been downloaded earlier. But it seems that the HttpBaseChannel can set maxProgress to either that or the full size of the file, depending on if it can find the original Content-Length in its cache, according to [1] (which is called from the channel's OnDataAvailable handler, the thing that eventually calls our OnProgress). So to fix this, we're going to need to be able to handle both cases.

I think this is most likely a regression from bug 1348087, which would mean 58 is affected, unless something has changed in the HTTP channel behavior during 59 (which I don't see any evidence for).

[0] https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/toolkit/mozapps/update/nsUpdateService.js#3639
[1] https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/netwerk/protocol/http/HttpBaseChannel.cpp#756

Comment 2

[Molly Howell (she/her) (no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=266c2637259a6ad1aa6568a822318745b8e44e93

Comment 3

[Molly Howell (she/her) (no longer active)]

Attachment: bug1420210.diff

Try push is green.

Comment 4

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

Comment on attachment 8932215 [details] [diff] [review]
bug1420210.diff

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -3631,19 +3631,26 @@ class ChannelDownloader extends CommonDo
Just prior to this is
if (progress > this._patch.size) {

progress also needs to be added with this._resumedFrom as follows
if (progress + this._resumedFrom > this._patch.size) {

>       LOG("ChannelDownloader:onProgress - progress: " + progress +
>           " is higher than patch size: " + this._patch.size);
>       AUSTLMY.pingDownloadCode(this.isCompleteUpdate,
>                                AUSTLMY.DWNLD_ERR_PATCH_SIZE_LARGER);
>       this.cancel(Cr.NS_ERROR_UNEXPECTED);
>       return;
>     }
> 
>-    if ((maxProgress + this._resumedFrom) != this._patch.size) {
>-      LOG("ChannelDownloader:onProgress - maxProgress: " +
>-          (maxProgress + this._resumedFrom) +
>+    let contentLength = 0;
>+    try {
>+      request.QueryInterface(Ci.nsIHttpChannel);
>+      contentLength = parseInt(request.getResponseHeader("Content-Length"), 10);
>+    } catch (ex) {
>+      // No need to handle this exception because the check below will fail.
>+    }
>+    if ((contentLength + this._resumedFrom) != this._patch.size) {
Could you check with the style to see what it recommends for addition and parens in the above if statement? I don't typically bother with the parens.

>+      LOG("ChannelDownloader:onProgress - contentLength: " +
>+          (contentLength + this._resumedFrom) +
>           " is not equal to expected patch size: " + this._patch.size);

r=me with the above

Since there have been a few issues since moving away from nsIIncrementalDownload, the change is on beta, and the change isn't needed on beta I'm tempted to just have the changes backed out on beta so it has more time to bake before it goes to release. What do you think?

Comment 5

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4)
> Comment on attachment 8932215 [details] [diff] [review]
> bug1420210.diff
> 
> >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
> >--- a/toolkit/mozapps/update/nsUpdateService.js
> >+++ b/toolkit/mozapps/update/nsUpdateService.js
> >@@ -3631,19 +3631,26 @@ class ChannelDownloader extends CommonDo
> Just prior to this is
> if (progress > this._patch.size) {
> 
> progress also needs to be added with this._resumedFrom as follows
> if (progress + this._resumedFrom > this._patch.size) {
> 
> >       LOG("ChannelDownloader:onProgress - progress: " + progress +
> >           " is higher than patch size: " + this._patch.size);
Also need to add it to the progress in the LOG message.

Comment 6

[Mike Taylor [:miketaylr]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4)
> Since there have been a few issues since moving away from
> nsIIncrementalDownload, the change is on beta, and the change isn't needed
> on beta I'm tempted to just have the changes backed out on beta so it has
> more time to bake before it goes to release. What do you think?

ni? :mhowell for the question.

Comment 7

[Molly Howell (she/her) (no longer active)]

Thanks Mike, sorry about that.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4)
> Since there have been a few issues since moving away from
> nsIIncrementalDownload, the change is on beta, and the change isn't needed
> on beta I'm tempted to just have the changes backed out on beta so it has
> more time to bake before it goes to release. What do you think?

Yes, I think that's a good idea. There do seem to still be issues.

Comment 8

[Julien Cristau [:jcristau] (back April 22)]

The updater change was backed out of beta58 in bug 1423967.

Comment 10

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

This has been backed out of beta

Comment 11

[Jim Mathies [:jimm]]

Hey Matt, do we have to back something out for 60?

Comment 12

[Molly Howell (she/her) (no longer active)]

Not if we can get bug 1426487 landed in time, but I'm not sure where that's at.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

We'll be backing something out here.  Jimm working on it.

Comment 15

[Jim Mathies [:jimm]]

bug 1426487 isn't looking like it'll make 60, but we still have a week. we'll discuss this at the platint standup this week.

Comment 16

[Jim Mathies [:jimm]]

typical backout: bug 1430846

Comment 17

[Julien Cristau [:jcristau] (back April 22)]

We don't have a week, we're building 60.0b1 early tomorrow, so we would need that backout today.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Moving the blocking flag to bug 1442407.

Comment 19

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

The backout happened on nightly in bug 1442407 and it was uplifted to beta. This issue is already addressed in the patch in progress in bug 1426487 so duping to that bug.