814099 - Paused downloads do not behave properly in panel after the session is restored.

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1) Start a download, and then pause it.
2) Restart Firefox
3) Open the Downloads Panel, right-click on the paused item, and choose to Resume it.
4) Right-click on the downloading item again.

What happens?

"Resume" is still an option in the context menu, and "Pause" is missing.

The ability to cancel the download is missing from the list. If you click on the "X" to cancel the download, the downloading item does not reflect the cancelled status - it just ceases to receive progress updates.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

It turns out that we prep the dataItem in DownloadsCommon with a getter for the nsIDownload if we load it from the database row.

Changing states overwrites the download attribute - but we can't do that if it's a getter. We have to delete the getter first.

At least, that's my understanding of it.

Comment 2

[:Paolo Amadini]

So, this looks like a regression from bug 808277. Why do we need to assign the
"download" property of the data item explicitly in the first place? Is this just
an optimization, or is it now needed for the summary view? In either case, a
comment there might be helpful.

In fact, the "download" property is rarely accessed directly, so if it's an
optimization, I'm not sure whether it's better to delete and re-assign the
property for each status update, or just let getDownload be executed later.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Paolo Amadini [:paolo] from comment #2)
> So, this looks like a regression from bug 808277. Why do we need to assign
> the
> "download" property of the data item explicitly in the first place? Is this
> just
> an optimization, or is it now needed for the summary view? In either case, a
> comment there might be helpful.

So yes, I introduced this in bug 808277, because I noticed that if I cancelled and restarted a download that was part of the summarized group, the summary would not reflect the status / progress of the restarted download.

Drilling into this, I noticed that this was because the restarted download that was being stored in the dataItem's inside of the DownloadsSummaryData was still referring to a download that was cancelled. I assumed that this was because the reference that the dataItem was holding onto was invalidated, and needed to be updated.

We don't hit this same problem with the indicator, since the indicator iterates over Services.downloads.activeDownloads as opposed to an internal collection of dataItems.

So, my solution was to update the reference. Does that sound like the right approach to you?

Note that the time that the internal download is accessed that I'm concerned about is in the generator _downloadsForSummary in DownloadsSummaryData.

Comment 4

[:Paolo Amadini]

That's true, when a download is retried we call GetDownloadFromDB in
DownloadManager.cpp, creating a different download object from the database.

It makes sense to update the reference in the dataItem, since the download has
the same ID and the new object is added to the list of active downloads.

Comment 5

[Marco Bonardo [:mak]]

So maybe, detect whether dataItem.download is a getter and if not replace it? This would be really really explicit, and if it's still a getter we don't need to do anything.

A nice comment above it explaining the reason we update the reference would be welcome, the current comment in the patch doesn't seem to be as clear as the comments here in the bug.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 684456 [details] [diff] [review]
Patch v2

Thanks Paolo! Ok, I've updated the comment. Is this more clear?

Comment 8

[Marco Bonardo [:mak]]

isgetter = Object.getOwnPropertyDescriptor(dataItem, "download").value === undefined

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3

Ok, cool - I'll use Marco's suggestion here.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 684466 [details] [diff] [review]
Patch v3

So I'm not sure who to direct this review request to, so I'll just default to Marco.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 684466 [details] [diff] [review]
Patch v3

Review of attachment 684466 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +764,5 @@
> +    // it alone - the next time the download property is accessed, the right
> +    // download object will be retrieved.
> +    //
> +    // Otherwise, we overwrite the download property with the passed in
> +    // nsIDownload.

I think this last phrase is just repeating concepts from the first one ("might now need updating"), so I'd remove it.

@@ +767,5 @@
> +    // Otherwise, we overwrite the download property with the passed in
> +    // nsIDownload.
> +    let isGetter = Object.getOwnPropertyDescriptor(dataItem, "download")
> +                         .value === undefined;
> +    if (!isGetter) {

I'm sorry that I may have misguided you, I was leaving and didn't have enough time to comment properly. I meant "the usual way we do is-getter check is...", and ended up suggesting a temp isGetter var with pseudocode :)

Btw, now that I look at it, it seems to add readability value. So maybe just rename to downloadIsGetter (otherwise I'm fine with just putting the condition in the if).

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4 - r+'d by mak

Ok, gotcha. I agree, I prefer the alias for readability - so I've kept it in.

Thanks!

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Landed on mozilla-inbound as: https://hg.mozilla.org/integration/mozilla-inbound/rev/78ed9104dadd

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78ed9104dadd

Comment 15

[Simona Badau, Desktop QA]

Verified on the latest Nightly that the paused downloads behave properly after the session is restored. After restoring the session and resuming the download, the items in the context menu are:
Pause
Open Containing Folder/Show In Finder(on Mac OS X)
Go To Download Page
Copy Download
Clear List.

Verified on Ubuntu 12.10, Mac OS X 10.7 and Windows 7:
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121203 Firefox/20.0 Build ID: 20121203030801
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20121203 Firefox/20.0 Build ID: 20121203030801
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20121202 Firefox/20.0 Build ID: 20121202030723