495680 - Problems with import of downloads.rdf after switch to toolkit download manager

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Sven Grull]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090531 SeaMonkey/2.0b1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090531 SeaMonkey/2.0b1pre

New bug as described in https://bugzilla.mozilla.org/show_bug.cgi?id=381157#c89

Reproducible: Always

Steps to Reproduce:
1. if already exists delete downloads.sqlite
2. start a download
3. in download manager dialog choose "save file"
Actual Results:  
After selecting "save file" it takes quite a long time until windows file selector opens (Probably in this time downloads.rdf is converted to downloads.sqlite - downloads.rdf file size is about 340K).
For every entry in downloads.rdf the error message "x:\...\file.zip could not be saved, because an unknown error occurred. Try saving to a different location." is displayed.
In error console there are several "Error: too much recursion" shown.
After clicking through all error messages the download manager window opens showing the file progress as finished/failed/canceled. 
But no file is really downloaded. 
The partition with the download directory would have enough space to save the files.

Expected Results:  
Import of downloads.rdf happens without error messages.

I tried to strip down downloads.rdf to reproduce the error with a smaller file. But unfortunately the error occurs only with the original file. Because a lot of private urls in the file I can't attach it to this bug. I will try to create a downloads.rdf that triggers this bug the next days.

Comment 4

[Sven Grull]

Attachment: downloads.rdf to trigger error message

This downloads.rdf is a stripped version of my own file that triggers the described error.

Comment 5

[Robert Kaiser]

