Pausing a download fails when done after a retry

VERIFIED FIXED in Firefox 28

Status

()

Firefox
Downloads Panel
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: VarCat, Assigned: mak)

Tracking

26 Branch
Firefox 29
Points:
---

Firefox Tracking Flags

(firefox26+ wontfix, firefox27+ wontfix, firefox28+ verified, firefox29+ verified)

Details

(Whiteboard: [bugday-20131209])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
FF 26.10
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build Id:20131202182626

FF Aurora:
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Build id: 20131203004003

FF Nightly:
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Build Id: 20131202092621

Environments:
Windows 7 x64
Mac Os x 10.9
Ubuntu 12.04 x32

STR:

1. Start a download.
2. Open Download Manager.
3. Cancel the download.
4. Press Retry.
5. After the download restarts pres Pause.

Expected:
The download is successfully paused.

Actual:
The pause button is disabled.

Note:
This bug is intermittent and appears to be easily reproducible downloading medium sized files eg(30-50 Mb). The occurrence of the bug is around 50%.
(Reporter)

Updated

4 years ago
Summary: Pausing a download fails after a retry. → Pausing a download fails when done after a retry

Updated

4 years ago
tracking-firefox26: --- → ?
Is this a regression from Fx25?
Flags: needinfo?(catalin.varga)
(Reporter)

Comment 2

4 years ago
The bug is not reproducible on Fx25 and Fx 25.0.1.
Flags: needinfo?(catalin.varga)
We're too late for FF26.0 but as this is a new regression we'll track for FF27 and try to find a backout or low risk forward fix in early Betas. Let's find a regression window to help narrow the search.
status-firefox26: --- → wontfix
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
tracking-firefox26: ? → +
tracking-firefox27: --- → +
tracking-firefox28: --- → +
tracking-firefox29: --- → +
Keywords: regression, regressionwindow-wanted

Comment 4

4 years ago
Using the steps in comment 0, I can consistently reproduce this bug in the Library window, while in the Panel the Pause command for the same download is enabled.

The strange thing is that I inspected in the debugger the function that determines if the Pause command should be enabled (invoked on right click), and both the "inProgress" and "resumable" properties of the data item are true:

http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/allDownloadsViewOverlay.js#627

The context menu, however, has the Pause command disabled. Marco, Mano, any idea of what might be going on here?

Comment 5

4 years ago
-g 2013-09-24-03-02-02-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
-g 2013-10-02-03-02-01-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
-g 2013-10-06-03-02-01-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
-g 2013-10-08-03-02-02-mozilla-central-firefox-27.0a1.en-US.linux-x86_64 56b0a41985f3
-b 2013-10-09-05-56-29-mozilla-central-firefox-27.0a1.en-US.linux-x86_64 c22969eec61d

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=56b0a41985f3&tochange=c22969eec61d

Checkins with "download": bug 917012, bug 923828.
Whiteboard: [bugday-20131209]
(Reporter)

Comment 6

4 years ago
Thank you Aleksej for providing the regression window.

I would add that during the testing on this bug the mozregression tool reported the following exception:

Downloading nightly from 2013-10-08
Installing nightly
Starting nightly
Was this nightly good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', or
'exit' and press Enter): A coding exception was thrown in a Promise rejection ca
llback.
Full message: ReferenceError: reason is not defined
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Pr
omise
Full stack: onFailure@chrome://browser/content/downloads/allDownloadsViewOverlay
.js:262
@resource://gre/modules/Promise.jsm:575
@resource://gre/modules/Promise.jsm:354

A coding exception was thrown in a Promise rejection callback.
Full message: ReferenceError: reason is not defined
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Pr
omise
Full stack: onFailure@chrome://browser/content/downloads/allDownloadsViewOverlay
.js:262
@resource://gre/modules/Promise.jsm:575
@resource://gre/modules/Promise.jsm:354

A coding exception was thrown in a Promise rejection callback.
Full message: ReferenceError: reason is not defined
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Pr
omise
Full stack: onFailure@chrome://browser/content/downloads/allDownloadsViewOverlay
.js:262
@resource://gre/modules/Promise.jsm:575
@resource://gre/modules/Promise.jsm:354
Keywords: regressionwindow-wanted
(Assignee)

Comment 7

