837117 - Downloaded items change position in the downloads view if Clear List is selected

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130201 Firefox/21.0
Build ID: 20130201030927

Please see the screencast for more details.
http://screencast.com/t/Mvr1d5m7T

Steps to reproduce:
1. Launch Firefox
2. Open the Downloads View: Tools menu -> Downloads
3. Perform 5 downloads of your choice and observe their position in the Downloads View
4. Open the downloads Panel, right click on any of the downloads and select Clear List

Expected results:
The panel's list is cleared and the position of the downloads in the Downloads View is the same.

Actual results:
The panel is cleared but some of the downloads from the Downloads view change position.

Comment 1

[Simona Badau, Desktop QA]

Also, after the Clear List action some of the downloads have a different state: some of them have an Unknown size and others are failed. (please see the screencast before and after the Clear List action).

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

taking for investigation

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

typos ftw!

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0edd11517bcd

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 710283 [details] [diff] [review]
patch v1.0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): downloads panel feature
User impact if declined: Clearing downloads list causes view corruption
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): limited to the feature, fixes typos
String or UUID changes made by this patch: none

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0b46a4903d4c

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0edd11517bcd

Comment 8

[Simona Badau, Desktop QA]

Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20130211 Firefox/21.0 (20130211031055)
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20130212 Firefox/20.0 (20130212042017)

I can still reproduce this issue on the latest Nightly and Aurora - the position of the downloads is still changed after Clearing the list from the panel. Moreover one of the downloads is deleted from the downloads view after clearing the list.

Steps to reproduce:
1. Launch Firefox
2. Open the downloads view
3. Perform 3 downloads 
4. Navigate to: http://bit.ly/159c6t5 and save the first image (save is as it is "index.jpeg")
5. Perform 3 more downloads.
6. Open the Panel, right click on any download and select Clear List.
7. Close the Library window and open it again.

Actual results:

After step 6:
- The downloads list gets reversed.
- The index.jpeg file is removed from the downloads view.
After step 7:
- The downloads list gets reversed again (the order of the downloads is the same as before step 6).
- The index.jpeg file is still missing from the downloads view.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Thank you Simona, everything was working when I tested it, but I surely missed some edge case here :(
I'm investigating your steps to reproduce now, will make a follow-up patch once I figure what's up.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Simona B [QA] from comment #8)
> - The index.jpeg file is removed from the downloads view.

This is expected, cause this image is a data: uri, we don't store data: uris in history, since it is not in history, when it's removed from the downloads list it also disappears from the view.
Here you can find list of protocols we don't add to history:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#1256

Now investigating the order issue.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

There is another edge case that I just found, due to a bug in toolkit we never add downloads history for 0-byte files, thus when you clear history they disappear cause we lack their history information.
I filed bug 840676 about that, I don't think we are going to fix it for Aurora considered it's a "minor" issue, but worth to mention it while I found it.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: part 2 - the missing puzzle piece!

Those entries without any Places data were indeed confusing the code, by causing this._lastSessionDownloadElement to go out-of-sync.  Really good catch with that data uri!

Now doing final testing before asking review.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 713074 [details] [diff] [review]
part 2 - the missing puzzle piece!

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

verified with Simona's STRs.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 713074 [details] [diff] [review]
part 2 - the missing puzzle piece!

first come first serve!

Comment 15

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

Comment on attachment 713074 [details] [diff] [review]
part 2 - the missing puzzle piece!

Yep - this makes sense. Good catch!

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/fdf15fa098d9

Simona, could you please verify this on tomorrow's (13) nightly, so we can uplift to Aurora a verified fix.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 713074 [details] [diff] [review]
part 2 - the missing puzzle piece!

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Downloads panel feature
User impact if declined: broken order of downloads after removals
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): minimal, just updating internal status. Limited to the feature.
String or UUID changes made by this patch: none

Comment 18

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130212 Firefox/21.0
Build ID: 20130212182646

Verified as fixed on the hourly tinderbox build - the position of the downloads remains the same after using the Clear List option from the panel.

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.7.5.

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1426b24daa7

Comment 20

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 20 beta 1 - the position of the downloaded items is not changed when clearing the downloads list.

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.8:
Build ID: 20130220104816
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0