910384 - [e10s] File picker doesn't work

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[Tom S [:evilpie]]

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this._cps._dbConnection is null" {file: "resource://gre/modules/ContentPrefService2.jsm" line: 619}]' when calling method: [nsIContentPrefService2::getByDomainAndName]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]

When clicking "Upload files" on http://ge.tt.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I just looked into this, and the file picker code will have to be substantially rewritten if we want to support sandboxing. The problem Tom identified happens because we're trying to access a special kind of pref that stores the last directory that was picked. But the real problem is that content processes should not touch the file system.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: open-cross-process-stream

It turns out that we already have some Fennec-era code to support the file picker dialog across process. It won't work with sandboxing, so I filed bug 949666 to fix that. This bug will just cover getting the existing code to work again.

When the user selects a file, the filename is saved in the HTML input element. Then when we submit the form, we take the file (as an nsIDOMBlob), call GetInternalStream on it, and then pack the stream up into an IPC message requesting that the parent send an HTTP POST request.

When the stream is sent across processes, we use nsFileInputStream::Serialize on it. This function only works with streams that have already been opened. (It would be insecure to just send an arbitrary filename, since the parent would have no way to verify that it's safe to open.) Unfortunately, the stream that we get from GetInternalStream uses a special DEFER_OPEN mode, so it's not actually open when we try to serialize it.

To fix the problem, this patch just calls the Available method on the stream before sending it to the parent. That forces the file to be opened. Using a random method seems kind of hacky, but I'm not sure what else to do. I considered forcing the file open in nsFIS::Serialize, but that method has no way to signal errors. I also considered changing nsDOMFileFile::GetInternalStream so that it doesn't use the deferred open flag. But there are lots of other uses of that method, and I'm afraid to change their behavior. I could add a new method to nsIDOMBlob to get the stream using custom flags, but that also seems gross.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: content-pref-service

This patch disables the use of the content pref service from a child process. Bug 949666 covers running this code in the parent, where it should really be. For now, I think it's fine to always default to the desktop directory when using the file picker.

Not sure who should review, but it looks like Felipe has worked on this code before.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8346805 [details] [diff] [review]
content-pref-service

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

I can't officially give review here, so sending it over to Olli.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8346805 [details] [diff] [review]
content-pref-service

Someone more familiar with b2g should look at this too.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8346799 [details] [diff] [review]
open-cross-process-stream

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

This means doing an fopen on the main thread, right?  That kind of sucks :-/

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8346799 [details] [diff] [review]
open-cross-process-stream

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

You were going to do something else here, I think.

Comment 8

[Tom S [:evilpie]]

Attachment: my new approach

I thought I would update what I worked on last week.

1. Fixed FilePickerProxy to use the new asychronous ::Open instead of ::Show
2. Introduced a new IPDL protocol PFilePicker that manages the child<->parent communication.
3. The parent sends a filehandle and name to the child, thus allowing for sandboxing.

In the last point is also still probably the biggest problem. I reused RemoteOpenFileChild in a sort of hackish way, because I needed something that implements nsIFile and knows how to work with FileHandles. Sadly the class seems to not allow for any cloning and a lot of other features are also not implemented (like GetFileSize). Maybe this class is not actually the right choice.

Comment 9

[Tom S [:evilpie]]

Kyle do you know if it would make sense to extend RemoteOpenFileChild for our purpose, or should I rather abandon that approach?

Comment 10

[Tom S [:evilpie]]

Attachment: 4131615.txt

I got one file upload working, by delaying the time until we set the file descriptor to null (either by ::Clone or ::OpenNSPRFileDecr) to the last required access. I just had to cache the filesize. Obviously for the real world we still need clong/duplicating working. I attached the diff for that.

Comment 11

[Tom S [:evilpie]]

Attachment: v1

I just need some help figuring out how to neuter the FilePickerParent reference in the Callback of the parent.

Comment 12

[Tom S [:evilpie]]

Oh this also still needs the content-pref-service patch. I also dropped the proper FileHandle business, because the required cloning of file descriptors seems to be almost impossible in practice.

Comment 13

[Tom S [:evilpie]]

Comment on attachment 8368141 [details] [diff] [review]
v1

You reviewed the original nsFilePickerProxy code, can you review it or suggest somebody else?

Comment 14

[Tom S [:evilpie]]

Right now you can still crash when the file picker is open and the child dies.

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 8368141 [details] [diff] [review]
v1

This patch is missing the new FilePickerParent code.

Comment 16

[Josh Matthews [:jdm]]

And was it _really_ necessary to change the indentation on a bunch of the FilePickerProxy methods?

Comment 17

[Tom S [:evilpie]]

Attachment: v1 refreshed

> This patch is missing the new FilePickerParent code.
Sorry, attached a patch with them included.
> And was it _really_ necessary to change the indentation on a bunch of the FilePickerProxy methods?
10 methods that do nothing ....

Comment 18

[Josh Matthews [:jdm]]

Comment on attachment 8368141 [details] [diff] [review]
v1

It would be nice to clean up the list of attachments.

Comment 19

[Josh Matthews [:jdm]]

Comment on attachment 8369472 [details] [diff] [review]
v1 refreshed

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

Nice; good and readable. I'd just like to see another version with the issues addressed.

::: dom/ipc/FilePickerParent.cpp
@@ +22,5 @@
> +  { }
> +
> +  NS_DECL_ISUPPORTS
> +
> +  NS_IMETHOD Done(int16_t aResult) MOZ_OVERRIDE;

This should be NS_DECL_NSIFILEPICKERSHOWNCALLBACK

@@ +71,5 @@
> +    }
> +  }
> +
> +  mCallback = nullptr;
> +  mFilePicker = nullptr;

