900977 - Connection reset when uploading a file doesn't free the handle

Status: NEW

(Core :: Networking: HTTP, defect, P3)

Description

[Vittorio Giovara]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

I was trying to upload a (rather small) file of about 50MB in form, when the connection to server was reset.


Actual results:

I wanted to delete the file I was uploading after the upload was interrupted by the reset, but I could not.

The OS warned me that the file was still in use by Firefox.


Expected results:

Well at the connection reset Firefox should have closed the uploaded file handle and let me free to delete my file.

Comment 1

[Vittorio Giovara]

The form was a bugreport at Sourceforge.net

Comment 2

[Loic]

Sometimes, it may happen. Are you able to reproduce the issue if you reset your web connection during uploading?

Comment 3

[Vittorio Giovara]

Yes, I was able to reproduce again.
Not even closing the tab freed the handle, I had to reopen the page and refresh it to free it.

Comment 4

[Loic]

Did you try with other browsers? Same behavior?

Comment 5

[Boris Zbarsky [:bzbarsky]]

This is a necko issue, actually: necko is not reading to the end of the "close-on-EOF" stream and also not closing it...  Nothing the form submission code can really do about it: it doesn't own the stream, and necko does.

We really need to fix the API for necko's upload streams.  :(

Comment 6

[Patrick McManus [:mcmanus]]

cc: nick and steve.. do either of you have the bandwidth to pick this up in the next few weeks? Its a pretty easy "something to on the side" bug afaict.

Comment 7

[u408661]

I can most likely find the cycles for this.

Comment 8

[u408661]

Attachment: 0001-Bug-900977-properly-close-mRequestStream-when-shutting-down-a-transaction.-r-mcmanus.patch

So setting up for that took way longer than writing the patch itself...

I was able to reproduce no problem on mac, with lsof showing an open file descriptor hanging around after the post gets reset. With the addition of this simple patch above, lsof no longer shows an open file descriptor hanging around after the connection is reset.

The patch is running through try right now https://tbpl.mozilla.org/?tree=Try&rev=3beb9ebd1dd7

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 790277 [details] [diff] [review]
0001-Bug-900977-properly-close-mRequestStream-when-shutting-down-a-transaction.-r-mcmanus.patch

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

thanks!

Comment 10

[u408661]

*sigh* and of course, this one-liner breaks pretty much every POST test we have. Joy. Will have a new patch incoming once the oranges are figured out.

Comment 11

[u408661]

OK, quick update. I've figured out one set of test failures (specifically netwerk/test/unit/test_307_redirect.js) is caused by re-using the same request body stream in 2 different transactions because of a redirect (it gets closed by the first transaction, then when the second one tries to use it, gets the error NS_BASE_STREAM_CLOSED).

Moving the Close of the stream to OnStopRequest fixes this issue, but there are still failures in some mochitests that I'm in the process of tracking down.

Comment 12

[u408661]

(In reply to Nicholas Hurley [:hurley] from comment #11)
> OK, quick update. I've figured out one set of test failures (specifically
> netwerk/test/unit/test_307_redirect.js)

I forgot to mention, this was also the cause of the failures in mochitest-browser-chrome. Mochitest 1 and 2 are still broken, though.

Comment 13

[u408661]

More update (as much for my memory as anything):

The saga of multiple people holding strong refs to the post data stream continues. For at least one of the mochitest failures (I suspect for the others, as well), the problem comes because there is an nsSHEntry that also holds a ref to the post data stream. In the case of docshell/tests/test_bug413310.html, the post completes as expected, so an entry is created in session history that holds a reference to the data stream. Sometime after that SHEntry is created, nsHttpChannel::OnStopRequest fires and closes the data stream. This particular mochitest then causes a history.back over the POST, which then tries to use the SHEntry's copy of the post data stream for a new channel (similar, but more roundabout to what happened in the xpcshell test mentioned in comment #11). Since this data stream is closed, we can't read any data from it, which (at least in mochitest) causes the browser to hang.

What the right fix for this is, I don't know (yet), and can't figure out right now. It's getting late for me and my brain is fried. The saga continues tomorrow...

Comment 14

[u408661]

Boris, I'm wondering if you know of an easy way to make session history not need the post stream (nsSHEntry::mPostData) in order to close this bug. If there's no easy way (I'm talking 1-3 lines easy), I'm tempted to WONTFIX this, as this behavior has been around a Really Long Time, and has a simple workaround (restart Firefox).

Comment 15

[Boris Zbarsky [:bzbarsky]]

The session history needs the post data stream so you can go back to post results (possibly reposting).  Closing the stream in necko manually is definitely wrong.

What I think should happen instead is to seek to the end and then try to read one byte, which will trigger the close-at-EOF behavior these streams typically have.... or something.  We'd have to make sure that the multiplex/MIME stream handles that right.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I could have sworn we had an existing bug on this, btw, with lots of discussion about how the state of this stream should be managed (e.g. the fact that the necko _consumer_ is responsible for rewinding the stream right now is all sorts of broken for similar reasons).

Comment 17

[u408661]

Attachment: bug900977.patch

Well, the seek + read tactic (in the new patch) clears up mochitest-2, but mochitest-1 still has some failures in XHR tests. I'm going to drop working on this for now, as I have more pressing things to deal with. Someone else can feel free to try to whip this patch into shape, if they really want.

Comment 18

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 19

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 20

[Vittorio Giovara]

just out of curiosity, what is the hold up for committing the patch and close this bug?

Comment 21

[Boris Zbarsky [:bzbarsky]]

It doesn't pass tests.  See comment 17.