4 years ago
the exceptions are likely Bug 924098, though I can still reproduce the issue with Nightly, where the exception is fixed, so the two things are likely unrelated.
I don't have ideas off-hand, must be debugged.
(In reply to Marco Bonardo [:mak] from comment #7)
> the exceptions are likely Bug 924098, though I can still reproduce the issue
> with Nightly, where the exception is fixed, so the two things are likely
> unrelated.
> I don't have ideas off-hand, must be debugged.

Hey :mak, given we only have a handful of beta's left for Fx27 can you help with the investigation(or recommend someone who can) so we can uplift a low risk solution to avoid shipping with this regression ?

Updated

4 years ago
Flags: needinfo?(mak77)
(Assignee)

Comment 9

4 years ago
I'll take this for now and then figure with Mano who has more time to look into it
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
(Assignee)

Comment 10

4 years ago
I was able to reproduce both in the Library and in the panel, though it depends on how the download is started, if it's started fresh (was not in history and I start from a webpagelink) then I don't see the bug anywhere, if instead it's retried from a previous session history entry (from the library) then .resumable is set to null instead of true, and it is never set to true.

I think the regression window is bogus, since I can reproduce with both patches backed out, should be possible to find a better regression-window using the following steps.

My STR:
1. create new profile
2. go to http://www.thinkbroadband.com/download.html
3. save the 100MB file
4. stop the download
5. Clear List in the panel (so only history of the download is left)
6. Open the Library and retry the download
7. Pause is disabled (dataItem.resumable is set to null, should be set to true)
(Assignee)

Comment 11

4 years ago
So, the issue seems to be that the downloads started by the Library are done through ContentAreaUtils:saveURL, that seems to use DownloadLegacyTransfer.

The nsITransfer is inited here
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#420
notice that null is passed as the temp file, though DownloadsLegacyTransfer doesn't set TryToKeepPartialData if a temp file is not specified
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadLegacy.js#222

So, this appears not to be a recent regression, it's just something that was not updated completely to the new jsdownloads api. If a download is started by the library it used the legacy saver, that doesn't properly handle resuming, nor in the panel nor in the Library.

We may stop using SaveURL, we don't appear to use its features after all, I thought we were prompting to the user for the file location, but looks like we pass aSkipPrompt=true (I'm not sure this is really right if the user choose "ask me every time", though this applies only to undiscoverable cases like drop/paste a link). Then we could directly use Downloads.js api to start a new download.

Alternatively, if we want to prompt properly in the edge cases, we should either fix the legacy implementation to not just obey aTempFile for deciding whether to keep part data, or provide a temp file, but this may not always be wanted for all of SaveURL consumers.

Paolo, any thoughts about this?
Flags: needinfo?(paolo.mozmail)
Keywords: regression
(Assignee)

Comment 12

4 years ago
well, I was just too good in trusting aSkipPrompt, we still prompt the user if useDownloadDir is false, it works inverted, if aSkipPrompt is false then it prompts even if useDownloadDir is true... not really nice.

So, just using jsdownloads api won't make, unless we also use promiseTargetFile to get the prompt.
(Assignee)

Comment 13

4 years ago
Created attachment 8364679 [details] [diff] [review]
wip

in addition to the above issue, I figured the regression range is not completely wrong, there are basically two bugs here, one is that we don't allow to resume files restarted from the Library, the other one is that we call goUpdateDownloadCommands too early, when .resumable is still false. Before bug 917012 we were getting more onStateChange notifications, and those were updating the commands status. We should not just check if state has changed, but also if resumable did.

This still needs some checks and cleanups but is mostly good for feedback
Attachment #8364679 - Flags: feedback?(paolo.mozmail)
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 14

4 years ago
Created attachment 8364680 [details] [diff] [review]
wip

forgot to qref
Attachment #8364679 - Attachment is obsolete: true
Attachment #8364679 - Flags: feedback?(paolo.mozmail)
Attachment #8364680 - Flags: feedback?(paolo.mozmail)

Comment 15

4 years ago
Comment on attachment 8364680 [details] [diff] [review]
wip

My only concern here is that contentAreaUtils.js is shared with applications that don't use the new Downloads API, where the DownloadURL function would just create a hidden download. On the other hand, the function uses promiseTargetFile that is defined there and meant as an internal function.

