Closed Bug 1127150 Opened 9 years ago Closed 9 years ago

Remove explicit filename field in form submission infrastructure

Categories

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

33 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: nsm, Assigned: nsm)

References

Details

Attachments

(1 file, 1 obsolete file)

The spec distinguishes between File and Blob so that:

  var f = new FormData();
  // Spec says if a Blob (which is not a File) is added, the name parameter is effectively ignored, always set to "blob".
  f.append("blob", new Blob(["hi"]), "blob1.txt");
  is(f.get("blob").name, "blob", "Blob's filename should be blob irrespective of what was passed.");
  // Fails in gecko as undefined (only because I haven't implemented get() correctly in Bug 1085283). Succeeds in actual XHR send().

  // Also this should succeed since spec says always create a new File object.
  is(f.get("blob") instanceof File, "...");

  // Spec says if a File is added, the name parameter is respected if passed.
  f.append("file1", new File(["hi"], "file1.txt"));
  is(f.get("file1").name, "file1.txt", "File's filename should be original's name if no filename is explicitly passed.");
  // This works fine.

  f.append("file2", new File(["hi"], "file2.txt"), "fakename.txt");
  is(f.get("file2").name, "fakename.txt", "File's filename should be explicitly passed name.");
  // This fails in Gecko.

I'm going to remove the separate filename we store in FormDataTuple and use File as specified by the spec.
Assignee: nobody → nsm.nikhil
Attachment #8563721 - Flags: review?(amarchesini) → review+
Backed out because it blocked the backout of bug 1085283.
And now for mochitest failures of its own.
https://treeherder.mozilla.org/logviewer.html#?job_id=6850396&repo=mozilla-inbound

Please verify your patches are green on Try before pushing. Especially on the weekend before an uplift.
The whatwg spec and w3c spec disagree on the behaviour of append.

Specifically:
W3C: "If value is a Blob, set value to a new File object whose name attribute value is "blob".

If value is a File and filename is given, set value's name attribute value to filename. "

WHATWG: "If value is a Blob object and not a File object, set value to a new File object, representing the same bytes, whose name attribute value is "blob".

If value is a File object and filename is given, set value to a new File object, representing the same bytes, whose name attribute value is filename."

The wpt tests follow w3c. Can I update the test or do wpt tests only follow w3c standards?
Flags: needinfo?(james)
Yes, I think the WHATWG one is the correct one to follow. Adding annevk and hallvors so they are aware of this.
Flags: needinfo?(james)
Nikhil, note that by the point step 4 is run, all Blob objects are File objects so the filename parameter affects all of them.

I would be somewhat open to removing the filename parameter if that makes things better. It's an artifact that makes this not a clean "MultiMap" and can be successfully emulated through the File constructor anyway.
Our nsFormSubmission and subclasses accepted a third filename argument to
explicitly specify the filename. Since switching from nsIDOMBlob to File in Bug
1085283, we can read out the filename directly from the File. This simplifies
the code, but introduces a change in the way Firefox submits form data to
servers.

Consider the code:
var fd = new FormData();
fd.append("blob1", new Blob(["hi"]), ""); // explicit empty filename as third arg
fd.append("file1", new File(["hi"], "")); // File's name is empty, no third arg.
xhr.send(fd);

Previously, the request body had filename="" in the first case, and filename="blob" in the second.
This patch will change it to both cases result in filename=""

This behaviour isn't exactly specced anywhere, nor in the HTML spec [1], nor in
RFC 2388. In addition Blink (at least Chromium v40) has the same behaviour
introduced by this patch. So shipping it seems ok to me.

Boris, is this acceptable?

[1]: http://www.w3.org/html/wg/drafts/html/master/semantics.html#multipart/form-data-encoding-algorithm
Attachment #8575660 - Flags: review?(amarchesini)
Attachment #8575660 - Flags: feedback?(bzbarsky)
Anne pointed out that my interpretation of the spec was wrong. I'm repurposing this bug to simplify the current (correct) implementation a bit.
Summary: FormData entry filename is not respected. → Remove explicit filename field in form submission infrastructure
Making this change allows structured cloning of FormData (required for FormData on workers) to stay simple by not requiring passing around another string.
Comment on attachment 8575660 [details] [diff] [review]
Use File's name instead of explicit file name in form submission related classes

1)  Consider this script:

  var data = new FormData();
  var file = new File(['x'], "myfile");
  data.append("foo", file);
  alert(data.get("foo") === file);

Per spec, this should alert true, I believe, since https://xhr.spec.whatwg.org/#create-an-entry step 3 will do nothing because we have a File and step 4 will do nothing because "filename" is not given.

So I think in CreateNewFileInstance if !aFilename.WasPassed() && aBlob.IsFile(), you just want to return &aBlob.

2) The voidString in nsFormData::GetSendInfo seems to be unused.

3) In nsFSMultipartFormData::AddNameFilePair can the aBlob actually be !IsFile()?  It might be worth documenting how, since FormData can never end up storing a Blob that's not a File internally, right?
Attachment #8575660 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8575660 [details] [diff] [review]
Use File's name instead of explicit file name in form submission related classes

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

Can we have a test for the FormSubmission?

::: dom/base/nsFormData.cpp
@@ +32,5 @@
> +  // value is filename."
> +  nsAutoString filename;
> +  if (aFilename.WasPassed()) {
> +    filename = aFilename.Value();
> +  } else {

} else if(aBlob.IsFile()) {
  aBlob.GetName(filename);
} else {
  filename = NS_LITERAL_STRING("blob");
}

no needs for nested if()

@@ +290,5 @@
>  {
>    nsFSMultipartFormData fs(NS_LITERAL_CSTRING("UTF-8"), nullptr);
>  
> +  nsAutoString voidString;
> +  voidString.SetIsVoid(true);

You are not using it, right?

::: dom/html/nsFormSubmission.cpp
@@ +458,1 @@
>        NS_ENSURE_SUCCESS(rv, rv);

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +458,5 @@
>        NS_ENSURE_SUCCESS(rv, rv);
> +
> +      nsAutoString filepath16;
> +      rv = aBlob->GetPath(filepath16);
> +      NS_ENSURE_SUCCESS(rv, rv);

same here.

@@ +461,5 @@
> +      rv = aBlob->GetPath(filepath16);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      if (!filepath16.IsEmpty()) {
> +        // File.path includes trailing "/"
> +        filename16 = filepath16 + filename16;

Wait... in any OS? What about windows? I still prefer this:

nsCOMPtr<nsIFile> localFile (do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
if (NS_WARN_IF(!localFile)) { ...

rv = localFile->InitWithNativePath(NS_ConvertUTF16toUTF8(filepath16));
if (NS_WARN_IF...

rv = localFile->Append(NS_ConvertUTF16toUTF8(filename));
if (NS_WARN_IF...

rv = localFile->GetPath(filename); // I guess this exists..
if (NS_WARN_IF...
Attachment #8575660 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/ddd0d3a6a1ae
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: