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

NEW
Unassigned

Status

()

P3
normal
5 years ago
8 months ago

People

(Reporter: vitto.giova, Unassigned)

Tracking

22 Branch
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-backlog])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
The form was a bugreport at Sourceforge.net

Comment 2

5 years ago
Sometimes, it may happen. Are you able to reproduce the issue if you reset your web connection during uploading?
Flags: needinfo?(vitto.giova)
(Reporter)

Comment 3

5 years ago
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.
Flags: needinfo?(vitto.giova)

Comment 4

5 years ago
Did you try with other browsers? Same behavior?

Updated

5 years ago
Summary: Connection reset when uploading a file doesn't free the handle → Connection reset when uploading a file doesn't free the handle
Component: Untriaged → HTML: Form Submission
Product: Firefox → Core
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.  :(
Component: HTML: Form Submission → Networking: HTTP
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
I can most likely find the cycles for this.
Assignee: nobody → hurley
Created attachment 790277 [details] [diff] [review]
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
Attachment #790277 - Flags: review?(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!
Attachment #790277 - Flags: review?(mcmanus) → review+
*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.
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.
(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.
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...
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).
Flags: needinfo?(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.
Flags: needinfo?(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).
Created attachment 796665 [details] [diff] [review]
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.
Attachment #790277 - Attachment is obsolete: true
Assignee: hurley → nobody
Whiteboard: [necko-backlog]
(Reporter)

Comment 20

8 months ago
just out of curiosity, what is the hold up for committing the patch and close this bug?
It doesn't pass tests.  See comment 17.
You need to log in before you can comment on or make changes to this bug.