Closed Bug 1264566 Opened 8 years ago Closed 8 years ago

[e10s] UAC error message "Acces Denied" when doing action on file/folder after upload in Firefox with form submit

Categories

(Core :: Networking: File, defect, P1)

Product:
Core
Component:
Networking: File
Version:
47 Branch
Platform:
x86_64
Windows 7
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
e10s + ---
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: afarre, Assigned: valentin)

References

Details

(Whiteboard: [necko-active])

Attachments

(6 files, 2 obsolete files)

Bug 1264566 - Part 1: Refactor FileDescriptor to fix leaks in content processes.
(deleted), text/x-review-board-request
valentin
: review+
baku
: review+
Details
Bug 1264566 - Part 2: Refactor all usage of FileDescriptor.
(deleted), text/x-review-board-request
valentin
: review+
Details
leaked_stack.txt
(deleted), text/plain
Details
0001-Bug-1264566-WIP-dirty-hack.patch
(deleted), patch
Details | Diff | Splinter Review
Bug 1264566 - Part 3: Release upload stream after request complete.
(deleted), text/x-review-board-request
valentin
: review+
Details
Close mUploadStream in the content process in OnStopRequest
(deleted), patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review 
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
Severity: normal → major
OS: Unspecified → Windows 7
Priority: -- → P4
Hardware: Unspecified → x86_64
Reproduced at Firefox Developer Edition: 47.0a2  released at 2016-04-10
I tried with this form (https://www.cs.tut.fi/~jkorpela/forms/file.html) on FF48, I can't reproduce it.

Could you test with a fresh profile, please.
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles
Flags: needinfo?(afarre)
In advance, I'm sorry to reply so late,

And going to work, I've also tried the upload (https://www.cs.tut.fi/~jkorpela/forms/file.html) with a single file and you can modify without problems as you pointed, but then I remembered the whole process I'm doing. 

I'm using Firefox to open WebSphere Administration Console (yes I know... no comments) to deploy a web application generated by Maven. As you probably already know, the usual maven working procedure it's to clean the folder before compile (and we can understand clean as delete target folder), then I've tried the following steps:

1 Create a temporal folder
2 Create a text file inside the previously created temporal folder
3 Upload text file to testing page (https://www.cs.tut.fi/~jkorpela/forms/file.html) -> OK
4 Try to delete temporal folder -> KO Windows ask for administration privileges to delete
5 If you close Firefox windows let you delete it without administration privileges
Flags: needinfo?(afarre)
Ok, it's due to e10s enabled. Surely an issue with process sandboxing. If you disable e10s in the General options, it works normally to delete/move file or folder.
Blocks: e10s
Severity: major → normal
Status: UNCONFIRMED → NEW
Component: HTML: Form Submission → Security: Process Sandboxing
Ever confirmed: true
Priority: P4 → --
Summary: Firefox keep uploaded files locked in Windows after form submit → [e10s] UAC error message "Acces Denied" when doing action on file/folder after upload in Firefox with form submit
tracking-e10s: --- → ?
(In reply to Loic from comment #4)
> Ok, it's due to e10s enabled. Surely an issue with process sandboxing. If
> you disable e10s in the General options, it works normally to delete/move
> file or folder.

This happens with the content sandbox disabled, but as you say, it is e10s only.
I'm guessing the issue is in File related Networking code.
Component: Security: Process Sandboxing → Networking: File
I can't reproduce on Windows 7 and https://www.cs.tut.fi/~jkorpela/forms/file.html
(In reply to Jim Mathies [:jimm] from comment #6)
> I can't reproduce on Windows 7 and
> https://www.cs.tut.fi/~jkorpela/forms/file.html

with security.sandbox.content.level = 0.
Keywords: qawanted
(In reply to Jim Mathies [:jimm] from comment #7)
> (In reply to Jim Mathies [:jimm] from comment #6)
> > I can't reproduce on Windows 7 and
> > https://www.cs.tut.fi/~jkorpela/forms/file.html
> 
> with security.sandbox.content.level = 0.

It's reproducible with the default value, 2.
(In reply to Jim Mathies [:jimm] from comment #7)
> (In reply to Jim Mathies [:jimm] from comment #6)
> > I can't reproduce on Windows 7 and
> > https://www.cs.tut.fi/~jkorpela/forms/file.html
> 
> with security.sandbox.content.level = 0.

I can reproduce with level 0 and with the sandbox disabled.

Jim - were you following the STR in comment 3? (where it's the folder you try and delete)
Flags: needinfo?(jmathies)
It seems the child process is still holding a handle to the file after it's been uploaded.
I'm taking a look at the problem. I assume it's somewhere around RemoteOpenChild or nsFileInputStream
Assignee: nobody → valentin.gosu
Whiteboard: [necko-active]
(In reply to Bob Owen (:bobowen) from comment #9)
> (In reply to Jim Mathies [:jimm] from comment #7)
> > (In reply to Jim Mathies [:jimm] from comment #6)
> > > I can't reproduce on Windows 7 and
> > > https://www.cs.tut.fi/~jkorpela/forms/file.html
> > 
> > with security.sandbox.content.level = 0.
> 
> I can reproduce with level 0 and with the sandbox disabled.
> 
> Jim - were you following the STR in comment 3? (where it's the folder you
> try and delete)

Yes, ok I can reproduce not being able to delete the folder. The file I can delete. This is an annoying bug since you have to close the browser.

bbarnes, would you consider this an M9 or tracking plus P(n)?
Flags: needinfo?(jmathies) → needinfo?(bbermes)
Keywords: qawanted
M9 as in blocking? If so, yes it should block, seems a pretty fundamental issue that would users leave and try it with another browser or stop what they were intending to do with their browser/website.
Flags: needinfo?(bbermes)
I have tracked this down to the fact that we are leaking the file handle in the child process, as it says in the following comment:
https://dxr.mozilla.org/mozilla-central/source/ipc/glue/FileDescriptor.h#154

Ben, do you think we could avoid leaking the handle in this case?
Since FxOS isn't really a concern anymore, do the constraints in bug 827749 still apply?
Thanks!
Flags: needinfo?(bent.mozilla)
Bill, do you have thoughts on this? Based on the try run, it seems we might need to have some kind of ref counting for file handles in the content process.
Flags: needinfo?(wmccloskey)
-> jld
Flags: needinfo?(bent.mozilla) → needinfo?(jld)
mozilla::ipc::FileDescriptor as I understand it: if you construct it yourself, it owns the underlying OS handle and will close it; if you receive it from IPC, it doesn't own the handle and you're required to take it and dispose of it yourself.  This is a footgun and I don't really like it, but there's a nontrivial amount of IPC protocol implementation that uses it now (not just B2G) which would need to be changed.

So the easy answer is that something else is misusing the FileDescriptor interface, and it's Not My Problem.  But… that interface is kind of nightmarish for trying understand what happens with nsFileInputStream::Deserialize and its array of file descriptors and all the possible error cases.

So this might justify finally taking the time to clean this up — apparently we can't make FileDescriptor::PlatformHandle take the object by rvalue reference yet, but replacing the method with TakePlatformHandle (clears mHandle, sets a flag for assertions) or similar should hopefully get the point across.  (Callers which want to “borrow” the platform handle, if any, could probably just hold a PlatformHandleType or an NSPR descriptor or something and close it in a destructor, but I haven't audited everything to see how well that would work; there might need to be a BorrowPlatformHandle where the caller is responsible for avoiding use-after-close.)
Flags: needinfo?(jld)
bumping back into e10s triage
Flags: needinfo?(wmccloskey)
valentin, is this something you can get too soonish? we are blocking e10s on it.
tracking-e10s: ? → m9+
Flags: needinfo?(valentin.gosu)
tracking-e10s: m9+ → +
Priority: -- → P1
I have tried tried to fix this individual issue, without completely changing how FileDescriptor works.
While it fixes the handle "leak", it causes crashes in other other tests.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d51c883a1e04

If possible, I would assign this bug to someone who knows the IPC code a bit better.
Flags: needinfo?(valentin.gosu)
Kanru, is there anyone on your team who has some time for this? It's not a blocker, so lower in priority than crash bugs.
Flags: needinfo?(kchen)
WeiCheng, do you want to take a look? Assume Valentin's patch in comment 20 works, we only need to fix the crashes in other tests. We can look at the IPC bits together.
Flags: needinfo?(kchen) → needinfo?(wpan)
I'm looking this.

Somehow FileDescriptor will be created in content process multiple times, with the same dup-ed handle/descriptor.

So I can't remote the condition here
https://dxr.mozilla.org/mozilla-central/source/ipc/glue/FileDescriptor.cpp#77
or it will crash because of double free same handle.
Flags: needinfo?(wpan)
Please just ignore my previous comment, I didn't read comment 17 thoroughly.

I think the mozilla::ipc::FileDescriptor's copy methods are problematic in content processes.
They shouldn't close the PlatformHandle and then just take a shallow copy.

To fix this, I duplicate the PlatformHandle when it's copying, so that
every mozilla::ipc::FileDescriptor can safely close their own PlatformHandle.

I've tested this on Linux and Windows, at least in this case, no process
holds the file lock right after upload complete.

There is an ongoing job on try server, I hope this won't cause regression.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5c6aedf3dd1
Assignee: valentin.gosu → wpan
It busted on all branches, apparently it needs more work.
Now FileDescriptor takes the ownership of the platform handle, in both parent
and content processes.
Copying will duplicate the underlying handle.
It also comes with move constructor to avoid duplicating if possible.

The getter will duplicate a new handle, so it is recommended that use
ScopedPlatformHandle to hold the duplicated handle.

Review commit: https://reviewboard.mozilla.org/r/55758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55758/
Attachment #8757205 - Flags: review?(jduell.mcbugs)
Attachment #8757206 - Flags: review?(jduell.mcbugs)
Callers should use ScopedPlatformHandle or put it into a RAII class
immediately.

Review commit: https://reviewboard.mozilla.org/r/55760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55760/
These patches should fix leaks in content processes, but to actually release the file handle, it's needed to close the tab, and wait few seconds for CC.

I *think* this is because we didn't release the nsMIMEInputStream after the form submitted.
However I know nothing about this, :smaug, can we schedule a Runnable to release nsMIMEInputStream at here?
https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#13718
Flags: needinfo?(bugs)
Attached file leaked_stack.txt (deleted) — 
BTW, this is the stack during CC on Windows.
Why is the channel which nsIDocument keeps alive keeping nsMIMEInputStream alive?
The lines here
xul.dll!nsMIMEInputStream::Release() Line 70   C++
xul.dll!mozilla::net::HttpBaseChannel::~HttpBaseChannel() Line 138     C++
don't seem to be quite right, so it isn't quite clear to me what thing in HttpBaseChannel has the pointer to nsMIMEInputStream.

In other words, why would we need to explicitly schedule runnable to release nsMIMEInputStream? Why doesn't that happen automatically inside necko?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #30)
> Why is the channel which nsIDocument keeps alive keeping nsMIMEInputStream
> alive?
> The lines here
> xul.dll!nsMIMEInputStream::Release() Line 70   C++
> xul.dll!mozilla::net::HttpBaseChannel::~HttpBaseChannel() Line 138     C++
> don't seem to be quite right, so it isn't quite clear to me what thing in
> HttpBaseChannel has the pointer to nsMIMEInputStream.
> 
> In other words, why would we need to explicitly schedule runnable to release
> nsMIMEInputStream? Why doesn't that happen automatically inside necko?

I suspect that's a bug. We should probably null out mUploadStream in nsHttpChannel::OnStopRequest() and HttpChannelChild::OnStopRequest()
After the form completed, there are two objects hold the nsMIMEStream, one is HttpChannelChild::mUploadStream, another one is nsSHEntry::mPostData.
So just null out mUploadStream in OnStopRequest() is not enough.

:smaug, is there any chance we can release it from history after POST complete?
Flags: needinfo?(bugs)
Hmm, session history is supposed to keep the postdata alive. That is the prompt about
"reposting" when reloading a page after a form submission.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #33)
> Hmm, session history is supposed to keep the postdata alive. That is the
> prompt about
> "reposting" when reloading a page after a form submission.

Thank you.

I thought this is the problem, but it's not (the root cause).
Those objects also hold nsMIMEInputStream until nsDocShell gone in non-e10s, but the file does not leak.

Because the file is closed by CLOSE_ON_EOF and reopened by REOPEN_ON_REWIND in this case, but there is no way to do this in content processes.
The flags are removed after IPC, even if they are not, the file reading is still happens in parent process, not content processes.

This patch is a hack to nsSHEntry, because it's useless to keep a stream alive in content processes if nobody is reading it.

:valentin, I'm confused, why do we still read it in parent process instead of content processes, even in e10s?
If this needs another refactor, can we just hack it for now?
Thanks.
Flags: needinfo?(valentin.gosu)
:jduell, do you have time to review these patches?
If not, which peer should I choose instead?

Thank you.
Flags: needinfo?(jduell.mcbugs)
(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (away 6/8-6/12) from comment #34)
> Created attachment 8759524 [details] [diff] [review]
> 0001-Bug-1264566-Trying-to-fix-leaks-in-content-processes.patch
> 
> (In reply to Olli Pettay [:smaug] from comment #33)
> > Hmm, session history is supposed to keep the postdata alive. That is the
> > prompt about
> > "reposting" when reloading a page after a form submission.
> 
> Thank you.
> 
> I thought this is the problem, but it's not (the root cause).
> Those objects also hold nsMIMEInputStream until nsDocShell gone in non-e10s,
> but the file does not leak.
> 
> Because the file is closed by CLOSE_ON_EOF and reopened by REOPEN_ON_REWIND
> in this case, but there is no way to do this in content processes.
> The flags are removed after IPC, even if they are not, the file reading is
> still happens in parent process, not content processes.

I thought that the file reading would happen in the content process.
So do we still have a problem because we don't close the file?

> 
> This patch is a hack to nsSHEntry, because it's useless to keep a stream
> alive in content processes if nobody is reading it.
> 
> :valentin, I'm confused, why do we still read it in parent process instead
> of content processes, even in e10s?
> If this needs another refactor, can we just hack it for now?
> Thanks.

So does this fix the handle leak? And does it still POST when you refresh the page?
If so, it doesn't look too hacky (I think there is code that does things that are much worse).
Also, add some comments to nsSHEntry so we don't forget the reasoning behind this change.
(In reply to Valentin Gosu [:valentin] from comment #36)
> I thought that the file reading would happen in the content process.
> So do we still have a problem because we don't close the file?

Yes, the current flow in e10s is:

1. content request parent to open the file
2. parent opens and keeps the file handle, sends it to content
3. content receives and keeps the handle
4. parent reads the file until the end, closes the file
5. the handle leaked in content

> So does this fix the handle leak? And does it still POST when you refresh
> the page?
> If so, it doesn't look too hacky (I think there is code that does things
> that are much worse).
> Also, add some comments to nsSHEntry so we don't forget the reasoning behind
> this change.

Ah, it fixed the leak but can not re-post by reload.
I've tried to close it in content process but did not go well.
I guess the "reopening" is conflict with content process.
For what it's worth, there's already code for letting a content process hold a by-pathname reference to a file rather than an open descriptor/handle: the DOM File/Blob implementation.
Flags: needinfo?(jduell.mcbugs)
Attachment #8757205 - Flags: review?(jduell.mcbugs) → review?(valentin.gosu)
Attachment #8757206 - Flags: review?(jduell.mcbugs) → review?(valentin.gosu)
In nsURILoader::OpenURI(), we are still using parent process to send the request:
https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsURILoader.cpp#821

:valentin, should we change this for e10s? (I guess this will be another bug though)
Another problem is we will need a reopenable file stream in IPC.

This patch will fix it in this case, also works for reposting, but may break in other cases though. The try push is still running:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3d16ff956e2
Attachment #8759524 - Attachment is obsolete: true
Comment on attachment 8757206 [details]
Bug 1264566 - Part 2: Refactor all usage of FileDescriptor.

https://reviewboard.mozilla.org/r/55760/#review54722

These all look fine.

::: dom/camera/GonkCameraControl.h:23
(Diff revision 1)
>  #define DOM_CAMERA_GONKCAMERACONTROL_H
>  
>  #include "base/basictypes.h"
>  #include "nsRefPtrHashtable.h"
>  #include "mozilla/ReentrantMonitor.h"
> +#include "mozilla/ipc/FileDescriptorUtils.h"

Is this needed in the header? Or would it be better to add it to the .cpp file?
Attachment #8757206 - Flags: review?(valentin.gosu) → review+
Attachment #8757205 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8757205 [details]
Bug 1264566 - Part 1: Refactor FileDescriptor to fix leaks in content processes.

https://reviewboard.mozilla.org/r/55758/#review54726

Looks good.
Comment on attachment 8757205 [details]
Bug 1264566 - Part 1: Refactor FileDescriptor to fix leaks in content processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55758/diff/1-2/
Attachment #8757205 - Attachment description: MozReview Request: Bug 1264566 - Part 1: Refactor FileDescriptor to fix leaks in content processes. r?jduell → Bug 1264566 - Part 1: Refactor FileDescriptor to fix leaks in content processes.
Attachment #8757206 - Attachment description: MozReview Request: Bug 1264566 - Part 2: Refactor all usage of FileDescriptor. r?jduell → Bug 1264566 - Part 2: Refactor all usage of FileDescriptor.
Comment on attachment 8757206 [details]
Bug 1264566 - Part 2: Refactor all usage of FileDescriptor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55760/diff/1-2/
https://reviewboard.mozilla.org/r/55760/#review54722

> Is this needed in the header? Or would it be better to add it to the .cpp file?

You're right, I forgot to remove this line during rebasing.
Part 1: Only changed commit message.
Part 2: Removed the inclusion in dom/camera/GonkCameraControl.h

try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c234af572ab0
https://reviewboard.mozilla.org/r/55758/#review54950

::: ipc/glue/FileDescriptor.h:55
(Diff revision 1)
> +  {
> +    typedef PlatformHandleType type;
> +    static type empty();
> +    static void release(type aFd);
> +  };
> +  typedef Scoped<PlatformHandleTrait> ScopedPlatformHandle;

I notice that `Scoped` is marked as deprecated in favor of `UniquePtr`.  See also bug 1255857, which allows `UniquePtr` to use an arbitrary type for the underlying “pointer”.
Whiteboard: [necko-active] → [necko-active], e10st?
(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #46)
> https://reviewboard.mozilla.org/r/55758/#review54950
> I notice that `Scoped` is marked as deprecated in favor of `UniquePtr`.  See
> also bug 1255857, which allows `UniquePtr` to use an arbitrary type for the
> underlying “pointer”.

Thanks for the reminder. I was attempting to use `UniquePtr`, but was struggling on making `PlatformHandle` nullable.
Now I've solved it and found a way to close the stream, just waiting the try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac9d55e8ecd5
Comment on attachment 8757205 [details]
Bug 1264566 - Part 1: Refactor FileDescriptor to fix leaks in content processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55758/diff/2-3/
Comment on attachment 8757206 [details]
Bug 1264566 - Part 2: Refactor all usage of FileDescriptor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55760/diff/2-3/
Comment on attachment 8765333 [details]
Bug 1264566 - Part 3: Release upload stream after request complete.

https://reviewboard.mozilla.org/r/60772/#review57668

r=me
Have you checked that all the tests still pass?
Attachment #8765333 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8765334 [details]
Bug 1264566 - Part 4: Close unused stream in IPC.

https://reviewboard.mozilla.org/r/60774/#review57670
Attachment #8765334 - Flags: review?(valentin.gosu) → review+
(In reply to Valentin Gosu [:valentin] from comment #52)
> Have you checked that all the tests still pass?

I think so:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac9d55e8ecd5

Also tested on Windows, the directory is delete-able after these patches.

Anyway, I made another push again that based on the most recent m-c.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d8260f17a75
There's a lot of orange in these try runs.
I fear that this is still breaking something. Could you do a try run without part 4?
Flags: needinfo?(valentin.gosu)
Comment on attachment 8757205 [details]
Bug 1264566 - Part 1: Refactor FileDescriptor to fix leaks in content processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55758/diff/3-4/
Comment on attachment 8757206 [details]
Bug 1264566 - Part 2: Refactor all usage of FileDescriptor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55760/diff/3-4/
Comment on attachment 8765333 [details]
Bug 1264566 - Part 3: Release upload stream after request complete.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60772/diff/1-2/
Comment on attachment 8765334 [details]
Bug 1264566 - Part 4: Close unused stream in IPC.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60774/diff/1-2/
Comment on attachment 8765334 [details]
Bug 1264566 - Part 4: Close unused stream in IPC.

https://reviewboard.mozilla.org/r/60774/#review59130
Attachment #8765334 - Flags: review+ → review?(valentin.gosu)
Attachment #8765334 - Flags: review?(valentin.gosu) → review?(amarchesini)
https://reviewboard.mozilla.org/r/55758/#review59422

::: ipc/glue/FileDescriptor.h:110
(Diff revision 4)
> -  }
>  
> -  PlatformHandleType
> -  PlatformHandle() const
> -  {
> -#ifdef DEBUG
> +  // Returns a duplicated handle, it is caller's responsibility to close the
> +  // handle.
> +  UniquePlatformHandle
> +  DuplicatePlatformHandle() const;

ClonePlatformHandle() seems a better name.

::: ipc/glue/FileDescriptor.h:129
(Diff revision 4)
>  
>    static bool
>    IsValid(PlatformHandleType aHandle);
>  
> -  void
> -  DuplicateInCurrentProcess(PlatformHandleType aHandle);
> +  static PlatformHandleType
> +  Duplicate(PlatformHandleType aHandle);

Clone ?
Comment on attachment 8765334 [details]
Bug 1264566 - Part 4: Close unused stream in IPC.

https://reviewboard.mozilla.org/r/60774/#review59424

I don't understand why this is needed. If the stream is not needed, we should get it, in the first place.
If there is something wrong, the bug is in how HTMLFormSubmission (or any underlying layer) uses the InputStream.
Attachment #8765334 - Flags: review?(amarchesini) → review-
(In reply to Andrea Marchesini (:baku) from comment #64)
> Comment on attachment 8765334 [details]
> Bug 1264566 - Part 4: Close unused stream in IPC.
> 
> https://reviewboard.mozilla.org/r/60774/#review59424
> 
> I don't understand why this is needed. If the stream is not needed, we
> should get it, in the first place.
> If there is something wrong, the bug is in how HTMLFormSubmission (or any
> underlying layer) uses the InputStream.

We still need the RemoteInputStream in content-side, so it can tell the parent process which blob to read.
(https://dxr.mozilla.org/mozilla-central/source/ipc/glue/InputStreamUtils.cpp#110)
And only in this case we don't need RemoteInputStream keep open.

I think we will have to do similar thing anyway. (in Blob or InputStream)
> I think we will have to do similar thing anyway. (in Blob or InputStream)

Can you ping me on IRC? I would like to understand this issue a bit better. Thanks!
Flags: needinfo?(wpan)
Some notes for future me:

When user clicks upload, child process request parent to show a file picker dialog.
Parent keeps the file id in BlobParent::IDTableEntry, then send back the file entry to child, as BlobChild::RemoteBlobImpl. [1]

Then user clicks submit, child will call mozilla::dom::FSMultipartFormData::AddNameBlobOrNullPair to gather all file inputs.

The function will call mozilla::dom::Blob::GetInternalStream, which will cause RemoteBlobImpl to send an IPC request, that will request parent to open the file, and send the FD back. [2]

Then child will call nsURILoader::OpenURI, sends nsMultiplexInputStream back to parent, let parent do the socket work. [3]

When parent deserialize the RemoteInputStream, it just use the cached File in BlobParent::IDTableEntry (please look [1]), so the FD in child does not matter in IPC.

Because child does not touch the RemoteInputStream (see [3]), the FD which obtained in [2] is useless.
And since we do not explicitly call Close() for the post data, those FD are leaked.


After a short discussion with :baku, we have agreed that the code should not involve changing in DOM, it should keep in network level.
Flags: needinfo?(wpan)
Comment on attachment 8757205 [details]
Bug 1264566 - Part 1: Refactor FileDescriptor to fix leaks in content processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55758/diff/4-5/
Attachment #8757205 - Flags: review?(amarchesini)
Attachment #8765334 - Flags: review?(valentin.gosu)
Attachment #8765334 - Flags: review?(amarchesini)
Attachment #8765334 - Flags: review-
Comment on attachment 8757206 [details]
Bug 1264566 - Part 2: Refactor all usage of FileDescriptor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55760/diff/4-5/
Comment on attachment 8765333 [details]
Bug 1264566 - Part 3: Release upload stream after request complete.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60772/diff/2-3/
Comment on attachment 8765334 [details]
Bug 1264566 - Part 4: Close unused stream in IPC.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60774/diff/2-3/
Attachment #8765334 - Flags: review?(valentin.gosu)
Comment on attachment 8765334 [details]
Bug 1264566 - Part 4: Close unused stream in IPC.

Sorry for the delay. I spent a couple of hours today debugging this issue.
We have a serious problem in the shutting down of RemoteInputStream, but I still think this should be handled differently.

Here a question for valentin:

We don't use the inputStreams in HTTPChannelChild (mUploadStream) but we serialize and send them to the parent process. Should we call Close() ? At least when OnStopRequest runs?

It's OK to have a non-used RemoteInputStream opened. But it must be closed "together" with the nsIInputStreams in the parent process.
Flags: needinfo?(valentin.gosu)
Attachment #8765334 - Flags: review?(amarchesini)
If part 2 is OK, can we land part 1~3 first?
At least the streams are CC-able after tab closed.
Flags: needinfo?(amarchesini)
Comment on attachment 8757205 [details]
Bug 1264566 - Part 1: Refactor FileDescriptor to fix leaks in content processes.

https://reviewboard.mozilla.org/r/55758/#review63138
Attachment #8757205 - Flags: review?(amarchesini) → review+
Yes please.
and maybe file a separate bug for this 'close-stream' issue and move patch 4 there.
Thanks! And sorry for the super delay in the review.
Flags: needinfo?(amarchesini)
Dear Sheriffs:
Please land part 1~3, thanks.

I'd prefer to leave this bug open because the OP's problem has not completely solved yet.
Keywords: checkin-needed, leave-open
(In reply to Andrea Marchesini (:baku) from comment #76)
> Yes please.
> and maybe file a separate bug for this 'close-stream' issue and move patch 4
> there.
> Thanks! And sorry for the super delay in the review.

No problem at all!
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95d12e109117
Part 1: Refactor FileDescriptor to fix leaks in content processes. r=valentin, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/08d0e33b4cb4
Part 2: Refactor all usage of FileDescriptor. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/0476118cfea7
Part 3: Release upload stream after request complete. r=valentin
Keywords: checkin-needed
Anyone already on VS2015 Update 3? After this bug was checked in today I get C2833 compile errors but not always in the same file. It seems that after a restart it compiles and goes on until the next error. I already clobbered my build directory. This is a comm-central suite compile but I doubt it matters here. Windows 7 in a virtualbox VM. 

+++ snipp +++
c:/seabuild/release/comm-central-15/obj-x86_64-pc-mingw32/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  cl.EXE -FonsHttpAuthCache.obj -c -Ic:/seabuild/release/comm-central-15/obj-x86_64-pc-mingw32/dist/stl_wrappers  -DNDEBUG=1 -DTRIMMED=1 -DWIN32_LEAN_AND_
MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DCOMPILER_MSVC -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Ic:/seamonke
y/comm-central/mozilla/netwerk/protocol/http -Ic:/seabuild/release/comm-central-15/obj-x86_64-pc-mingw32/netwerk/protocol/http -Ic:/seabuild/release/comm-central-15/obj-x86_64-pc-mingw32/ipc/ipdl/_ipdlheaders -Ic:/seamonkey/comm-central/mozilla/ipc/chromium/src -I
c:/seamonkey/comm-central/mozilla/ipc/glue -Ic:/seamonkey/comm-central/mozilla/dom/base -Ic:/seamonkey/comm-central/mozilla/netwerk/base -Ic:/seabuild/release/comm-central-15/obj-x86_64-pc-mingw32/dist/include  -Ic:/seabuild/release/comm-central-15/obj-x86_64-pc-m
ingw32/dist/include/nspr -Ic:/seabuild/release/comm-central-15/obj-x86_64-pc-mingw32/dist/include/nss        -MD -FI c:/seabuild/release/comm-central-15/obj-x86_64-pc-mingw32/mozilla-config.h -DMOZILLA_CLIENT   -TP -nologo -wd5026 -wd5027 -Zc:sizedDealloc- -Zc:thr
eadSafeInit- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -FS -wd4251 -wd4244 -wd4267 -wd4345 -wd4351 -wd4800 -wd4819 -wd4595 -we4553 -GR-  -O2 -Oy   -Fdgenerated.pdb  c:/seamonkey/comm-central/mozilla/netwerk/protocol/http/nsHttpAuthCache.cpp
nsHttpAuthCache.cpp

c:/seamonkey/comm-central/mozilla/ipc/glue/FileDescriptor.cpp(210): error C2833: 'operator PlatformHandleType' is not a recognized operator or type
Comfirmed that this breaks build on VS2015u3. Replacing PlatformHandleType with FileDescriptor::PlatformHandleType fixes that error.
Thanks Xidorn. Compile is running.
Depends on: 1289316
(In reply to Andrea Marchesini (:baku) from comment #73)
> Comment on attachment 8765334 [details]
> Bug 1264566 - Part 4: Close unused stream in IPC.
> 
> Sorry for the delay. I spent a couple of hours today debugging this issue.
> We have a serious problem in the shutting down of RemoteInputStream, but I
> still think this should be handled differently.
> 
> Here a question for valentin:
> 
> We don't use the inputStreams in HTTPChannelChild (mUploadStream) but we
> serialize and send them to the parent process. Should we call Close() ? At
> least when OnStopRequest runs?
> 
> It's OK to have a non-used RemoteInputStream opened. But it must be closed
> "together" with the nsIInputStreams in the parent process.

I consulted with :jduell, and we should probably close mUploadStream.
For the child we can probably do that after right after serializing it.
Flags: needinfo?(valentin.gosu)
Assignee: wpan → nobody
valentin, can the necko team pick up the rest of this work?
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-active], e10st? → [necko-active]
Closing the upload stream in OnStopRequest seems to release the handle and get rid of the issue described in comment 0.
Attachment #8779905 - Flags: review?(jduell.mcbugs)
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Attachment #8779905 - Flags: review?(jduell.mcbugs) → review+
Flags: needinfo?(valentin.gosu)
Keywords: leave-open → checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c4097d6c1a
Close mUploadStream in the content process in OnStopRequest. r=jduell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f6c4097d6c1a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1293765
Depends on: 1297663
This was backed out in order to fix bug 1297663.
https://hg.mozilla.org/mozilla-central/rev/536486980375
Status: RESOLVED → REOPENED
status-firefox51: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---
Depends on: 1293628
This bug is a mess!  The patches except the Attachment #8779905 [details] [diff] has already landed on 50 (now beta).  Why wasn't this bug marked fixed?  It had to be marked fixed/resolved in comment 50, apparently.

Bug 1293628 is dupl of bug 1293765 which just needs an uplift to beta.  I didn't request uplift approvals on that bug because this bug didn't have properly set status flags at all and I didn't know which versions have been affected.

I'm not sure what is the issue in comment 87 being discussed, but it definitely needs a new followup bug.  It's a very bad practice to span a single bug across multiple releases.  There the mess starts.

Closing this bug now, Valentin, if there is any more work needed to happen, please open a new bug(s).
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox49: --- → affected
status-firefox50: --- → fixed
status-firefox51: --- → fixed
status-firefox52: --- → fixed
No longer depends on: 1293628
Flags: needinfo?(valentin.gosu)
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1304056
Too late for a fix in 49. Honza, thanks for setting the flags.
status-firefox49: affected → wontfix
Flags: needinfo?(valentin.gosu)
Attachment #8765334 - Attachment is obsolete: true
Attachment #8779905 - Attachment is obsolete: true
Depends on: 1329911
Attachment #8779905 - Attachment is obsolete: false
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: