1250148 - Ensure that all the formdata regressions are fixed

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Web compat regression

Per bug 1241171 comment 25.  Filing a separate bug so we actually track sanely for Aurora...

Thomas, a link to a specific testcase demonstrating bug 1241171 comment 22 would really help.  Or does bug 1241171 comment 25 describe the problem?

Comment 1

[Andrea Marchesini [:baku]]

Attachment: form.patch

I want to see if it's fully green on try first. Then I want to add a test.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: form.patch

Comment 3

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

var f = document.createElement("form"); f.innerHTML = "<input type=file name=test>"; var fd = new FormData(f); fd.get("test");
(run in the web console) gives different behavior with the patch than what we have on beta.
And since with that script we end up getting magical File (with filename="blob") out from form + formdata, it sure smells like a variant of bug 1241171.
Can we please revert back to the beta/FF45 behavior and add more tests.
We've had so many regressions in this area that I'm not ready to have Aurora as a playground for new behavior.
So, first make Nightly and Aurora to behave as beta, and then perhaps sanitize behavior on Nightly only?

Comment 4

[Thomas Daede [:TD-Linux]]

I was reproducing the problem by submitting a POST, then looking at the Network tab in devtools to see the request. But the new FormData() method in comment 3 looks to show the same bug - an empty input type="file" having filename="blob".

The most prominent website this breaks is 4chan, e.g. go to https://boards.4chan.org/g/ (note: board is technically sfw, but it is 4chan) and try to post a reply to a thread without a file attached to the reply, it will fail due to the site rejecting 0 length files. I suspect the breakage on other sites is more subtle, as they may happily accept a 0 byte file named "blob", which the user might not notice.

Comment 5

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #3)
> var f = document.createElement("form"); f.innerHTML = "<input type=file
> name=test>"; var fd = new FormData(f); fd.get("test");
> (run in the web console) gives different behavior with the patch than what
> we have on beta.

But what we have in beta is different than what we have in production.
With this patch we go back to production.

Comment 6

[Andrea Marchesini [:baku]]

With this:

|var f = document.createElement("form"); f.innerHTML = "<input type=file name=test>"; var fd = new FormData(f); fd.get("test");| 

here the behavior we have:

* beta -> result is empty string. This breaks 4chan.
* FF45 -> result is null. This is because at that time we support nullptr blobs in StringOrBlob class. This is not anymore.
* This patch -> result is a blob.

With this:

|var f = document.createElement("form"); f.setAttribute("enctype", "multipart/form-data"); f.innerHTML = "<input type=file name=test>"; var fd = new FormData(f); var xhr = new XMLHttpRequest(); xhr.open("POST", window.location.href, true); xhr.send(fd);| 

* beta -> Content-Disposition: form-data; name="test"
* FF45 -> Content-Disposition: form-data; name="test"; filename=""\nContent-Type: application/octet-stream
* This patch -> Content-Disposition: form-data; name="test"; filename=""\nContent-Type: application/octet-stream

So I think that this patch is good enough because for HTML submission it has the correct behaviour and not the broken one as beta has.

I don't really want to support the first test because if get() works fine returning undefined, what about getAll()?
Should we return a sequence with 1 element that is undefined? Or we should skip that item completely and return an empty sequence?

Comment 7

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

(In reply to Andrea Marchesini (:baku) from comment #6)
> * beta -> result is empty string. This breaks 4chan.
> * FF45 -> result is null. This is because at that time we support nullptr
> blobs in StringOrBlob class. This is not anymore.
I don't understand this. beta is FF45

Comment 8

[Andrea Marchesini [:baku]]

Sorry, I confused FF45 with current FF44. Read the previous comment applying: s/FF45/FF44/g

Comment 9

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

Well, I don't understand "* beta -> result is empty string. This breaks 4chan." then.
I'm not aware of us having any bug open to have regressions in beta (== FF45) related to this stuff.
If we have some regression (== web sites broken) in beta, that is the first thing to fix, asap.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: form.patch

Comment 11

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

Comment on attachment 8722531 [details] [diff] [review]
form.patch

>+      fileStream = bufferedStream;
>+    }
>+  } else {
>+      contentType.AssignLiteral("application/octet-stream");
Some extra spaces before contentType



>-  virtual nsresult AddNameBlobPair(const nsAString& aName,
>-                                   mozilla::dom::Blob* aBlob) = 0;
>+  virtual nsresult AddNameBlobOrNullPair(const nsAString& aName,
>+                                          mozilla::dom::Blob* aBlob) = 0;
nit, align the latter param

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74f968e36041

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8722531 [details] [diff] [review]
form.patch

Approval Request Comment
[Feature/regressing bug #]: multiple bugs for several FormData spec changes.
[User impact if declined]: web incompatibility
[Describe test coverage new/current, TreeHerder]: test included
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2230fd20b8 for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=22193616&repo=mozilla-inbound

Comment 15

[David Baron :dbaron:]

... and also https://treeherder.mozilla.org/logviewer.html#?job_id=22197422&repo=mozilla-inbound

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e176668613b9

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e176668613b9

Comment 18

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Marking affected & tracking for aurora.

Comment 19

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8722531 [details] [diff] [review]
form.patch

This is a pretty big patch, but some of it is tests. 
Seems ok in m-c so far, let's uplift in hopes this fixes some web compat issues.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ece4dcb28433