Closed Bug 910384 Opened 6 years ago Closed 6 years ago

[e10s] File picker doesn't work

Categories

(Toolkit :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox31 --- verified

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(4 files, 8 obsolete files)

1.93 KB, patch
gwagner
: review+
smaug
: review+
Felipe
: feedback+
Details | Diff | Splinter Review
8.81 KB, patch
bent.mozilla
: review-
Details | Diff | Splinter Review
31.23 KB, patch
Details | Diff | Splinter Review
13.20 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
* 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.
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.
Summary: Electrolysis: this._cps._dbConnection is null → [e10s] File picker doesn't work
Attached patch open-cross-process-stream (obsolete) — Splinter Review
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.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8346799 - Flags: review?(khuey)
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.
Attachment #8346805 - Flags: review?(felipc)
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.
Attachment #8346805 - Flags: review?(felipc)
Attachment #8346805 - Flags: review?(bugs)
Attachment #8346805 - Flags: feedback+
Comment on attachment 8346805 [details] [diff] [review]
content-pref-service

Someone more familiar with b2g should look at this too.
Attachment #8346805 - Flags: review?(bugs)
Attachment #8346805 - Flags: review?(anygregor)
Attachment #8346805 - Flags: review+
Attachment #8346805 - Flags: review?(anygregor) → review+
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 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.
Attachment #8346799 - Flags: review?(khuey)
Assignee: wmccloskey → evilpies
Attached patch my new approach (obsolete) — Splinter Review
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.
Kyle do you know if it would make sense to extend RemoteOpenFileChild for our purpose, or should I rather abandon that approach?
Flags: needinfo?(khuey)
Depends on: 963294
Attached patch 4131615.txt (obsolete) — Splinter Review
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.
Attached patch v1 (obsolete) — Splinter Review
I just need some help figuring out how to neuter the FilePickerParent reference in the Callback of the parent.
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 on attachment 8368141 [details] [diff] [review]
v1

You reviewed the original nsFilePickerProxy code, can you review it or suggest somebody else?
Attachment #8368141 - Flags: review?(josh)
Right now you can still crash when the file picker is open and the child dies.
Comment on attachment 8368141 [details] [diff] [review]
v1

This patch is missing the new FilePickerParent code.
Attachment #8368141 - Flags: review?(josh)
And was it _really_ necessary to change the indentation on a bunch of the FilePickerProxy methods?
Attached patch v1 refreshed (obsolete) — Splinter Review
> 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 ....
Attachment #8369472 - Flags: review?(josh)
Comment on attachment 8368141 [details] [diff] [review]
v1

It would be nice to clean up the list of attachments.
Attachment #8368141 - Attachment is obsolete: true
Attachment #8367559 - Attachment is obsolete: true
Attachment #8361234 - Attachment is obsolete: true
Attachment #8346799 - Attachment is obsolete: true
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.
Attachment #8369472 - Flags: review?(josh) → feedback+
Attached patch v2 (obsolete) — Splinter Review
I find the new error handling quite ugly. Maybe we should just make Open synchronous and have it return a boolean indicating success?
Attachment #8369472 - Attachment is obsolete: true
Attachment #8369592 - Flags: review?(josh)
Flags: needinfo?(khuey)
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.
Attachment #8369592 - Flags: review?(josh) → review+
Tom, you need to hg add PFilePicker.ipdl.
Attached patch v2 (obsolete) — Splinter Review
This includes the PFilePicker.ipdl, which the previous patch was missing. Note it actually changed from v1, so that we can pass null_t.
Attachment #8369592 - Attachment is obsolete: true
Depends on: 966467
No longer depends on: 963294
Attached patch file-input v2 (obsolete) — Splinter Review
Here's an updated version of Tom's patch.
Attachment #8371882 - Attachment is obsolete: true
Attachment #8372901 - Flags: review+
Attached patch blob-changesSplinter Review
This patch uses remote blobs to send files across the IPC channel. It seems to work pretty well.
Attachment #8372902 - Flags: review?(bent.mozilla)
Attached patch file-input v2Splinter Review
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.
Attachment #8372901 - Attachment is obsolete: true
I think we might want to call GetSize and GetLastModifiedDate from an other thread, to avoid the possible "stat" call on the main-thread.
(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 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.
Attachment #8372902 - Flags: review?(bent.mozilla) → review-
Duplicate of this bug: 973536
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
Whiteboard: [leave open]
Attached patch blob-changes v2Splinter Review
With the off-main-thread stat.
Attachment #8380036 - Flags: review?(bent.mozilla)
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.
Attachment #8380036 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/edf18d2929c3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Blocks: 992415
Reproduced in Nightly 2014-02-10, Win 7 x64.
Verified fixed FF 31 RC 2.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.