1162658 - Update FormData to match latest spec

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: fd.patch

Probably I have to split the web-platform-test changes in a separate patch.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: fd.patch

1 test was still orange on try.

Comment 3

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

Comment on attachment 8603406 [details] [diff] [review]
fd.patch

Why do we want to make these backwards incompatible changes?
I'm especially worried about dropping void append(USVString name, Blob value, optional USVString filename); given that at least based on the source code
both webkit and blink support that useful looking method.

Comment 4

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

Comment on attachment 8603406 [details] [diff] [review]
fd.patch

Oh, those changes are very new to the spec.
https://github.com/whatwg/xhr/commit/f9d4c255cb11ee6e80a310f5463a6b601c90376f
Anyhow, I think we shouldn't remove support for filename. It has been there for ages, since Bug 690659.

Comment 5

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

I think if File is passed as the second param, and third param isn't passed, then filename should be
file.name. But why to drop support for the 3rd param?

Comment 6

[Anne (:annevk)]

So that we don't change identity of the objects you pass in. And arguably if you care about changing the filename you should use new File().

I made these changes based on sicking's suggestions.

Comment 7

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

I'm not saying the new API is bad, but the change itself isn't backwards compatible.
And I see no strong reasons for the breakage.

Comment 8

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

Simplicity is always a good thing. Especially since the patterns used in FormData is being copied to other APIs, so simplicity here is even more valuable.

Comment 9

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

As I said, I'm not talking about the new version of the API being bad in anyway, but breaking the old
version, which has been there for years... And supporting the old version isn't particularly difficult and I can see blob + filename being even rather useful.

Comment 10

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

At least before the change we should get some data whether blob + filename is being used.
Randomly changing years old APIs feels really weird.

Comment 11

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

MDN for instance has examples of using the 3rd param
https://developer.mozilla.org/en-US/docs/Web/Guide/Using_FormData_Objects
And also other documentation for web devs have examples of using the 3rd param.
Sounds rather risky to remove it without knowing how large % of the append(name, blob, ...) calls
also have the 3rd param used. Need some telemetry here.

Comment 12

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

