Valentin Gosu [:valentin] (he/him) Reporter
	Description • 8 years ago

+++ This bug was initially created as a clone of Bug #1264566 +++

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160410004056

Steps to reproduce:

1.- Select file to upload
2.- Submit form
3.- Try to delete file
4.- Operative system tells that file is locked


Actual results:

Operative system doesn't allow to delete or update file


Expected results:

Operative system allow to delete or update file

	Valentin Gosu [:valentin] (he/him) Reporter
	Comment 1 • 8 years ago

Honza, do you think you can take this? I don't think I know enough about the lifetime of the upload stream to make this work.

	Honza Bambas (:mayhemer)
	Comment 2 • 8 years ago

OK, I will take it.  What is the current status of the patches?  I'm thinking if we should back everything out or try to go from where we are now.

	Valentin Gosu [:valentin] (he/him) Reporter
	Comment 3 • 8 years ago

From what I understand, the patches landed in Bug 1264566 don't actually change the behaviour, they just improve the serialization of file descriptors.
The problem that remains is finding the appropriate moment to release the upload stream, so we don't "leak" a file handle in the child.

	Marcia Knous [:marcia]
	Comment 5 • 7 years ago

[Tracking Requested - why for this release]: Carrying over noms from the duplicate bug.

	Gerry Chang [:gchang]
	Comment 6 • 7 years ago

Track 51- and mark 51 won't fix as this is late beta and no actions for now.

	The 8472
	Comment 7 • 7 years ago

Maybe the files should be opened with FILE_SHARE_DELETE? While that won't solve the leak it'll make the timing of closing the FD less critical.

	Julien Cristau [:jcristau]
	Comment 9 • 7 years ago

Honza. is there any hope of a fix in time for 52 (i.e. probably within a week or so)?

	Honza Bambas (:mayhemer)
	Comment 10 • 7 years ago

Jason, could you please find an owner for this?  I won't probably have time to work on this bug in the needed time frame.  Thanks.

	Julien Cristau [:jcristau]
	Comment 12 • 7 years ago

It's getting late for 52, and bug 1329911 (which had the initial tracking request) was addressed.

	Honza Bambas (:mayhemer)
	Comment 14 • 7 years ago

This seems to be a problem for too many users.  I'll put it back on my list.  Dropping ni for jason (after 30+ days :))

	Honza Bambas (:mayhemer)
	Comment 15 • 7 years ago

So, to sum what we have changed (in the http code) in the past and how it evolved.

* First, bug 1264566 has been reported, added two changes.

  2016-07-25: https://hg.mozilla.org/mozilla-central/rev/0476118cfea7
    Just drops (nullifies) the upload stream in both 
    HttpChannelChild::OnStopRequest and nsHttpChannel::OnStopRequest

  2016-08-17: https://hg.mozilla.org/mozilla-central/rev/f6c4097d6c1a
    Adds Close() call on the upload stream in HttpChannelChild::OnStopRequest
    Rational for this change seems to be https://bugzilla.mozilla.org/show_bug.cgi?id=1264566#c87, 
    for which I need some clarification first

* It then regressed, bug 1293765 reported.

  2016-08-20: https://hg.mozilla.org/mozilla-central/rev/8cc4907776c8
    Upload stream nullified in nsHttpChannel::OnStopRequest only when 
    there is no authentication in progress (NTLM loops multiple times
    so that we can loose the stream for next auth round)

* Another regression found, bug 1297663.

  2016-09-01: https://hg.mozilla.org/mozilla-central/rev/536486980375
    Backs out Close() call from HttpChannelChild::OnStopRequest 
    (the change from 2016-08-17)
    The cause of this bug was never analyzed.

* And a final regression, bug 1329911.

  2017-01-17: https://hg.mozilla.org/mozilla-central/rev/792c6f70dde5
    Backs out any manipulation with upload streams from both Child 
    and parent http channels, effectively backing out bug 1264566
    Cause has never been deeper understood either.

	Honza Bambas (:mayhemer)
	Comment 16 • 7 years ago