I'm not sure how to proceed, maybe we can simply preprocess the file and include the new function only if MOZ_JSDOWNLOADS is defined.

(In reply to Marco Bonardo [:mak] from comment #13)
> Before bug 917012 we were getting more onStateChange notifications, and
> those were updating the commands status. We should not just check if state
> has changed, but also if resumable did.

This change will probably fix the UI issue, but we should also check that existing onStateChange consumers are able to handle correctly the case where the state did not actually change, and for example don't display notifications twice.

We should in fact assume that the "resumable" property can change at any time, even asynchronously after a download has completed.
Attachment #8364680 - Flags: feedback?(paolo.mozmail)
(Assignee)

Comment 16

4 years ago
(In reply to :Paolo Amadini from comment #15)
> Comment on attachment 8364680 [details] [diff] [review]
> wip
> 
> My only concern here is that contentAreaUtils.js is shared with applications
> that don't use the new Downloads API, where the DownloadURL function would
> just create a hidden download. On the other hand, the function uses
> promiseTargetFile that is defined there and meant as an internal function.

yes, my concern was mostly contentAreaUtils, though it's the only shared thing across the two windows and has already the promiseTargetFile util. otherwise I should basically duplicate this util in a jsdownloads module, and still contentAreaUtils couldn't use it unless jsdownloads are defined.

> I'm not sure how to proceed, maybe we can simply preprocess the file and
> include the new function only if MOZ_JSDOWNLOADS is defined.

This one may be a good idea.

> (In reply to Marco Bonardo [:mak] from comment #13)
> This change will probably fix the UI issue, but we should also check that
> existing onStateChange consumers are able to handle correctly the case where
> the state did not actually change, and for example don't display
> notifications twice.
> We should in fact assume that the "resumable" property can change at any
> time, even asynchronously after a download has completed.

bug 917012 changed the status, consumers were already handling multiple notifications, so it's unlikely they are not "ready", but I get the point.
If we want to be more bullet-proof, we may either introduce a new onResumableChange notification to nsIDownloadProgressListener, or add a new DOWNLOAD_DOWNLOADING_RESUMABLECHANGED state (existing consumers will just ignore it) or wait to send DOWNLOAD_DOWNLOADING until we asynchronously got the correct resumable state.
Clearly the former change would make this not backportable, since it's an API change (partly also the second suggestion, but it's a const addition, so it would be fine)
You know better the jsdownloads internals than me, so what do you think is more appropriate for it?
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 17

4 years ago
(In reply to :Paolo Amadini from comment #15)
> Comment on attachment 8364680 [details] [diff] [review]
> wip
> 
> My only concern here is that contentAreaUtils.js is shared with applications
> that don't use the new Downloads API, where the DownloadURL function would
> just create a hidden download.

Another alternative is to ifdef this, and if MOZ_JSDOWNLOADS is not defined, fallback to SaveURL()

Comment 18

4 years ago
(In reply to Marco Bonardo [:mak] from comment #16)
> > I'm not sure how to proceed, maybe we can simply preprocess the file and
> > include the new function only if MOZ_JSDOWNLOADS is defined.
> 
> This one may be a good idea.

OK, I think this sounds like the best alternative.

> > (In reply to Marco Bonardo [:mak] from comment #13)
> bug 917012 changed the status, consumers were already handling multiple
> notifications, so it's unlikely they are not "ready", but I get the point.

If you already audited the consumers, then I think we can simply allow multiple notifications for the same state. After all, this change only applies to the Desktop middle layer.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 19

4 years ago
I was starting liking the DOWNLOAD_DOWNLOADING_RESUMABLECHANGED state, consumers may explicitly handle or ignore it, do you see downsides to it?

Comment 20

4 years ago
(In reply to Marco Bonardo [:mak] from comment #19)
> I was starting liking the DOWNLOAD_DOWNLOADING_RESUMABLECHANGED state,
> consumers may explicitly handle or ignore it, do you see downsides to it?

Mainly, that it is not a download state. This means that any time the DOWNLOAD_DOWNLOADING state needs to be checked (including CSS files), the DOWNLOAD_DOWNLOADING_RESUMABLECHANGED state should be checked as well.

onResumableChange might be the valid alternative here. In any case we should see this as a workaround specific to the Desktop middle layer, since the Library front-end is not able to handle the "resumable" change efficiently in onProgressChange.

This is why in the Downloads API we have only one centralized change notification: in the end, every front-end needs to react differently to different combinations of property changes, based on how efficiently they handle the changes.
(Assignee)

Comment 21

4 years ago
(In reply to :Paolo Amadini from comment #20)
> onResumableChange might be the valid alternative here. In any case we should
> see this as a workaround specific to the Desktop middle layer, since the
> Library front-end is not able to handle the "resumable" change efficiently
> in onProgressChange.

the problem is that resumable changes are not notified at all, the Panel works just because it enforces an updateCommands when the contextual menu is opened, surely we may do the same in the Library (or something similar) but that's not very efficient (also cause in the async world we can't wait for the context menu to be opened to fetch async properties to populate it)

> This is why in the Downloads API we have only one centralized change
> notification: in the end, every front-end needs to react differently to
> different combinations of property changes, based on how efficiently they
> handle the changes.

yes, provided the changes are notified somehow.
(Assignee)

Comment 22

4 years ago
I think I will go for the easy workaround, and file a bug to investigate adding some sort of notification for resumable changes
(Assignee)

Updated

4 years ago
Blocks: 950073
Whiteboard: [bugday-20131209] → [bugday-20131209][triage]
We're past the time for speculative fixes on beta, so this is going to be a wontfix there but still tracking for potential forward fix if low-risk enough to take to FF28
status-firefox27: affected → wontfix
(Assignee)

Comment 24

4 years ago
Created attachment 8366884 [details] [diff] [review]
patch

So, this is what I had in mind for a simple Library-only fix of the state update issue, there's still the DownloadURL question open, though it's now ifdefed on MOZ_JSDOWNLOADS
Attachment #8364680 - Attachment is obsolete: true
Attachment #8366884 - Flags: review?(paolo.mozmail)

Comment 25

4 years ago
Comment on attachment 8366884 [details] [diff] [review]
patch

This looks good to me.

Note that on Firefox for Metro the DownloadURL function will be defined, but
will not work until we switch to the jsdownloads API. I guess we're fine with
just refraining from using it :-)
Attachment #8366884 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Comment 26

4 years ago
Tested locally:
1. Restarting a resumable download from the library (that doesn't have an entry in the panel, see comment 10) should allow to pause and resume the download through the contextual menu
1a. it should also be pausable/resumable from the downloads panel
2. drop a link to the downloads widget should start a download
2a. drop a link to the Library downloads view should start a download
3. copy a link and paste into the library downloads view should start a download
3a. copy a link and paste into the downloads panel should start a download
4. all of the above should have the pause action enabled, if the link was resumable
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/87a0f1a43d39
Keywords: checkin-needed
Whiteboard: [bugday-20131209][triage] → [bugday-20131209][triage][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/87a0f1a43d39
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox29: affected → fixed
Resolution: --- → FIXED
Whiteboard: [bugday-20131209][triage][fixed-in-fx-team] → [bugday-20131209][triage]
Target Milestone: --- → Firefox 29
(Assignee)

Comment 29

4 years ago
Comment on attachment 8366884 [details] [diff] [review]
patch

I think it's worth evaluating the uplift of this fix to avoid regressing users download experience

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new downloads api and bug 917012
User impact if declined: starting a download from the Library makes it not resumable, even if it may be
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): the patch is pretty simple and only affects very specific use-cases
String or IDL/UUID changes made by this patch: none
Attachment #8366884 - Flags: approval-mozilla-aurora?
Attachment #8366884 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
No longer blocks: 950073
Whiteboard: [bugday-20131209][triage] → [bugday-20131209]
Keywords: verifyme
(Reporter)

Comment 31

4 years ago
Verified as fixed using the following environment:

FF 28.0b2
Build Id: 20140210161136
OS: Win 7 x64, Ubuntu 13.04 x64, OS X 10.8.5
status-firefox28: fixed → verified
Verified as fixed with latest Aurora on: Win 8 x64, Ubuntu 13.10 x86, OS X 10.8.5.
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
Keywords: verifyme
(Assignee)

Updated

2 years ago
Depends on: 1243445
You need to log in before you can comment on or make changes to this bug.