Closed Bug 595333 Opened 9 years ago Closed 9 years ago

Downloads started during DownloadView initialization are added twice

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: wesj, Assigned: wesj)

Details

Attachments

(1 file)

This is only noticeable in Fennec after the patch in Bug 594504. The patch opens the downloads manager, and sends a progress event at around the same time for a new queued download. As a result, both the download progress listener, and the nsIDownloadManager create richlistitems for the download, and it shows up twice.
Here I just check every time an item is created to make sure one with the same id doesn't already exist. I am a bit worried about its performance implications if the list is huge, but it seems like an easy way to solve any related bugs.
Attachment #474192 - Flags: review?(mark.finkle)
Comment on attachment 474192 [details] [diff] [review]
Check before creating

This patch would work OK, but I was curious to figure out why this was happening at all. Here is my guess:

* _createItem is called in two places: when we fill the full list and in downloadStarted, called via a DownloadProgressListener
* The listener is not connected until the DownloadsView is fully initialize - which happens when the view is made visible.
* Tapping the alert notification starts a download and then makes the view visible.
* In the view initialization code, the listener is connected first then the full list is loaded async.

I think the listener is in a race with loading the full list, and it might be winning.

I want to check to see if just putting the check for the existing item in the downloadStarted method is enough to fix this bug or not.

If it gets more complicated, we can use this patch. But if we can move the check out of _createItem, it would mitigate the performance risk Wes pointed out.
It is a race between the listener and the full list code. Because the full list code is async, it is hard to simply delay the listener. We could add a state flag to let us know when loading begins and ends, but the patch here is simpler and works without much overhead.
Comment on attachment 474192 [details] [diff] [review]
Check before creating

if(  -> if (

(must break Wes of this)
Attachment #474192 - Flags: review?(mark.finkle) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/cb86fc4a74bf
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
bugspam
Assignee: nobody → wjohnston
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110913
Firefox/9.0a1 Fennec/9.0a1
Device: Samsung Galaxy S
OS: Android 2.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.