Closed Bug 690659 Opened 12 years ago Closed 11 years ago

filename parameter in the FormData.append method


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

Not set





(Reporter: andrey.mir, Assigned: evilpie)




(Keywords: dev-doc-complete)


(3 files, 4 obsolete files)

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

Steps to reproduce:

In the updated spec FormData.append method have third parameter, which allows to specify the for the uploaded file
Right now the usage of FormData interface is limited, since many web applications validate uploaded file by its extension.
I'm currently working on a patch for this. Could any official Mozilla team member get Ownership of this issue so i can eventually submit my patch for review?
Assignee: nobody → johnvillarzavatti
Ever confirmed: true
This patch fixes FormData.append so it conforms to the XMLHttpRequest Level 2 Specification.
Attachment #593450 - Flags: review?(bugs)
Comment on attachment 593450 [details] [diff] [review]
Patch to conform FormData.append to the XHR2 Spec

> [scriptable, uuid(256c9139-5a29-41e1-8698-f9f9aae7d6cf)]
You should update the uuid

But Jonas knows this code a lot better than I do.
Attachment #593450 - Flags: review?(bugs) → review?(jonas)
Fixed patch so it conforms to some style requirements (80 columns max) and removed an "String.Length() == 0" error from the code.
Attachment #593450 - Attachment is obsolete: true
Attachment #593450 - Flags: review?(jonas)
Attachment #593456 - Flags: review?(bugs)
Attachment #593456 - Flags: review?(bugs) → review?(jonas)
Component: General → DOM: Core & HTML
Product: Firefox → Core
QA Contact: general → general
There's two problems with this patch.

First off, you're currently treating "" as "not passing the argument", whereas the spec says to treat that as "use empty string as name". To fix this you should use the [optional_argc] feature to check if the argument was specified instead.

Second, the hasFilename member is unnecessary. Just set the filename to null (call filename.SetIsVoid(true)) to indicate that no name is specified.
Hi, any progress on this?
Unfortunately, i haven't been able to dedicate enough time to solve this problem, but anyone is welcome to pick up the patch and fix the errors in it. I'm currently finishing an internal project and there's little time i can put on this. :-(
Providing a real-world argument for fixing this bug, in case it helps.

dropbox.js users get a degraded experience in Firefox because of this bug. In WebKit, most file writes are done using a multipart/form-data XHR request that doesn't require CORS preflight. In Firefox, this bug forces us to use a different server-side API that does require a CORS preflight. The main HTTP request has to wait on the OPTIONS request to complete, which can have a significant impact for users on high-latency connections.

I hope this helps!
Assignee: johnvillarzavatti → evilpies
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 7 Branch → Trunk
Attached patch v2 (obsolete) — Splinter Review
This patch should include all the comments you made on the previous patch. The way I am handling the optional argument seem to allow for omitting [optional_argc], without the described wrong behavior.
Attachment #593456 - Attachment is obsolete: true
Attachment #714944 - Flags: review?(jonas)
Keywords: dev-doc-needed
Comment on attachment 714944 [details] [diff] [review]

Review of attachment 714944 [details] [diff] [review]:

