Update FormData to match latest spec

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

({dev-doc-needed})

Trunk
mozilla46
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox40 affected, firefox46 fixed)

Details

Attachments

(2 attachments, 10 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8603369 [details] [diff] [review]
fd.patch

Probably I have to split the web-platform-test changes in a separate patch.
Attachment #8603369 - Flags: review?(bugs)
(Assignee)

Comment 2

3 years ago
Created attachment 8603406 [details] [diff] [review]
fd.patch

1 test was still orange on try.
Attachment #8603369 - Attachment is obsolete: true
Attachment #8603369 - Flags: review?(bugs)
Attachment #8603406 - Flags: review?(bugs)

Comment 3

3 years ago
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.
Attachment #8603406 - Flags: review?(bugs) → feedback?(annevk)

Comment 4

3 years ago
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.
Attachment #8603406 - Flags: review-

Comment 5

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

3 years ago
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.
Keywords: dev-doc-needed

Comment 7

3 years ago
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.
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

3 years ago
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.
At least before the change we should get some data whether blob + filename is being used.
Randomly changing years old APIs feels really weird.

Updated

3 years ago
Attachment #8603406 - Flags: feedback?(annevk) → feedback+
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.
(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.
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?
Flags: needinfo?(jonas)
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.
Flags: needinfo?(jonas)
That seems like a reasonable compromise to me.
(Assignee)

Comment 16

3 years ago
annevk, can you update the spec as :sicking is suggesting in comment 14?
Flags: needinfo?(annevk)

Comment 17

3 years ago
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...
Flags: needinfo?(annevk)
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

3 years ago
It would not be visible there I guess. So yeah, that would argue for changing the object if we wanted to expose that.
(Assignee)

Comment 20

3 years ago
Created attachment 8607558 [details] [diff] [review]
fd.patch
Attachment #8603406 - Attachment is obsolete: true
Attachment #8607558 - Flags: review?(bugs)
Comment on attachment 8607558 [details] [diff] [review]
fd.patch

># HG changeset patch
># Parent db7d6032f62a25c24b7b3ffac710a2cfdc155399
># User Andrea Marchesini <amarchesini@mozilla.com>
>
>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.
Attachment #8607558 - Flags: review?(bugs) → review-
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).
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.
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.
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.
(Assignee)

Comment 26

3 years ago
Created attachment 8608758 [details] [diff] [review]
fd.patch
Attachment #8607558 - Attachment is obsolete: true
Attachment #8608758 - Flags: review?(bugs)
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.)
Attachment #8608758 - Flags: review?(bugs) → review-
See comment 14. I think it's fine to change the identity when a filename is explicitly provided.
(Assignee)

Comment 29

3 years ago
Created attachment 8609003 [details] [diff] [review]
fd.patch

'blob' supported also by the other encoders.
Attachment #8608758 - Attachment is obsolete: true
Attachment #8609003 - Flags: review?(bugs)
(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.
Flags: needinfo?(jonas)
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?
Flags: needinfo?(jonas)
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.
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.
well, the best option would be to reveal both data and filename, So comment 22.
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.
I think I can live with the behavior the patch has. Weird API still.
looks like blink has blob->file conversion for iteration
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 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.
Attachment #8609003 - Flags: review?(bugs) → review-
(Assignee)

Comment 40

3 years ago
> 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.
(Assignee)

Comment 41

3 years ago
I'm lost. What do we want to do with this bug? annevk, bz?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(annevk)
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?
Flags: needinfo?(bzbarsky)
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

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

3 years ago
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!
Flags: needinfo?(annevk)

Comment 46

3 years ago
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?
Flags: needinfo?(annevk)

Comment 47

3 years ago
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
Flags: needinfo?(annevk)

Comment 48

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

3 years ago
Adding use counters and tracking this issue in https://code.google.com/p/chromium/issues/detail?id=498790
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.)
(Assignee)

Comment 51

3 years ago
Not working on this.
Assignee: amarchesini → nobody

Comment 52

3 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1237674
(Assignee)

Comment 54

2 years ago
Created attachment 8705575 [details] [diff] [review]
formData2.patch

Probably I have to submit a new patch fixing some broken test in WPT.
Attachment #8609003 - Attachment is obsolete: true
Attachment #8705575 - Flags: review?(bugs)
(Assignee)

Comment 55

2 years ago
Created attachment 8705596 [details] [diff] [review]
formData2.patch
Assignee: nobody → amarchesini
Attachment #8705575 - Attachment is obsolete: true
Attachment #8705575 - Flags: review?(bugs)
Attachment #8705596 - Flags: review?(bugs)
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 on attachment 8705596 [details] [diff] [review]
formData2.patch

So parts of the ::Set changes need to be undone
Attachment #8705596 - Flags: review?(bugs)
(Assignee)

Comment 58

2 years ago
Created attachment 8705974 [details] [diff] [review]
formData2.patch
Attachment #8705596 - Attachment is obsolete: true
Attachment #8705974 - Flags: review?(bugs)
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?
(Assignee)

Comment 60

2 years ago
> +  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.
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?
(Assignee)

Comment 62

2 years ago
> +  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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Attachment #8705974 - Flags: review?(bugs) → review-
(Assignee)

Comment 64

2 years ago
Created attachment 8706685 [details] [diff] [review]
formData2.patch
Attachment #8705974 - Attachment is obsolete: true
Attachment #8706685 - Flags: review?(bugs)
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
Attachment #8706685 - Flags: review?(bugs) → review-
(Assignee)

Comment 66

2 years ago
Created attachment 8707336 [details] [diff] [review]
formData2.patch
Attachment #8706685 - Attachment is obsolete: true
Attachment #8707336 - Flags: review?(bugs)
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.
Attachment #8707336 - Flags: review?(bugs) → review+
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)
(Assignee)

Comment 69

2 years ago
Created attachment 8709894 [details] [diff] [review]
interdiff

This interdiff makes tests and spec happy again.
Attachment #8709894 - Flags: review?(bugs)
Comment on attachment 8709894 [details] [diff] [review]
interdiff

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># 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?
Attachment #8709894 - Flags: review?(bugs) → review-
(Assignee)

Comment 71

2 years ago
Created attachment 8709975 [details] [diff] [review]
id.patch
Attachment #8709894 - Attachment is obsolete: true
Attachment #8709975 - Flags: review?(bugs)
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
Attachment #8709975 - Flags: review?(bugs) → review+

Comment 74

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1079f6d91f65
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Comment 75

2 years ago
> 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.
Flags: needinfo?(bugs)
See bug 1241171
Flags: needinfo?(bugs)
You need to log in before you can comment on or make changes to this bug.