(In reply to Olli Pettay [:smaug] from comment #11)
> And also other documentation for web devs have examples of using the 3rd
> param.
I mean non-MDN sites.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I agree with Olli, I think.  Effectively backing out bug 1127150 (not quite, but more or less)is one thing.  But changing the spec to no longer work like it used to when a third arg is passed is too likely to break web pages.  At the very least we'd need a use counter to see whether people are in fact passing that third arg.  We've supported that third arg, at least for append() since Firefox 22, so going on two years now.

Jonas, what makes you think this change is compatible enough?

Comment 14

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

It's a good point that removing the 'filename' argument might not be web compatible any longer.

So maybe we can treat |fd.append(name, blob, filename)| as |fd.append(name, new File([blob], filename, { type: blob.type }))|.

I still would really like to stop wrapping all Blobs into a File and thereby loose identity though. That seems less likely to break any pages given that we only very recently made that detectable through the enumeration API.

Comment 15

[Boris Zbarsky [:bzbarsky]]

That seems like a reasonable compromise to me.

Comment 16

[Andrea Marchesini [:baku]]

annevk, can you update the spec as :sicking is suggesting in comment 14?

Comment 17

[Anne (:annevk)]

Currently when we map FormData to entries those can have a filename associated with them. Rather than changing objects, perhaps we should simply store the passed in filename along with the entry? I'd still much rather not have the filename argument at all though...

Comment 18

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

Adding syntax sugar is usually simpler than changing data models.

Would the page be able to get to the associated filename if we just store the filename rather than create a File? I.e. what would enumeration return?

Comment 19

[Anne (:annevk)]

It would not be visible there I guess. So yeah, that would argue for changing the object if we wanted to expose that.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: fd.patch

Comment 21

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

Comment on attachment 8607558 [details] [diff] [review]
fd.patch

># HG changeset patch
># Parent db7d6032f62a25c24b7b3ffac710a2cfdc155399
># User Andrea Marchesini <[e-mail]>
>
>diff --git a/dom/base/nsFormData.cpp b/dom/base/nsFormData.cpp
>--- a/dom/base/nsFormData.cpp
>+++ b/dom/base/nsFormData.cpp
>@@ -18,65 +18,49 @@ using namespace mozilla::dom;
> nsFormData::nsFormData(nsISupports* aOwner)
>   : nsFormSubmission(NS_LITERAL_CSTRING("UTF-8"), nullptr)
>   , mOwner(aOwner)
> {
> }
> 
> namespace {
> 
>-// Implements steps 3 and 4 of the "create an entry" algorithm of FormData.
>-already_AddRefed<File>
>-CreateNewFileInstance(Blob& aBlob, const Optional<nsAString>& aFilename)
>+already_AddRefed<Blob>
>+MaybeWriteBlobInFile(Blob& aBlob, const Optional<nsAString>& aFilename)
> {
>-  // Step 3 "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"."
>-  // Step 4 "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."
>-  nsAutoString filename;
>-  if (aFilename.WasPassed()) {
>-    filename = aFilename.Value();
>-  } else {
>-    // If value is already a File and filename is not passed, the spec says not
>-    // to create a new instance.
>-    nsRefPtr<File> file = aBlob.ToFile();
>-    if (file) {
>-      return file.forget();
>-    }
>-
>-    filename = NS_LITERAL_STRING("blob");
>+  if (!aFilename.WasPassed()) {
>+    nsRefPtr<Blob> blob = &aBlob;
>+    return blob.forget();
>   }
> 
>-  return aBlob.ToFile(filename);
>+  return aBlob.ToFile(aFilename.Value());
> }
Don't we want to still keep "blob" as the default filename for Blobs?

in other words, I think we don't want to see any changes to formData_test.js
(only possibly some new tests), because that means backwards incompatible change.

Comment 22

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

Since I'm not too happy with breaking backwards compatibility, I think we should change
the spec so that filename is stored with the data when needed, and iteration/get would return
a dictionary containing that data + filename (and filename would have been implicitly assigned to
"blob" in case of non-File-Blobs).

Comment 23

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

Why is that more backwards compatible?

If we just make sure to use the name "blob" for the case when the stored Blob isn't a File, then it seems like the only thing changing here is enumeration. Which is also changed if we return a dictionary.

Comment 24

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

I didn't say that is more backward compatible, but would let the caller of get() or iterator to actually know what will be sent to server.
But I don't care strongly, as long as the current API still keeps working as it has worked for years.
New methods can do whatever.

Comment 25

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

I think having the enumeration return exactly what the user set, rather than a dictionary, offers more benefits. The only downside is that devs have to remember that "blob" is the default name, but that only matters if they don't explicitly set a name, yet care what name is actually submitted.

Comment 26

[Andrea Marchesini [:baku]]

Attachment: fd.patch

Comment 27

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

Comment on attachment 8608758 [details] [diff] [review]
fd.patch

Why only one of AddNameBlobPair implementations use "blob"? That is not what happens currently.

And doesn't use of MaybeWriteBlobInFile change the identity if
Blob + filename is used? I mean, get() start to return different object and I
thought sicking wanted to not change identity.

(please reask review if I misunderstood something here.)

Comment 28

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

See comment 14. I think it's fine to change the identity when a filename is explicitly provided.

Comment 29

[Andrea Marchesini [:baku]]

Attachment: fd.patch

'blob' supported also by the other encoders.

Comment 30

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

(In reply to Jonas Sicking (:sicking) from comment #28)
> See comment 14. I think it's fine to change the identity when a filename is
> explicitly provided.

I don't quite see why that is needed. Why we need to create File object at all in that case?
I would expect us to internally store triplet(name, data, filename?)
where data is Blob or USVString. And get(name) would just return data.

Comment 31

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

That makes the datamodel much more complex.

It means that the enumeration needs to expose the { value, filename? } pair, rather than just expose a value which is either a string/blob/file(/directory in the future), we now need to expose a dictionary.

That makes the API more complex and also means bigger changes to the spec.

The third argument is obsolete and should be deprecated anyway. It was a workaround to the fact that we didn't have a File constructor.

I guess I also don't see the advantage, but maybe I'm missing it?

Comment 32

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

Why would iteration have to expose a dictionary. (I was suggesting that sure, because it might be nice API, but I don't see any reason for "have to").

To me storing the triplet internally makes the API simpler. No special casing Blob -> create File.
Always just store the triplet.

Comment 33

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

So are you suggesting that the defined filename would not be exposed during enumeration? That seems worse than the syntax sugar in comment 14. I don't agree that that makes the API simpler.

Comment 34

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

well, the best option would be to reveal both data and filename, So comment 22.

Comment 35

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

Which seems more complicated to me.

But I'm spending too much time on this bug. I think the patch as it stands is the best solution, but I'll let the rest of you fight it out.

Comment 36

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

I think I can live with the behavior the patch has. Weird API still.

Comment 37

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

looks like blink has blob->file conversion for iteration

Comment 38

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

So I think Blob should always wrapped into File, and if filename isn't passed "blob" should be used.
That way iteration/get doesn't need to deal with both Files and Blobs

Comment 39

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

Comment on attachment 8609003 [details] [diff] [review]
fd.patch

So why Blob without filename doesn't get wrapped, but with filename does get.
I think always create a new File for Blob, and never new File if a File was already passed. That makes the API at least a bit more consistent.

Comment 40

[Andrea Marchesini [:baku]]

> So why Blob without filename doesn't get wrapped, but with filename does get.
> I think always create a new File for Blob, and never new File if a File was
> already passed. That makes the API at least a bit more consistent.

What you are proposing is exactly what the existing code does.

Comment 41

[Andrea Marchesini [:baku]]

I'm lost. What do we want to do with this bug? annevk, bz?

Comment 42

[Boris Zbarsky [:bzbarsky]]

Anne is on vacation and I haven't been following this very closely.... :(   I thought we had a plan.  Who at this point is objecting to it and what are they asking for instead?

Comment 43

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

I objected whatever the plan was when this bug was filed. It would have broken backwards compatibility of a rather old API. And comment 39 felt the most natural behavior to me (and apparently that is what the old behavior was in the spec). So it is not clear to me why any API change is needed.
But I feel like I'm missing some background information, since I don't know what lead to the spec change.

Comment 44

[Philip Jägenstedt [:foolip]]

I came across this difference between the spec and Blink's IDL and was pointed here by Anne in https://github.com/whatwg/xhr/commit/f9d4c255cb11ee6e80a310f5463a6b601c90376f#commitcomment-11582009

Is the core concern here that removing the filename argument could break too much content? If it would help inform the discussion, I could add a use counter for it in Blink.

Also, can someone summarize the proposal for making this strictly backwards compatible? Is it just to revert the spec change and leave it alone?

Comment 45

[Anne (:annevk)]

The API was changed so you could store a Blob in a FormData without that Blob being turned into a File. Turning it more into an actual map API that it was supposed to be. The filename argument was removed since that only existed because File did not have a constructor, which it does now.

(This came up again due to the changes discussed to the <input type=file> API, where we'd like to behave similarly.)

I would like to leave the specification as is. If that is really too problematic I suppose we could create a new File object only when filename is provided (and otherwise leave Blob and File objects as-is). Use counters would be appreciated Philip!

Comment 46

[Philip Jägenstedt [:foolip]]

Precisely what code path of the old model is it that needs to be measured? Is it when you pass in a Blob with a filename, or a File with a filename that doesn't match File.name? It would be easy to just measure how often the optional filename argument is used, but is there more to it?

The weirdness I guess you want to get rid of is where FormData.get converts the internal Blob or File to a new File if there was a filename?

Comment 47

[Anne (:annevk)]

That is indeed what I'd like to get rid of (although it doesn't happen on the get() side). I guess the interesting cases are:

* You pass in a Blob
* You pass in a Blob and a filename
* You pass in a File and a filename

Comment 48

[Philip Jägenstedt [:foolip]]

In Blink the wrapping actually does happen in FormData.get(). It always returns a File, so whenever you pass in a Blob or if you provide a filename, get() will wrap the object in a new File, such that fd.get(name)!==fd.get(name) and also is not the same object you passed in. Pretty weird, huh? Fortunately all of this is behind a flag, only FormData.append() has shipped.

I'll add use counters for the cases you mention. It seems like if the filename argument is needed for compat, creating a new File in append() would be the least weird.

Comment 49

[Philip Jägenstedt [:foolip]]

Adding use counters and tracking this issue in https://code.google.com/p/chromium/issues/detail?id=498790

Comment 50

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

I would backout the spec change, and only after we have some data about the API usage, possibly change
it. Keeping the spec as it is now may lead changes in other browser engines and then they may just need to back the changes out if it isn't web compatible.

(There are plenty of examples in the web where the filename parameter is used, so I would be surprised if we could remove it easily.)

Comment 51

[Andrea Marchesini [:baku]]

Not working on this.

Comment 52

[Philip Jägenstedt [:foolip]]

The use counters for this have now reached Chrome stable:
https://www.chromestatus.com/metrics/feature/timeline/popularity/839
https://www.chromestatus.com/metrics/feature/timeline/popularity/840
https://www.chromestatus.com/metrics/feature/timeline/popularity/841
https://www.chromestatus.com/metrics/feature/timeline/popularity/842
https://www.chromestatus.com/metrics/feature/timeline/popularity/843

At these usage levels, it's not conclusive for me what we should do. If anything important were at stake, the filename argument might be possible to remove at these levels, but usage isn't low enough that I think we should make changes just because it seems nicer.

I would suggest, then, to keep an optional filename argument and handle it as per comment #14. If no filename is given, we don't wrap the object in a new object.

Comment 53

[Philip Jägenstedt [:foolip]]

Spec bug: https://github.com/whatwg/xhr/issues/25

Comment 54

[Andrea Marchesini [:baku]]

Attachment: formData2.patch

Probably I have to submit a new patch fixing some broken test in WPT.

Comment 55

[Andrea Marchesini [:baku]]

Attachment: formData2.patch

Comment 56

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

Since browsers implement 3 param variant of set(), I filed https://github.com/whatwg/xhr/issues/41
It is unclear whether the spec is web compatible.

Comment 57

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

Comment on attachment 8705596 [details] [diff] [review]
formData2.patch

So parts of the ::Set changes need to be undone

Comment 58

[Andrea Marchesini [:baku]]

Attachment: formData2.patch

Comment 59

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

Comment on attachment 8705974 [details] [diff] [review]
formData2.patch

EmptyBlobImpl would sound a bit better to my ears.

CreateNewFileInstance should be renamed, since it doesn't always create anything, but just returns the aBlob.
Perhaps GetBlobForFormDataStorage or some such?

+  RefPtr<File> file = aBlob.ToFile(aFilename.Value(), aRv);
Why does that work? Doesn't ToFile return non-null only when dealing with
file based blob impls?

Comment 60

[Andrea Marchesini [:baku]]

> +  RefPtr<File> file = aBlob.ToFile(aFilename.Value(), aRv);
> Why does that work? Doesn't ToFile return non-null only when dealing with
> file based blob impls?

Yeah, I know. I'm thinking about how to change this and make the interface File and Directory less confusing.
But this will be a follow up.

Comment 61

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

well, the issue that if that isn't fixed, after this patch we'll be in an odd state where the API says 
.append and .set take any Blobs as params, but actually can't handle non-files.
Could we at least keep typedef (File or USVString) FormDataEntryValue; until 
 RefPtr<File> file = aBlob.ToFile(aFilename.Value(), aRv); handling is fixed?

Comment 62

[Andrea Marchesini [:baku]]

> +  RefPtr<File> file = aBlob.ToFile(aFilename.Value(), aRv);
> Why does that work? Doesn't ToFile return non-null only when dealing with
> file based blob impls?

No. If you set a name, we transform a blob in a file. The current setup should work correctly.

Comment 63

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

Comment on attachment 8705974 [details] [diff] [review]
formData2.patch

ok, ToFile() deals with files only, ToFile(param1, param2); can actually create a File.


"blob" as filename is special cased now only for nsFSMultipartFormData::AddNameBlobPair, other 
AddNameBlobPair implementations don't seem to have anything like that.



Also the 2 first issues from comment 59 are still valid.

Comment 64

[Andrea Marchesini [:baku]]

Attachment: formData2.patch

Comment 65

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

Comment on attachment 8706685 [details] [diff] [review]
formData2.patch

>+already_AddRefed<Blob>
>+GetBlobForFormDataStorage(Blob& aBlob, const Optional<nsAString>& aFilename,
>+                           ErrorResult& aRv)
nit, extra space before ErrorResult

>+nsFSURLEncoded::AddNameBlobPair(const nsAString& aName,
>+                                Blob* aBlob)
...
>+  RefPtr<File> file = aBlob->ToFile();
>+  if (file) {
>+    file->GetName(filename);
>+  } else {
>+    filename.AssignLiteral("blob");
>+  }


>+nsFSMultipartFormData::AddNameBlobPair(const nsAString& aName,
>+                                       Blob* aBlob)
...
>+  RefPtr<File> file = aBlob->ToFile();
>+  if (file) {
>+    file->GetName(filename16);
>+  } else {
>+    filename16.AssignLiteral("blob");
>+  }

>+nsFSTextPlain::AddNameBlobPair(const nsAString& aName,
>+                               Blob* aBlob)
...
>+  
>+  RefPtr<File> file = aBlob->ToFile();
>+  if (file) {
>+    file->GetName(filename);
>+  } else {
>+    filename.AssignLiteral("blob");
>+  }
I'm not happy with this code duplication, especially given that the previous patch already missed
having "blob" in two cases.
I do think we need tests covering each of these cases and having some helper method which keeps this "blob"
handling in one place.
static method which takes Blob& as a param and returns a string or has nsAString& as out param or something.




>   /**
>    * Submit a name/file pair
>    *
>    * @param aName the name of the parameter
>-   * @param aFile the file to submit. The file's name will be used
>+   * @param aBlob the file to submit. The file's name will be used
well, aBlob isn't necessarily a file anymore. some tweak to the comment please

Comment 66

[Andrea Marchesini [:baku]]

Attachment: formData2.patch

Comment 67

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

Comment on attachment 8707336 [details] [diff] [review]
formData2.patch

I still think we need some tests for checking that blobs in various form submission encoding.

Comment 68

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

We realized on IRC that form submission part of the patch need to be updated to follow the HTML spec.
(there is a minor inconsistency there vs FormData API)

Comment 69

[Andrea Marchesini [:baku]]

Attachment: interdiff

This interdiff makes tests and spec happy again.

Comment 70

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

Comment on attachment 8709894 [details] [diff] [review]
interdiff

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  5120a603f84a532a31922fc343b1db24d8033071
>
>diff --git a/dom/base/FormData.cpp b/dom/base/FormData.cpp
>--- a/dom/base/FormData.cpp
>+++ b/dom/base/FormData.cpp
>@@ -338,17 +338,24 @@ FormData::Constructor(const GlobalObject
> NS_IMETHODIMP
> FormData::GetSendInfo(nsIInputStream** aBody, uint64_t* aContentLength,
>                       nsACString& aContentType, nsACString& aCharset)
> {
>   nsFSMultipartFormData fs(NS_LITERAL_CSTRING("UTF-8"), nullptr);
> 
>   for (uint32_t i = 0; i < mFormData.Length(); ++i) {
>     if (mFormData[i].value.IsBlob()) {
>-      fs.AddNameBlobPair(mFormData[i].name, mFormData[i].value.GetAsBlob());
>+      ErrorResult rv;
>+      RefPtr<File> file =
>+        mFormData[i].value.GetAsBlob()->ToFile(NS_LITERAL_STRING("blob"), rv);
>+      if (NS_WARN_IF(rv.Failed())) {
>+        return rv.StealNSResult();
>+      }
>+
>+      fs.AddNameBlobPair(mFormData[i].name, file);

I don't understand this change. We end up replacing all the files with new files with filename "blob"?


>@@ -57,18 +57,16 @@ SendJSWarning(nsIDocument* aDocument,
> static void
> RetrieveFileNameOrBlob(Blob* aBlob, nsAString& aFilename)
> {
>   MOZ_ASSERT(aBlob);
> 
>   RefPtr<File> file = aBlob->ToFile();
>   if (file) {
>     file->GetName(aFilename);
>-  } else {
>-    aFilename.AssignLiteral("blob");
>   }
> }
so this method has now nothing to do with Blob, so drop OrBlob from the name?

Comment 71

[Andrea Marchesini [:baku]]

Attachment: id.patch

Comment 72

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

Comment on attachment 8709975 [details] [diff] [review]
id.patch

I would drop OrBlob from RetrieveFileNameOrBlob now that it isn't about blob anymore.

Could you add a comment to ReadFormData that Blob::Create creates File or Blob based on the type of blobImpl

Comment 73

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1079f6d91f65

Comment 74

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1079f6d91f65

Comment 75

[none11given]

> I don't understand this change. We end up replacing all the files with new
> files with filename "blob"?

It seems this wasn't changed in the final patch and now that behavior has landed in my Firefox Dev Edition, breaking some websites' multipart forms when submitting with an empty file input. Because |filename="blob"| is sent even when the input is empty, some servers are attempting to process the file they think exists and then failing.

End of form data with emtpy file
In FF44:

> Content-Disposition: form-data; name="upfile"; filename=""
> Content-Type: application/octet-stream
> 
> 
> -----------------------------481544977694--

In FF46:

> Content-Disposition: form-data; name="upfile"; filename="blob"
> Content-Type: application/octet-stream
> 
> 
> -----------------------------320453973040--

Please revert this change. I'll open a new bug if needed.

Comment 76

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

See bug 1241171