Last Comment Bug 690659 - filename parameter in the FormData.append method
: filename parameter in the FormData.append method
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: mozilla22
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
http://xhr.spec.whatwg.org/#dom-formd...
: 739174 (view as bug list)
Depends on:
Blocks: 832923
  Show dependency treegraph
 
Reported: 2011-09-29 22:53 PDT by Andrey Mironov
Modified: 2013-03-22 10:28 PDT (History)
14 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to conform FormData.append to the XHR2 Spec (11.43 KB, patch)
2012-02-01 07:54 PST, John Villar
no flags Details | Diff | Splinter Review
Newest patch, fixed minor problems (11.66 KB, patch)
2012-02-01 08:10 PST, John Villar
jonas: review-
Details | Diff | Splinter Review
v2 (14.92 KB, patch)
2013-02-17 10:41 PST, Tom Schuster [:evilpie]
jonas: review+
Details | Diff | Splinter Review
v3 (15.07 KB, patch)
2013-02-20 06:11 PST, Tom Schuster [:evilpie]
jonas: review+
Details | Diff | Splinter Review
test (3.96 KB, patch)
2013-02-20 06:13 PST, Tom Schuster [:evilpie]
jonas: review+
Details | Diff | Splinter Review
test v2 (5.38 KB, patch)
2013-02-28 04:44 PST, Tom Schuster [:evilpie]
jonas: review+
Details | Diff | Splinter Review
fwiw, this fixed the bustage for me... (760 bytes, patch)
2013-03-03 12:13 PST, Mats Palmgren (vacation)
Ms2ger: review+
Details | Diff | Splinter Review

Description Andrey Mironov 2011-09-29 22:53:32 PDT
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 2012-01-31 13:45:02 PST
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 2012-02-01 07:54:57 PST
Created attachment 593450 [details] [diff] [review]
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] 2012-02-01 08:00:24 PST
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 2012-02-01 08:10:10 PST
Created attachment 593456 [details] [diff] [review]
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) PTO Until July 5th 2012-02-08 17:59:43 PST
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 2012-05-11 11:45:34 PDT
Hi, any progress on this?
Comment 7 John Villar 2012-05-11 11:56:38 PDT
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 2012-12-09 16:12:08 PST
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 9 Tom Schuster [:evilpie] 2013-02-17 06:19:03 PST
*** Bug 739174 has been marked as a duplicate of this bug. ***
Comment 10 Tom Schuster [:evilpie] 2013-02-17 10:41:30 PST
Created attachment 714944 [details] [diff] [review]
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) PTO Until July 5th 2013-02-19 10:56:46 PST
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) PTO Until July 5th 2013-02-19 10:57:19 PST
But this does need tests before it can land.
Comment 13 Tom Schuster [:evilpie] 2013-02-20 06:11:37 PST
Created attachment 716005 [details] [diff] [review]
v3

I had one small error in the previous patch, i was using the wrong filename to EncodeVal.
Comment 14 Tom Schuster [:evilpie] 2013-02-20 06:13:49 PST
Created attachment 716006 [details] [diff] [review]
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) PTO Until July 5th 2013-02-24 23:57:27 PST
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 Schuster [:evilpie] 2013-02-25 06:24:28 PST
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) PTO Until July 5th 2013-02-25 09:37:48 PST
Ugh, that is very silly but indeed correct.
Comment 18 Tom Schuster [:evilpie] 2013-02-27 11:35:48 PST
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) PTO Until July 5th 2013-02-27 13:13:55 PST
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 Schuster [:evilpie] 2013-02-28 04:44:39 PST
Created attachment 719435 [details] [diff] [review]
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) PTO Until July 5th 2013-02-28 11:30:14 PST
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 23 Mats Palmgren (vacation) 2013-03-03 12:08:44 PST
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 (vacation) 2013-03-03 12:13:49 PST
Created attachment 720450 [details] [diff] [review]
fwiw, this fixed the bustage for me...
Comment 25 :Ms2ger 2013-03-03 12:18:44 PST
Comment on attachment 720450 [details] [diff] [review]
fwiw, this fixed the bustage for me...

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

Ship it.

Note You need to log in before you can comment on or make changes to this bug.