263127 - nsDownloader keeps tempfile open too long

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jesse Ruderman]

When I give nsDownloader a temp file to download to, I run into two problems:

1. onDownloadComplete sometimes happens before the file is fully written.  So
when the program tries to open the file, it sees an empty (?) file.

2. Firefox usually keeps the file open too long, so I can't delete the file
manually.  Also, some programs get a sharing violation when they try to open the
file, perhaps because they open the file read/write instead of read.

These two problems might have the same root cause.  That is, it might be that
whatever keeps the file open too long also prevents it from being flushed when
it should be.

Comment 1

[Jesse Ruderman]

Attachment: testcase (requires chrome privs)

This testcase requires chrome privs.  One way to test it is to install
http://ted.mielczarek.org/code/mozilla/extensiondev/ and paste this testcase
into a chrome Real-Time HTML Editor.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

clarifying summary, the object keeps the file open, not its init function.

onstopr should probably call mSink->Flush() and then release it (in the case
where it wrote to the file itself)

Comment 3

[Ben Bucksch (:BenB)]

Appears as problem for TomTom, can't delete temp files. We need to either fix it or stop using the class.

Comment 4

[Andreas Wuest]

Attachment: possible solution v0

Null out the sink in |OnStopRequest| if we have one.

Comment 5

[Andreas Wuest]

Attachment: patch v1

Also works if we only have a sink but |status| did not indicate success, and flus()es first.

Comment 6

[Andreas Wuest]

biesi says: instead of Flush(), do Close() and the null out.

Comment 7

[Wladimir Palant]

Attachment: Patch v1.1

Replaced Flush() by Close(), made destructor do the same as OnStopRequest() and adjusted style slightly.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 268487 [details] [diff] [review]
Patch v1.1

please write a unit test before checking this in

Comment 9

[Wladimir Palant]

Attachment: xpcshell testcase

Comment 10

[Wladimir Palant]

Attachment: xpcshell testcase

Sorry, used wrong bug number in the testcase.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 268805 [details] [diff] [review]
xpcshell testcase

I'd have used an existing path instead of writing a new request handler, but sure...

also, you have to stop the server in onDownloadComplete, or you'll get xpconnect assertions

Comment 12

[Wladimir Palant]

Attachment: Testcase with biesi's corrections

Comment 13

[Benjamin Smedberg]

I need this for bug 352762 for 1.9

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in netwerk/base/src/nsDownloader.cpp;
/cvsroot/mozilla/netwerk/base/src/nsDownloader.cpp,v  <--  nsDownloader.cpp
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/netwerk/test/unit/test_bug263127.js,v
done
Checking in netwerk/test/unit/test_bug263127.js;
/cvsroot/mozilla/netwerk/test/unit/test_bug263127.js,v  <--  test_bug263127.js
initial revision: 1.1
done