filename parameter in the FormData.append method

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Andrey Mironov, Assigned: evilpie)

Tracking

({dev-doc-complete})

Trunk
mozilla22
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 2

6 years ago
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.
Attachment #593450 - Flags: review?(bugs)

Comment 3

6 years ago
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)

Comment 4

6 years ago
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.
Attachment #593450 - Attachment is obsolete: true
Attachment #593450 - Flags: review?(jonas)
Attachment #593456 - Flags: review?(bugs)

Updated

6 years ago
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.
Attachment #593456 - Flags: review?(jonas) → review-

Comment 6

5 years ago
Hi, any progress on this?

Comment 7

5 years ago
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

5 years ago
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!
Duplicate of this bug: 739174
Assignee: johnvillarzavatti → evilpies
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 7 Branch → Trunk

Updated

5 years ago
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.
Attachment #593456 - Attachment is obsolete: true
Attachment #714944 - Flags: review?(jonas)
Keywords: dev-doc-needed
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.
Attachment #714944 - Flags: review?(jonas) → review+
But this does need tests before it can land.
Created attachment 716005 [details] [diff] [review]
v3

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)
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.
Attachment #716006 - Flags: review?(jonas)
Attachment #716005 - Flags: review?(jonas) → review+
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.
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.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMBlobBuilder.cpp#296
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.
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+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1370add87e77
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9f4e7e70cd69
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;
  ^
Created attachment 720450 [details] [diff] [review]
fwiw, this fixed the bustage for me...
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d077ae8a9b2
https://hg.mozilla.org/mozilla-central/rev/1370add87e77
https://hg.mozilla.org/mozilla-central/rev/9f4e7e70cd69
https://hg.mozilla.org/mozilla-central/rev/1d077ae8a9b2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Blocks: 832923
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.