(In reply to Honza Bambas (:mayhemer) from comment #15)
>   2016-08-17: https://hg.mozilla.org/mozilla-central/rev/f6c4097d6c1a
>     Adds Close() call on the upload stream in HttpChannelChild::OnStopRequest
>     Rational for this change seems to be
> https://bugzilla.mozilla.org/show_bug.cgi?id=1264566#c87, 

It's actually https://bugzilla.mozilla.org/show_bug.cgi?id=1264566#c91

	Honza Bambas (:mayhemer)
	Comment 17 • 7 years ago

(In reply to Honza Bambas (:mayhemer) from comment #15)
> * Another regression found, bug 1297663.
> 
>   2016-09-01: https://hg.mozilla.org/mozilla-central/rev/536486980375
>     Backs out Close() call from HttpChannelChild::OnStopRequest 
>     (the change from 2016-08-17)
>     The cause of this bug was never analyzed.
> 

Regressions: an already submitted form cannot be resend with F5 - nothing happens.
Reason: postDataSeekable->Seek(nsISeekableStream::NS_SEEK_SET, 0); fails at https://dxr.mozilla.org/mozilla-central/rev/34585620e529614c79ecc007705646de748e592d/docshell/base/nsDocShell.cpp#11203 with 0x80470002 (BASE_STREAM_CLOSED).
Hence, we can't simply close the upload stream directly on the child this way.

	Honza Bambas (:mayhemer)
	Comment 18 • 7 years ago

- this allows defered open and close after eof on remote file stream (on child)
- there is still a handle open and kept by the child, it's probably created (cloned) as part of ipc messaging via FileDescriptor class from the parent handle (already closed)

Not sure at the moment if this is intentional or is just an unwanted leak.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 19 • 7 years ago

I notice there are several duplicates which makes me think it is a common problem for users. It looks complicated though.  Honza, Patrick, any hope of fixing this for 53? 
I could also ask QE to check if it is still an issue in 54/55.

	Honza Bambas (:mayhemer)
	Comment 20 • 7 years ago

I won't have time to work on this in couple of weeks.  Dropping it now.  I might get to this to fix for 55, but please if anyone can take this instead, do so.

The idea is to not open the file on the child process at all (=to not carry the cloned handle from the parent process) but only use a mockup input stream on child that would at most know the size of the file to make Available() work.

	Honza Bambas (:mayhemer)
	Comment 21 • 7 years ago

Comment on attachment 8846063 [details] [diff] [review]
wip1

This patch is completely wrong, realized few minutes after I've submitted it.

	Honza Bambas (:mayhemer)
	Comment 22 • 7 years ago

I've spent some more time on this.  If we close the file on the child process as soon as the file input stream is deserialized, upload still works well and the file is not held open anymore.  Let's say this is part 1 of the fix here.

And now part 2 - since part 1 breaks FileReader content API [1][2].  The remote (file) stream on the child process, when read using this API, is copied to a pipe on a stream transport thread, and there is a capability to open the file stream remotely (RemoteInputStream::ReallyBlockAndWaitForStream, RemoteInputStream::SetStream).  Hence, it seems there is a ground to implement "open on demand" and "close on EOF" in RemoteInputStream that would solve this bug.

Ben, do you have an idea how complicated that would be to do?


[1] https://w3c.github.io/FileAPI/#readAsDataText
[2] https://developer.mozilla.org/en-US/docs/Using_files_from_web_applications#Handling_the_upload_process_for_a_file

	Honza Bambas (:mayhemer)
	Comment 23 • 7 years ago

(In reply to (offline 29.3. - 3.4.) Honza Bambas (:mayhemer) from comment #22)
> And now part 2 - since part 1 breaks FileReader content API [1][2].  The
> remote (file) stream on the child process, when read using this API, is
> copied to a pipe on a stream transport thread, and there is a capability to
> open the file stream remotely
> (RemoteInputStream::ReallyBlockAndWaitForStream,
> RemoteInputStream::SetStream).  Hence, it seems there is a ground to
> implement "open on demand" and "close on EOF" in RemoteInputStream that
> would solve this bug.

To make this comment more complete...  The problem is that RemoteInputStream is still assigned mStream, but that mStream has mFD == nullptr.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 25 • 7 years ago

Thanks for handing this off. 
Patrick, back to you (or maybe to Ben) to find a new owner. Maybe it is not realistic to fix this for 53.

	Patrick McManus [:mcmanus]
	Comment 26 • 7 years ago

I'll ping jason.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 27 • 7 years ago

(In reply to (offline 29.3. - 3.4.) Honza Bambas (:mayhemer) from comment #22)
> Ben, do you have an idea how complicated that would be to do?

Hi Honza. I've been checked out a little to long to answer you competently here, let's ask baku?

	Jason Duell
	Comment 28 • 7 years ago

We should move on this.

Also asking bkelly in case he has ideas on the architecture here.  Ben, the most obvious fix here is to close the fd in the child after we upload, but that's breaking some DOM APIs--see comment 22.

	Ben Kelly [:bkelly, not reviewing]
	Comment 29 • 7 years ago

Going to defer to :baku here as he knows this code better than I do.

	Honza Bambas (:mayhemer)
	Comment 31 • 7 years ago

:baku, any news here?  This is a serious regressions that needs to be taken care of soon.  Thanks.

	Andrea Marchesini [:baku] Assignee
	Comment 32 • 7 years ago

If FileReader is the only broken API, this is not a big deal, I can fix it. I'm saying that because FileReader works with nsIASyncInputStream, so I can easily 'reopen' the file on the parent side and take a FD back to the content process.

Can you give 1 day to do an experiment? I'll be back tomorrow with some results.
I don't cancel the NI.

	Honza Bambas (:mayhemer)
	Comment 33 • 7 years ago

(In reply to Andrea Marchesini [:baku] from comment #32)
> Can you give 1 day to do an experiment? 

Sure, go ahead!  Thanks.

	Honza Bambas (:mayhemer)
	Comment 34 • 7 years ago

So, how did the experiment go?  This is a really bad bug.  The more releases it misses the worse :(

	Andrea Marchesini [:baku] Assignee
	Comment 35 • 7 years ago

I take this bug. This issue is part of my PBlob refactoring. Basically my approach is the 'extreme' approach of your WIP: I don't serialize any nsIInputStream from parent to child when used by Blobs. Blobs, in content process, have only a nsIAsyncInputStream, so that, only if needed, we move the 'real' inputStream from parent to child process.
This approach fixes the issue with FileReader and BlobURL. The patches are written and in review.

	Honza Bambas (:mayhemer)
	Comment 37 • 7 years ago

(In reply to Andrea Marchesini [:baku] from comment #36)
> 
> *** This bug has been marked as a duplicate of bug 1353629 ***

Just to make sure you are aware of the main change that fixes the upload problem (but breaks blob) which is in the networking code:

https://bugzilla.mozilla.org/attachment.cgi?id=8852070&action=diff#a/netwerk/base/nsFileStreams.cpp_sec5

	Andrea Marchesini [:baku] Assignee
	Comment 38 • 7 years ago

The new setup doesn't require this change because the nsFileInputStream is not sent to the child process as it used to be.
More informations are available here: https://dxr.mozilla.org/mozilla-central/source/dom/file/ipc/IPCBlobUtils.h#12

	Andrew Overholt [:overholt]
	Comment 39 • 7 years ago

baku, did all the relevant PBlob refactoring work land in 55? We don't want to backport that to 54, do we?

	Andrea Marchesini [:baku] Assignee
	Comment 40 • 7 years ago

It seems stable enough, but 54 is released in 1 week, probably it's better to wait for 55.

	Gerry Chang [:gchang]
	Comment 41 • 7 years ago

Too late for 54 as 54 RC is released. Mark 54 won't fix.