No need. These will be cleared in the destructor, which is called from Send__delete__.

@@ +84,5 @@
> +                           const nsString& aDefaultExtension,
> +                           const InfallibleTArray<nsString>& aFilters,
> +                           const InfallibleTArray<nsString>& aFilterNames)
> +{
> +  nsCOMPtr<nsIFilePicker> filePicker = do_CreateInstance("@mozilla.org/filepicker;1");

Just assign straight to mFilePicker instead of using a temporary.

@@ +100,5 @@
> +    return true;
> +  }
> +
> +  if (NS_FAILED(filePicker->Init(window, mTitle, mMode))) {
> +    return true;

All of these early returns will cause this actor to leak until the child process exits.

::: dom/ipc/FilePickerParent.h
@@ +14,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class FilePickerParent : public PFilePickerParent

Add an ActorDestroy method that notifies the callback that this object doesn't exist anymore. That will solve the unsafety mentioned in the XXX above.

@@ +23,5 @@
> +  : mTitle(aTitle)
> +  , mMode(aMode)
> +  {}
> +
> +  virtual ~FilePickerParent() {}

Don't inline this. I think that's why you needed to include nsIFilePicker.h elsewhere.

@@ +43,5 @@
> +  int16_t mMode;
> +};
> +
> +} // namespace dom
> +} // namespace mozillas

More mozillas, more problems.

::: dom/ipc/TabParent.cpp
@@ +37,5 @@
>  #include "nsIDOMElement.h"
>  #include "nsIDOMEvent.h"
>  #include "nsIDOMWindow.h"
>  #include "nsIDialogCreator.h"
> +#include "nsIFilePicker.h"

Why?

::: widget/xpwidgets/nsFilePickerProxy.cpp
@@ +155,4 @@
>  
> +bool
> +nsFilePickerProxy::Recv__delete__(const InfallibleTArray<InputFile>& files,
> +                                  const int16_t& result)

aFiles, aResult.

::: widget/xpwidgets/nsFilePickerProxy.h
@@ +43,5 @@
>      NS_IMETHODIMP GetFile(nsIFile** aFile);
>      NS_IMETHODIMP GetFileURL(nsIURI** aFileURL);
>      NS_IMETHODIMP GetFiles(nsISimpleEnumerator** aFiles);
>      NS_IMETHODIMP Show(int16_t* aReturn);
> +    NS_IMETHOD Open(nsIFilePickerShownCallback *aCallback);

Make this NS_IMETHODIMP like the others.

@@ +47,5 @@
> +    NS_IMETHOD Open(nsIFilePickerShownCallback *aCallback);
> +
> +    // PFilePickerChild
> +    virtual bool
> +    Recv__delete__(const InfallibleTArray<InputFile>& files, const int16_t& result);

aFiles, aResult.

Comment 20

[Tom S [:evilpie]]

Attachment: v2

I find the new error handling quite ugly. Maybe we should just make Open synchronous and have it return a boolean indicating success?

Comment 21

[Josh Matthews [:jdm]]

Comment on attachment 8369592 [details] [diff] [review]
v2

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

This looks good to me. If you aren't thrilled by all the Send__delete__ calls, you could put them behind a CancelEarly method like the HTTP code, which would read a bit clearer.