Could you copy one of the "Error: too much recursion" errors in here (Using the "Copy context menu item in the console)? The location where this happens might give us some info what is failing here. I suspect it's toolkit code but I'm not sure.

Comment 6

[Frank Wein [:mcsmurf]]

So if you first open the Download Manager once and then start a download the problem does not happen?

Comment 7

[Sven Grull]

(In reply to comment #5)
> Could you copy one of the "Error: too much recursion" errors in here (Using the
> "Copy context menu item in the console)? The location where this happens might
> give us some info what is failing here. 

There is no more data available, "Error: too much recursion" is already the copied entry.

(In reply to comment #6)
> So if you first open the Download Manager once and then start a download the
> problem does not happen?

Opening the download manager without starting any download is enough to trigger the errors.

Comment 8

[Robert Kaiser]

Shawn, any idea what could be happening there? This sound like it would be toolkit code, but we have nothing to confirm it...

Comment 9

[Shawn Wilsher :sdwilsh]

well, the download state in the downloads.rdf is set to -1, which means it's in the NOT_STARTED state.  So, we'll go ahead and try to start it.  We check for downloads in this state at startup, and try to start them (startup of the service).

Comment 10

[kmike]

I got a similar error after switching from pre-toolkit-download-manager build to Mozilla/5.0 (X11; U; Linux i686; rv:1.9.1pre) Gecko/20090610 SeaMonkey/2.0b1pre

One of the files has started downloading again after the conversion. Looking in the downloads.rdf, the file entry seems to be corrupted (a couple of duplicate lines):

  <RDF:Description RDF:about="file:///home/mike/NEW/proofforchobos.wmv"
                   NC:Name="proofforchobos.wmv">
    <NC:ProgressMode>normal</NC:ProgressMode>
    <NC:ProgressMode>none</NC:ProgressMode>
    <NC:StatusText>Downloading</NC:StatusText>
    <NC:StatusText>Finished</NC:StatusText>
    <NC:Transferred>0KB of  0KB</NC:Transferred>
    <NC:Transferred>55872KB of 55872KB</NC:Transferred>
    <NC:URL RDF:resource="http://rs425l33.rapidshare.com/files/181009094/3193060/proofforchobos.wmv"/>
    <NC:URL RDF:resource="http://rs425tl2.rapidshare.com/files/181009094/3196738
/proofforchobos.wmv"/>
    <NC:File RDF:resource="file:///home/mike/NEW/proofforchobos.wmv"/>
    <NC:DownloadState NC:parseType="Integer">0</NC:DownloadState>
    <NC:DownloadState NC:parseType="Integer">1</NC:DownloadState>
    <NC:ProgressPercent NC:parseType="Integer">0</NC:ProgressPercent>
    <NC:ProgressPercent NC:parseType="Integer">100</NC:ProgressPercent>
  </RDF:Description>

Also, all converted entries in the new d/l manager contain "0 bytes" in the "transferred" column. Is it a known bug?

Comment 11

[Robert Kaiser]

kmike:
Yes, your file seems to have some kind of strange corruption, having two things in there. The "0 bytes" thing is probably from us setting a number of defaults when importing the old SeaMonkey downloadmanager lists, as there's a lot of things that are not completely compatible in inporting.

Shawn, should we do something at import time so that we don't import erroneaos NOT_STARTED cases?

All in all, I think we should probably ship SeaMonkey 2.0 Beta 1 as it is wrt import, maybe relnote this and gather more feedback from that.

Comment 12

[Shawn Wilsher :sdwilsh]

I guess we could just not import NOT_STARTED downloads.  That's kinda odd that it's in that state in the rdf file though...

Comment 13

[Ian Neal]

Confirming this is a problem, I get this as well, but the 12 entries I have with problems are those with the following in:
<NC:DownloadState NC:parseType="Integer">-1</NC:DownloadState>
No duplicate lines for me.
I am presuming -1 has a meaning that means that download manager tries to process it in some way which it shouldn't.

Comment 14

[Ian Neal]

Just opening the download manager is sufficient, you do not need to start a download to trigger the error messages.
At the moment there are no checks on state in nsDownloadManager::ImportDownloadHistory()
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp#628
Should be fairly easy to fix, but is in toolkit, so cannot really block this beta release on the issue. Saying that we will take a reviewed fix.
Nominate as a blocker for another release if you want.

Comment 15

[Robert Kaiser]

I think we probably should have a one-liner there switching the state of NOT_STARTED download to CANCELED, then those that might have rightfully had that state can always be retried ;-)

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> I think we probably should have a one-liner there switching the state of
> NOT_STARTED download to CANCELED, then those that might have rightfully had
> that state can always be retried ;-)
I concur

Comment 17

[Frank Wein [:mcsmurf]]

Attachment: Patch

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 385834 [details] [diff] [review]
Patch

r=sdwilsh

You'll have to add a unit test (not hard) for this to land on branch though.

Comment 19

[Frank Wein [:mcsmurf]]

Attachment: Patch

now with unit test

Comment 20

[Shawn Wilsher :sdwilsh]

Comment on attachment 388116 [details] [diff] [review]
Patch

>+    if (state == -1)
>+      state = 3;
Could we use the named constants here please so it's clearer in the code as to what is going on?

r=sdwilsh

Comment 21

[Robert Kaiser]

Frank, can we move forward on this please? This is definitely wanted for SeaMonkey 2.0b2, even if there's no way of marking that.

Comment 22

[Frank Wein [:mcsmurf]]

Comment on attachment 388116 [details] [diff] [review]
Patch

Wanted for the next SeaMonkey Beta, this could (in theory) affect users switching from Firefox 2 to Firefox 3.5.x, but as far as I know the FF 2 download manager has never set that state in the downloads.rdf file.

Comment 23

[Robert Kaiser]

Important info for approval and any later readers of this bug (just told it on IRC but should state it here as well):

The relevant code this is patching is only on 1.9.1, the whole downloads.rdf import code has been removed from mozilla-central for the 1.9.2 cycle. Because of that, this patch is only for landing on 1.9.1 and not mozilla-central.

Comment 24

[Samuel Sidler (old account; do not CC)]

Comment on attachment 388116 [details] [diff] [review]
Patch

Approved for 1.9.1.3. a=ss

Comment 25

[Frank Wein [:mcsmurf]]

Pushed to 1.9.1 branch, http://hg.mozilla.org/releases/mozilla-1.9.1/rev/56b99735ac85

Comment 26

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.1.3 using unit test.