1186932 - Implement support for form submission of a picked directory

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonathan Watt [:jwatt]]

Following on from bug 1164310, we should add support for submitting forms where an input element has been used to pick a directory.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - FormData and WebIDL

This patch is needed to update HTMLFormSubmission algorithm everywhere, plus to have the WebIDL union Directory+Blob+USVString.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - Form submission with directory support

Here the good news: chrome doesn't submit the directory tree but it sends it as a empty blob with unknown type. This is good for the blink API subset, but maybe not for the Directory upload API.

I'll provide a test in a separate patch.

Comment 3

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

What does chrome upload? when you select a directory containing multiple files, what does it end up uploading?

Comment 4

[Andrea Marchesini [:baku]]

> What does chrome upload? when you select a directory containing multiple
> files, what does it end up uploading?

Uploading my 'Desktop' directory I see:

-----WebKitFormBoundary7LnfUzpK69k76eBA
Content-Disposition: form-data; name="foo"; filename="Desktop"
Content-Type: application/octet-stream

Nothing else.

Comment 5

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

And you actually had some files in Desktop ? ;)


So you're saying that uploading a directory doesn't actually upload anything, even though .files contains the files in the directory, but <input type=file multiple> still uploads all the files in .files?
Does <input type=file webkitdirectory> work differently to <input type=file webkitdirectory multiple> ?

Comment 6

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

When method=get is used, Chrome certainly uploads the name of the each file in the directory.
Didn't find example site for post testing and was lazy to setup local server.



What spec says FormDataEntryValue should include Directory? Per Upload spec form submission doesn't include the directory, only files.

Comment 7

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

Comment on attachment 8763357 [details] [diff] [review]
part 2 - Form submission with directory support

blink doesn't seem to add anything to form upload for directory, at least in method=get case, and per upload spec directories shouldn't be there either, so I don't quite understand all the directory handling her.e

Comment 8

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

Comment on attachment 8763356 [details] [diff] [review]
part 1 - FormData and WebIDL

So it is unclear to me why we want this, per which spec.

Seems like upload spec is missing the part how to create FormData asynchronously from HTMLFormElement, since that is need to have all the files in place.

Comment 9

[Andrea Marchesini [:baku]]

> blink doesn't seem to add anything to form upload for directory, at least in
> method=get case, and per upload spec directories shouldn't be there either,
> so I don't quite understand all the directory handling her.e

This setup is required for several reasons:

1. what does it happen if I have a form with an input type="file" containing a directory object and I do: new FormData(that_form).get(that_input) ? We should be consistent and support Directory everywhere when the pref is enabled.

2. for internal code is useful to have WebIDL to generate the type to support directory everywhere.

3. I need to support Directory in every HTMLFormSubmission and FormData is a HTMLFormSubmission...

Important to note is that this change is not exposed directly from FormData: you cannot append a directory for instance.

Definitely we should update the spec to introduce this changes.

About chrome, it does something quite wrong: FormData() converts any Directory object in a File object having:

file.name => name of the directory
file.size => 4096 (that is the default size of the directory on a linux filesystem, not the real size of the content of the directory)
file.type = ""
file.webkitRelativePath = ""

This clearly seems a bug in chrome.

Comment 10

[Andrea Marchesini [:baku]]

This is interesting. Chrome has the behavior implemented in this patch only for: <input type=file multiple>
For <input type=file webkitdirectory> it sends all the files recursively. This has to be implemented.

Comment 11

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