::: widget/xpwidgets/nsFilePickerProxy.cpp
@@ +156,5 @@
> +bool
> +nsFilePickerProxy::Recv__delete__(const MaybeInputFiles& aFiles,
> +                                  const int16_t& aResult)
> +{
> +  if (aFiles.type() == MaybeInputFiles::Tnull_t) {

Just surround the next block with |if (aFiles.type() == MaybeInputFiles::TInputFiles) {| and you can remove this block.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Tom, you need to hg add PFilePicker.ipdl.

Comment 23

[Tom S [:evilpie]]

Attachment: v2

This includes the PFilePicker.ipdl, which the previous patch was missing. Note it actually changed from v1, so that we can pass null_t.

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: file-input v2

Here's an updated version of Tom's patch.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: blob-changes

This patch uses remote blobs to send files across the IPC channel. It seems to work pretty well.

Comment 26

[Tom S [:evilpie]]

Attachment: file-input v2

Nice. I just updated file-input with some changes that I had locally. Used void_t instead of null_t. Removed the now unnecessary changes to the GTK WidgetFactory. Renamed ActorDestroy parameter to aWhy.

Comment 27

[Tom S [:evilpie]]

I think we might want to call GetSize and GetLastModifiedDate from an other thread, to avoid the possible "stat" call on the main-thread.

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Tom Schuster [:evilpie] from comment #27)
> I think we might want to call GetSize and GetLastModifiedDate from an other
> thread, to avoid the possible "stat" call on the main-thread.

Oh yeah, I considered that. I realized that we're already calling stat on the main thread in single-process Firefox:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsFormSubmission.cpp#531
GetSize calls stat as far as I can tell.

Also, it's hard to imagine how the inode for the file the user just picked wouldn't already be in the OS buffer cache. In order to display it in the file picker, I suspect we would have already had to stat it.

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8372902 [details] [diff] [review]
blob-changes

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

Overall this looks fine except for the main thread IO :(

::: dom/ipc/FilePickerParent.cpp
@@ +55,5 @@
>  
> +static void
> +AddFile(ContentParent* aParent, nsIFile* aFile, InfallibleTArray<PBlobParent*>& aFiles)
> +{
> +  if (!aFile) {

Nit: This seems like something you could assert instead of checking at runtime.

@@ +65,5 @@
> +  // We need to get the size and date for the file so that it doesn't appear as
> +  // a mystery blob to GetOrCreateActorForBlob.
> +  uint64_t size, lastModified;
> +  domfile->GetSize(&size);
> +  domfile->GetMozLastModifiedDate(&lastModified);

This is pretty awful, you're introducing main thread IO here. I know, it's not obvious, but our DOMFile API is pretty much a giant footgun. I don't know what the best fix is here (maybe a change to the FilePickerParent class that delays a call to Done() until the size is known, or fixing the actual filepicker code that is old as the hills...) but I don't think we should add more of this ever.

@@ +67,5 @@
> +  uint64_t size, lastModified;
> +  domfile->GetSize(&size);
> +  domfile->GetMozLastModifiedDate(&lastModified);
> +
> +  BlobParent *blob = aParent->GetOrCreateActorForBlob(domfile);

This can fail if the child process crashes I think... Do you care? I guess you eventually ignore the potential failure in FilePickerParent::Done() anyway but seems worth noting.

@@ +75,4 @@
>  void
>  FilePickerParent::Done(int16_t aResult)
>  {
> +  ContentParent *parent = static_cast<ContentParent *>(Manager()->Manager());

Nit: Here and everywhere in non-JS land we put the * on the left :)

@@ +75,5 @@
>  void
>  FilePickerParent::Done(int16_t aResult)
>  {
> +  ContentParent *parent = static_cast<ContentParent *>(Manager()->Manager());
> +  InfallibleTArray<PBlobParent*> files;

Since this is in the parent you probably should use a fallible array here and heck for failures. Then you can SwapElements into an InfallibleTArray before sending the response.

::: widget/xpwidgets/nsFilePickerProxy.cpp
@@ +99,5 @@
>  NS_IMETHODIMP
>  nsFilePickerProxy::GetFile(nsIFile** aFile)
>  {
>    NS_ENSURE_ARG_POINTER(aFile);
> +  return NS_ERROR_FAILURE;

It's a bug to call these so I think we should MOZ_ASSERT(false) here.

Comment 31

[Tom S [:evilpie]]

I landed the first two patches. With this the file picker works, Bill's blob patch is just a topping.
To make FilePickerParent compile on b2g, I had to move the callback class into the parent class, I hope nobody minds.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8da5f0f2f82c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e38ac18ab84

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8da5f0f2f82c
https://hg.mozilla.org/mozilla-central/rev/3e38ac18ab84

Comment 33

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: blob-changes v2

With the off-main-thread stat.

Comment 34

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8380036 [details] [diff] [review]
blob-changes v2

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

This looks great, thanks!

::: dom/ipc/FilePickerParent.cpp
@@ +149,3 @@
>    }
>  
> +  mRunnable = new FileSizeAndDateRunnable(this, domfiles);

Nit: I'd assert !mRunnable here. Is there any way that you can reenter here?

@@ +212,5 @@
>    if (mCallback) {
>      mCallback->Destroy();
>    }
> +  if (mRunnable) {
> +    mRunnable->Destroy();

Might as well set mRunnable to null here too, right?

::: widget/xpwidgets/nsFilePickerProxy.cpp
@@ +167,5 @@
> +  if (mDomfiles.IsEmpty()) {
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIDOMFile> domfile = mDomfiles[0];

Nit: Is it worth asserting that the length is exactly 1 here? Seems like that would help people find bugs more easily.

Comment 35

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/edf18d2929c3

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/edf18d2929c3

Comment 37

[Paul Silaghi, QA [:pauly]]

Reproduced in Nightly 2014-02-10, Win 7 x64.
Verified fixed FF 31 RC 2.