::: content/html/content/src/nsFormSubmission.cpp
@@ +467,5 @@
> +    if (!aFilename.IsVoid()) {
> +      rv = EncodeVal(aFilename, filename, true);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    } else {
> +      if (filename16.IsEmpty()) {

Move the code which gets the filename into this else-branch.
Attachment #714944 - Flags: review?(jonas) → review+
But this does need tests before it can land.
Attached patch v3Splinter Review
I had one small error in the previous patch, i was using the wrong filename to EncodeVal.
Attachment #714944 - Attachment is obsolete: true
Attachment #716005 - Flags: review?(jonas)
Attached patch test (obsolete) — Splinter Review
Tests different combinations for |filename| that I could come up with. Also adds SpecialPowers.getFile for construction a File with only filename, data and properties.
Attachment #716006 - Flags: review?(jonas)
Comment on attachment 716006 [details] [diff] [review]

Review of attachment 716006 [details] [diff] [review]:

::: testing/specialpowers/content/specialpowersAPI.js
@@ +1308,5 @@
>    },
> +
> +  getFile: function(window, name, blobparts, parameters) {
> +    var utils = this.getDOMWindowUtils(window);
> +    return utils.getFile(name, blobparts, parameters);

Did you test

new window.File(new Blob(blobparts), { type: parameters.type, name: name })

I.e. the File constructor takes a blob as it's first argument, and a dictionary as the second argument which can contain "type", "name" etc.

If that works, even cleaner would be to make the parameters object include the name rather than having it as a separate argument.
Attachment #716006 - Flags: review?(jonas) → review+
Thanks for the reviews!

Sadly the constructor of |File| only takes a string as filepath, nsIFile or nsIDOMFile as the first argument, the second argument is the PropertyBag that you described. I think it would make more sense when the constructor more closely matched the |Blob| constructor.
Ugh, that is very silly but indeed correct.
TypeError: Value does not implement interface Blob. at http://mochi.test:8888/tests/content/html/content/test/test_formData.html:58

We have some problems with the overloading and the second parameter not being recognized as a Blob because it's cross-origin. Or something like that.
I think the simplest solution would be to switch to using:

new window.File(new Blob(blobparts), { type: parameters.type, name: name })

and then change the code at [1] to use an nsIDOMBlob rather than an nsIDOMFile. I think you just need to change the type (and the name) and things should work fine.

Attached patch test v2Splinter Review
Update test. Changes the File constructor to allow Blobs. This however still did not fix the error, so I am now explicitly unwrapping the File.
Attachment #716006 - Attachment is obsolete: true
Attachment #719435 - Flags: review?(jonas)
Comment on attachment 719435 [details] [diff] [review]
test v2

Review of attachment 719435 [details] [diff] [review]:

r=me with that fixed.

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +296,2 @@
>    nsCOMPtr<nsIFile> file;
>    nsCOMPtr<nsIDOMFile> domFile;

Simply changing this to be "nsCOMPtr<nsIDOMBlob> domBlob" should work...

@@ +312,5 @@
> +    nsCOMPtr<nsIDOMBlob> blob = do_QueryInterface(supports);
> +    if (blob) {
> +      if (mContentType.IsEmpty()) {
> +        blob->GetType(mContentType);
> +      }

... then you should be able to remove all of this

@@ +325,1 @@
>      domFile = do_QueryInterface(supports);

Since the QI here will become

domBlob = do_QueryInterface(supports);

which succeeds either if the "supports" is an nsIDOMFile or an nsIDOMBlob (file inherits blob).

::: content/html/content/test/test_formData.html
@@ +51,5 @@
> +    var fd = new FormData();
> +    fd.append("empty", blob);
> +    fd.append("explicit", blob, "explicit-file-name");
> +    fd.append("explicit-empty", blob, "");
> +    file = SpecialPowers.unwrap(SpecialPowers.wrap(window).File(blob, {name: 'testname'}));

I take it adding a function to SpecialPowers with something like:

createNamedFile(window, blob, name) {
  return new window.File(blob, { name: name, type: blob.type })

Didn't work?

No big deal either way though, as long as your approach works that's absolutely fine too.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +1274,5 @@
>    },
>    isWindowPrivate: function(win) {
>      return PrivateBrowsingUtils.isWindowPrivate(win);
> +  }

Please remove this change.
Attachment #719435 - Flags: review?(jonas) → review+
This appears to have broken my build (FF debug Linux64, clang 3.2):
content/base/src/nsFormData.cpp:86:30: error: copying parameter of type 'Optional<nsAString_internal>' invokes deleted constructor
      Append(aName, domBlob, Optional<nsAString>());
../../../dist/include/mozilla/dom/BindingDeclarations.h:310:3: note: function has been explicitly marked deleted here
  Optional(const Optional& other) MOZ_DELETE;
Comment on attachment 720450 [details] [diff] [review]
fwiw, this fixed the bustage for me...

Review of attachment 720450 [details] [diff] [review]:

Ship it.
Attachment #720450 - Flags: review+
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Blocks: 832923
You need to log in before you can comment on or make changes to this bug.