690659 - filename parameter in the FormData.append method

Status: RESOLVED FIXED

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

Description

[Andrey Mironov]

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 http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#the-append-method
Right now the usage of FormData interface is limited, since many web applications validate uploaded file by its extension.

Comment 1

[John Villar]

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?

Comment 2

[John Villar]

Attachment: Patch to conform FormData.append to the XHR2 Spec

This patch fixes FormData.append so it conforms to the XMLHttpRequest Level 2 Specification.

Comment 3

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

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.

Comment 4

[John Villar]

Attachment: Newest patch, fixed minor problems

Fixed patch so it conforms to some style requirements (80 columns max) and removed an "String.Length() == 0" error from the code.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[Bronislav Klučka]

Hi, any progress on this?

Comment 7

[John Villar]

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. :-(

Comment 8

[Victor Costan]

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!

Comment 10

[Tom S [:evilpie]]

Attachment: v2

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.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

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.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But this does need tests before it can land.

Comment 13

[Tom S [:evilpie]]

Attachment: v3

I had one small error in the previous patch, i was using the wrong filename to EncodeVal.

Comment 14

[Tom S [:evilpie]]

Attachment: test

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.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 716006 [details] [diff] [review]
test

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.

Comment 16

[Tom S [:evilpie]]

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.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ugh, that is very silly but indeed correct.

Comment 18

[Tom S [:evilpie]]

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.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMBlobBuilder.cpp#296

Comment 20

[Tom S [:evilpie]]

Attachment: test v2

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.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 22

[Tom S [:evilpie]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1370add87e77
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9f4e7e70cd69

Comment 23

[Mats Palmgren (inactive)]

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 24

[Mats Palmgren (inactive)]

Attachment: fwiw, this fixed the bustage for me...

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 720450 [details] [diff] [review]
fwiw, this fixed the bustage for me...

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

Ship it.

Comment 26

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d077ae8a9b2

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1370add87e77
https://hg.mozilla.org/mozilla-central/rev/9f4e7e70cd69
https://hg.mozilla.org/mozilla-central/rev/1d077ae8a9b2

Comment 28

[Tom S [:evilpie]]

I am documented this as well:
https://developer.mozilla.org/en-US/docs/DOM/XMLHttpRequest/FormData
https://developer.mozilla.org/en-US/docs/Firefox_22_for_developers