Closed
Bug 1420210
Opened 7 years ago
Closed 7 years ago
Background update is interrupted by About Nightly window
Categories
(Toolkit :: Application Update, defect, P1)
Tracking
()
RESOLVED
DUPLICATE
of bug 1426487
People
(Reporter: bmaris, Assigned: mhowell)
References
(Depends on 1 open bug, Blocks 1 open bug, )
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
[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)
Assignee | ||
Comment 1•7 years ago
|
||
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
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Try push is green.
Attachment #8932215 -
Flags: review?(robert.strong.bugs)
Updated•7 years ago
|
Blocks: 1348087
Keywords: regression
Comment 4•7 years ago
|
||
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+
Comment 5•7 years ago
|
||
(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•7 years ago
|
||
(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)
Assignee | ||
Comment 7•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
The updater change was backed out of beta58 in bug 1423967.
Comment 10•7 years ago
|
||
This has been backed out of beta
Comment 11•7 years ago
|
||
Hey Matt, do we have to back something out for 60?
Flags: needinfo?(mhowell)
Assignee | ||
Comment 12•7 years ago
|
||
Not if we can get bug 1426487 landed in time, but I'm not sure where that's at.
Flags: needinfo?(mhowell)
We'll be backing something out here. Jimm working on it.
Comment 15•7 years ago
|
||
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)
Comment 17•7 years ago
|
||
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)
tracking-firefox60:
--- → blocking
Moving the blocking flag to bug 1442407.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(mhowell)
Comment 19•7 years ago
|
||
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
Closed: 7 years ago
Flags: needinfo?(jmathies)
Resolution: --- → DUPLICATE
Updated•7 years ago
|
status-firefox57:
unaffected → ---
status-firefox58:
unaffected → ---
status-firefox59:
unaffected → ---
status-firefox60:
affected → ---
tracking-firefox60:
+ → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•