345012 - Session restore triggers lots of exceptions

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Use my profiling firefox build
2)  Start up
3)  Open the Error Console

ACTUAL RESULTS:  A whole bunch of

Error: [Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]"  nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)"  location: "JS frame :: file:///home/bzbarsky/mozilla/profile/obj-firefox/dist/bin/components/nsSessionStore.js :: anonymous :: line 2068"  data: no]
Source File: file:///home/bzbarsky/mozilla/profile/obj-firefox/dist/bin/components/nsSessionStore.js
Line: 2068

EXPECTED RESULTS: no errors

I have no idea how this build got in this state, so I'm not sure how to go about reproducing this in general...  I did modify the nsSessionStore.js code like so:

@@ -2066,6 +2066,7 @@ AutoDownloader.prototype = {
       var file =
         Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
+      dump(this._filename + "\n");
       file.initWithPath(this._filename);

and the output I get is:

  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///tmp/gifmake.htm
  file:///home/bzbarsky/chapter-05.html
  file:///home/bzbarsky/chapter-04.html
  file:///home/bzbarsky/chapter-03.html
  file:///home/bzbarsky/chapter-06.html
  file:///home/bzbarsky/chapter-04.html

All these are indeed not valid filenames.  More importantly, I have no idea where they're coming from.  Furthermore, this is happening even though I did NOT crash on the previous startup.  So I wouldn't even expect session-restore to be running...

Comment 1

[Dietrich Ayala (:dietrich)]

This is caused by SS attempting to restart downloads that were not finished at the previous shutdown. I think the problem is that this occurs even if we're not restoring state. We should only be restarting downloads if we're restoring the previous session. Patch forthcoming.

Comment 2

[Dietrich Ayala (:dietrich)]

Another problem this patch needs to fix is that the restarted downloads were canceled downloads from 3 months ago.

Comment 3

[Simon Bünzli]

Even better would be to only restart downloads after a crash (otherwise Firefox should be able to cancel them itself). And regularly canceled downloads should never be restarted anyway (this might be a different bug though).

Comment 4

[Dietrich Ayala (:dietrich)]

Hrm, I just tried to reproduce this on the branch, and was not able to. The only way I can get a download to restart is by restoring after a crash. Manually canceled downloads were not restarted either. When my trunk finishes building I'll see if I can reproduce it there. 

Comment 5

[Boris Zbarsky [:bzbarsky]]

If there's anything I can attach or any sort of logging I can add to the code to help with diagnosing this, just let me know.

Comment 6

[Simon Bünzli]

It might help if you could attach the downloads.rdf file which triggers these redownloads. I've several times been informed about this issue, but was never able to lay my hands on a defective downloads.rdf for further inquiry.

Afterwards cleaning the downloads list should get rid of the errors, BTW.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: downloads.rdf

Comment 8

[Simon Bünzli]

That file is nicely hosed up. I see 26 references for file:///tmp/gifmake.htm which all point to the same download entry which itself claims to be at the same time downloading and canceled (DownloadState=0 and DownloadState=3). This sounds like a bug (or rather two) in the downloading code.

Several other files (mainly chapter-0?.html) claim to have been initialized as downloads but apparently never got anywhere (DownloadState=0 and ProgressPercent=0). This sounds like a Downloads bug as well.

The first issue might be quite easy to work around. For the second we might consider simply ignoring this edge case and only restart downloads which have already obviously started (ProgressPercent > 0).

BTW: Is downloads.rdf still used in trunk builds at all or has the downloading code also migrated to using SQLite on the Trunk?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> BTW: Is downloads.rdf still used in trunk builds at all or has the downloading
> code also migrated to using SQLite on the Trunk?

The trunk code still uses downloads.rdf, unfortunately :).
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp&rev=1.67&mark=1595#1592

Comment 10

[Boris Zbarsky [:bzbarsky]]

> That file is nicely hosed up.

It's pretty typical, actually.  Almost all the downloads.rdf files I have around for all the profiles I've ever used with Firefox look about like that...  The one exception are the profiles I've never downloaded anything with.  ;)

Please do file bugs on Download Manager as needed and all.

Comment 11

[Simon Bünzli]

While the correct way to fix this would be to fix bug 345117, we should at least make sure that downloads are only resumed after Firefox quits unexpectedly (AKA crashes). Otherwise we'll occasionally get unnecessarily confused users like przemko at http://forums.mozillazine.org/viewtopic.php?t=448797 .

Comment 12

[Simon Bünzli]

Attachment: restart downloads only after a crash

Comment 13

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.35

Comment 14

[Simon Bünzli]

Comment on attachment 232588 [details] [diff] [review]
restart downloads only after a crash

Drivers: This one-line patch restricts restarting the downloads to crashes so that those who always resume their sessions aren't unnecessarily bothered in case their downloads.rdf is corrupted. Risk: very low.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 232588 [details] [diff] [review]
restart downloads only after a crash

a=drivers for MOZILLA_1_8_BRANCH