(In reply to Andrea Marchesini (:baku) from comment #10)
> This is interesting. Chrome has the behavior implemented in this patch only
> for: <input type=file multiple>
well, that selects just multiple files, so if you selected a directory (that shouldn't be possible, but maybe it is in Chrome?), you select just the directory, not what it contains



(In reply to Andrea Marchesini (:baku) from comment #9)
> > blink doesn't seem to add anything to form upload for directory, at least in
> > method=get case, and per upload spec directories shouldn't be there either,
> > so I don't quite understand all the directory handling her.e
> 
> This setup is required for several reasons:
> 
> 1. what does it happen if I have a form with an input type="file" containing
> a directory object and I do: new FormData(that_form).get(that_input) ? We
> should be consistent and support Directory everywhere when the pref is
> enabled.
Well, depends. Upload spec never submits directories, so if FormData is supposed to reflect what gets submitted, then it should not contain entries for Directories


> 
> 2. for internal code is useful to have WebIDL to generate the type to
> support directory everywhere.
If we want it for internal use, fine, but exposing it to the web (assuming Directory is enabled) isn't what the specs say

> 
> 3. I need to support Directory in every HTMLFormSubmission and FormData is a
> HTMLFormSubmission...
Why you need to support Directory in HTMLFormSubmission? The spec says one shouldn' submit anything related to Directories, only the files the directories contain. Though, upload spec doesn't matter here now, only
what blink API does. 
So, what does Chrome do when submitting a directory containing files and subdirectories?
(not talking about FormData API here, but the actual submission)


> Definitely we should update the spec to introduce this changes.
Yeah, filed https://github.com/WICG/directory-upload/issues/40


> About chrome, it does something quite wrong: FormData() converts any
> Directory object in a File object having:
What does this mean? Chrome doesn't have Directory objects and File objects don't have Directory objects.

> 
> file.name => name of the directory
> file.size => 4096 (that is the default size of the directory on a linux
> filesystem, not the real size of the content of the directory)
> file.type = ""
> file.webkitRelativePath = ""
> 
> This clearly seems a bug in chrome.
Could you please ask MS folks what Edge will do?
What does Chrome do on OSX or Windows (Windows especially)?

Comment 12

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

And need to test both webkitdirectory and dnd+webkitEntries here. Those are separate cases.

Comment 13

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

(In reply to Andrea Marchesini (:baku) from comment #9)
> file.name => name of the directory
> file.size => 4096 (that is the default size of the directory on a linux
> filesystem, not the real size of the content of the directory)
> file.type = ""
> file.webkitRelativePath = ""
> 
> This clearly seems a bug in chrome.

If a FormData containing this kind of odd File is then submitted to server using XHR, what gets uploaded?

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 1 - FormData and Directory

This is needed and it's not exposed except when Directory API is enabled.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 2 - Form submission with directory support

With this patch we support the directory objects listed as input values but no content. This is what chromium does when the HTMLInputElement doesn't have webkitdirectory attribute.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 3 - tests + fixes

Here tests. The behavior here implemented is: when a directory is DnDed in a <input type="file" multiple webkitdirectory name="foo" /> the result is:

foo.files => [ array of sub Files ]
foo.webkitEntries = []
submit => all the files sent

For: <input type="file" multiple name="foo" />

foo.files => []
foo.webkitEntries = [ DirectoryEntry ]
submit =>  Content-Disposition: form-data; name="foo"; filename="data"\r\nContent-Type: application/octet-stream

In this last test, chromium populates "files" with a File object with size 4096 and no type. Using this File object will make any API to fail. This seems a chromium bug because 4096 is the default size of a directory on linux filesystems.

Comment 17

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

Comment on attachment 8764013 [details] [diff] [review]
part 1 - FormData and Directory

>+FSURLEncoded::AddNameDirectoryPair(const nsAString& aName,
>+                                   Directory* aDirectory)
>+{
>+  // TODO
>+  return NS_OK;
>+}
I assume the Todos are fixed in the following patches.

Comment 18

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

Comment on attachment 8764014 [details] [diff] [review]
part 2 - Form submission with directory support

>+FSMultipartFormData::AddNameDirectoryPair(const nsAString& aName,
>+                                          Directory* aDirectory)
>+{
>+  // Encode the control name
>+  nsAutoCString nameStr;
>+  nsresult rv = EncodeVal(aName, nameStr, true);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsAutoCString dirname;
>+  nsAutoString dirname16;
>+
>+  if (Directory::WebkitBlinkDirectoryPickerEnabled(nullptr, nullptr)) {
>+    ErrorResult error;
>+    nsAutoString path;
>+    aDirectory->GetPath(path, error);
>+    if (NS_WARN_IF(error.Failed())) {
>+      error.SuppressException();
>+    } else {
>+      dirname16 = path;
>+    }
>+  }
Shouldn't we return early if the pref isn't enabled. We don't support Directory upload stuff here anyway, so better to not even enable anything related to it
So, change the test to

if (!Directory::WebkitBlinkDirectoryPickerEnabled(nullptr, nullptr)) {
  return NS_OK;
}

Comment 19

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

Comment on attachment 8764015 [details] [diff] [review]
part 3 - tests + fixes


>   Callback(nsresult aStatus, const Sequence<RefPtr<File>>& aFiles) override
>   {
>-    if (NS_SUCCEEDED(aStatus)) {
>-      mInputElement->AfterSetFilesOrDirectoriesInternal(mSetValueChanged);
>-    }
>+    if (NS_WARN_IF(NS_FAILED(aStatus))) {
>+      return;
>+    }
>+
>+    nsTArray<OwningFileOrDirectory> array;
>+    for (uint32_t i = 0; i < aFiles.Length(); ++i) {
>+      OwningFileOrDirectory* element = array.AppendElement();
>+      element->SetAsFile() = aFiles[i];
>+    }
>+
>+    mInputElement->mFilesOrDirectories.Clear();
>+    mInputElement->mFilesOrDirectories.AppendElements(array);
Why we need the temporary 'array' here?
Couldn't you just append elements on by one to mFilesOrDirectories?
But especially with this change, I don't understand the reason to have both AfterSetFilesOrDirectoriesCallback and DispatchChangeEventCallback.
Please fix this somehow or add good comments what the both callback classes are used for.


> void
> HTMLInputElement::MozSetDndFilesAndDirectories(const nsTArray<OwningFileOrDirectory>& aFilesOrDirectories)
Could you add a comment before the method name (both in .h and .cpp) that the method is for testing only.
I just want to prevent us to use this stuff when dnd is implemented. Event dispatching may not be quite right, and needs testing then.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 3 - tests + fixes

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 4 - interdiff

I didn't merge the 2 callbacks but I added comments to make the logic more clear.

Comment 22

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

Comment on attachment 8765832 [details] [diff] [review]
part 4 - interdiff

some clarifications to AfterSetFilesOrDirectoriesCallback, please, per IRC.
(sorry, I'm dumb today :) )

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 3 - tests + fixes

Comment 24

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

Comment on attachment 8765919 [details] [diff] [review]
part 3 - tests + fixes

Could GetFilesCallback be merged now to DispatchChangeEventCallback?
But separate bug or patch if we want to do that.

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 4 - Moving GetFilesHelper to separate files

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 5 - preferences enabled for testing

Comment 27

[Andrea Marchesini [:baku]]

Attachment: part 6 - GetFilesHelper executed on the parent process

This is the main patch for this second round of review: File objects with an underlying real file, must be created on the parent process because otherwise they cannot be used in any parent-side API based on nsIInputStream.

In parent process/non-e10s, File->GetInternalStream() returns a  NS_NewLocalFileInputStream if the File is a BlobImplFile. But for e10s, we use a remote input stream based on the sharing of FileDescriptors (see dom/ipc/PBlobStream.ipdl). This works only in one direction parent->child where the parent opens the FDs and it shares them with the child.

In the previous setup, the File object was created on the child process and everything was broken. Plus, we should avoid any FileSystem operation in the child process.

Comment 28

[Andrea Marchesini [:baku]]

Attachment: part 7 - path in RemoteBlobImpl

We have to propagate the path from parent to child process. This is the last bit to make HTML Form Submission to work fine.

Comment 29

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

Comment on attachment 8766805 [details] [diff] [review]
part 4 - Moving GetFilesHelper to separate files

Tiny bit annoying to review when "move" includes also un-inlining some methods.

Comment 30

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

Comment on attachment 8767022 [details] [diff] [review]
part 7 - path in RemoteBlobImpl


>+    if (NS_WARN_IF(!params.path().IsEmpty())) {
>+      ASSERT_UNLESS_FUZZING();
>+      return nullptr;
>+    }
I don't understand this assertion. Why path must be empty?
Please add a comment.
Is the idea that we try to ensure blobs bound to some path are always created in parent process, and not sent
from child to parent?

Comment 31

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

Comment on attachment 8766806 [details] [diff] [review]
part 5 - preferences enabled for testing

Is this a wrong patch or something? The title says "preferences enabled for testing ", but the patch has nothing to do with testing or preferences.

Please upload the right patch, or explain what this patch is about and ask review again.

Comment 32

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

Yes, part 5 is partially the same as part 6, but they aren't exactly the same. 
So I think there is some issue with generating those patches.

Comment 33

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

Comment on attachment 8767021 [details] [diff] [review]
part 6 - GetFilesHelper executed on the parent process

So I'm not sure if this is the right patch either then.

Comment 34

[Andrea Marchesini [:baku]]

Attachment: part 5 - preferences enabled for testing

Comment 35

[Andrea Marchesini [:baku]]

Attachment: part 6 - GetFilesHelper executed on the parent process

Comment 36

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

Comment on attachment 8767563 [details] [diff] [review]
part 5 - preferences enabled for testing

Do we have tests for dom.webkitBlink.dirPicker.enabled + dom.webkitBlink.filesystem.enabled only, without dom.input.dirpicker ?

If so, r+. If not, modify the test so that it can be used for webkit stuff only too.

Comment 37

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

Comment on attachment 8767564 [details] [diff] [review]
part 6 - GetFilesHelper executed on the parent process

>+GetFilesHelperChild::SetResults(nsresult aStatus)
>+{
>+  MOZ_ASSERT(mPendingOperation);
>+  MOZ_ASSERT(NS_FAILED(aStatus));
>+  mPendingOperation = false;
>+
>+  mErrorResult = aStatus;
>+  OperationCompleted();
>+}
>+
>+void
>+GetFilesHelperChild::SetResults(const nsTArray<PBlobChild*>& aBlobs)
Couldn't you just have one SetResults, which takes perhaps a pointer to an array (possibly null) and nsresult.
Up to you, but I think it would make the code simpler.

>+{
>+  MOZ_ASSERT(mPendingOperation);
>+  mPendingOperation = false;
>+
>+  mErrorResult = NS_OK;
>+
>+  for (uint32_t i = 0; i < aBlobs.Length(); ++i) {
>+    RefPtr<BlobImpl> impl = static_cast<BlobChild*>(aBlobs[i])->GetBlobImpl();
>+    MOZ_ASSERT(impl->IsFile());
>+
>+    RefPtr<File> file = File::Create(mGlobal, impl);
>+    MOZ_ASSERT(file);
>+
>+    if (!mFiles.AppendElement(file, fallible)) {
>+      mErrorResult = NS_ERROR_OUT_OF_MEMORY;
>+      break;
>+    }
>+  }
It would be nice to keep the IPC stuff in ipc layer as much as possible. So, would it be possible that RecvGetFilesResponse goes through the
result and creates the File objects? Or perhaps GetFilesHelperChild could have
AppendResult(BlobImpl*); method and then some Finish(nsresult aRv);
In error case only Finish was called.
But this is just an optional way to implement this. So the approach the patch has taken is ok too.



>@@ -657,16 +673,18 @@ private:
>   bool mIsAlive;
>   nsString mProcessName;
> 
>   static ContentChild* sSingleton;
> 
>   nsCOMPtr<nsIDomainPolicy> mPolicy;
>   nsCOMPtr<nsITimer> mForceKillTimer;
> 
>+  nsRefPtrHashtable<nsIDHashKey, GetFilesHelperChild> mGetFilesPendingRequests;
This must have a comment. Took a bit time to understand what this is for and how it is used.


> #ifdef NS_PRINTING
>   RefPtr<embedding::PrintingParent> mPrintingParent;
> #endif
>+
>+  nsRefPtrHashtable<nsIDHashKey, GetFilesHelper> mGetFilesPendingRequests;
This too.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac9e026e72e
Implement support for form submission of a picked directory - part 1 - FormData and Directory, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8105df56b232
Implement support for form submission of a picked directory - part 2 - Form submission with directory support, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e3c703163d
Implement support for form submission of a picked directory - part 3 - tests + fixes, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/57a8705babf1
Implement support for form submission of a picked directory - part 4 - Moving GetFilesHelper to separate files, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae45daca7d02
Implement support for form submission of a picked directory - part 5 - preferences enabled for testing, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee73417964bb
Implement support for form submission of a picked directory - part 6 - GetFilesHelper executed on the parent process, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d969bf83027
Implement support for form submission of a picked directory - part 7 - path in RemoteBlobImpl, r=smaug

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e5ba0ffc0c2
Implement support for form submission of a picked directory - part 8 - explicit for GetFilesHelper, r=me

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ac9e026e72e
https://hg.mozilla.org/mozilla-central/rev/8105df56b232
https://hg.mozilla.org/mozilla-central/rev/e3e3c703163d
https://hg.mozilla.org/mozilla-central/rev/57a8705babf1
https://hg.mozilla.org/mozilla-central/rev/ae45daca7d02
https://hg.mozilla.org/mozilla-central/rev/ee73417964bb
https://hg.mozilla.org/mozilla-central/rev/2d969bf83027
https://hg.mozilla.org/mozilla-central/rev/1e5ba0ffc0c2