Background update is interrupted by About Nightly window

RESOLVED DUPLICATE of bug 1426487

Status

()

defect
P1
major
RESOLVED DUPLICATE of bug 1426487
2 years ago
a year ago

People

(Reporter: bogdan_maris, Assigned: mhowell)

Tracking

(Depends on 1 bug, Blocks 1 bug, {regression})

59 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment)

[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)
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
Assignee: nobody → mhowell
Status: NEW → ASSIGNED
Priority: -- → P1
Try push is green.
Attachment #8932215 - Flags: review?(robert.strong.bugs)
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?
Attachment #8932215 - Flags: review?(robert.strong.bugs) → review+
(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.
(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.
Flags: needinfo?(mhowell)
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.
Flags: needinfo?(mhowell)
Assignee

Updated

2 years ago
Depends on: 1423967
The updater change was backed out of beta58 in bug 1423967.
Duplicate of this bug: 1423434
Hey Matt, do we have to back something out for 60?
Flags: needinfo?(mhowell)
Not if we can get bug 1426487 landed in time, but I'm not sure where that's at.
Flags: needinfo?(mhowell)
Depends on: 1426487
Duplicate of this bug: 1438801
We'll be backing something out here.  Jimm working on it.
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.
Flags: needinfo?(jmathies)
typical backout: bug 1430846
Flags: needinfo?(jmathies)
We don't have a week, we're building 60.0b1 early tomorrow, so we would need that backout today.
Flags: needinfo?(mhowell)
Flags: needinfo?(jmathies)
Assignee

Updated

a year ago
Flags: needinfo?(mhowell)
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.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(jmathies)
Resolution: --- → DUPLICATE
Duplicate of bug: 1426487
You need to log in before you can comment on or make changes to this bug.