Ensure that all the formdata regressions are fixed

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bzbarsky, Assigned: baku)

Tracking

Trunk
mozilla47
Points:
---

Firefox Tracking Flags

(firefox46+ fixed, firefox47+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

[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?
Flags: needinfo?(tdaede)
(Assignee)

Updated

3 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 1

3 years ago
Created attachment 8722002 [details] [diff] [review]
form.patch

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

Comment 2

3 years ago
Created attachment 8722074 [details] [diff] [review]
form.patch
Attachment #8722002 - Attachment is obsolete: true
Attachment #8722074 - Flags: review?(bugs)
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?
Attachment #8722074 - Flags: review?(bugs) → review-
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.
Flags: needinfo?(tdaede)
(Assignee)

Comment 5

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

Comment 6

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

Comment 8

3 years ago
Sorry, I confused FF45 with current FF44. Read the previous comment applying: s/FF45/FF44/g
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.
(Assignee)

Comment 10

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

Comment 13

3 years ago
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
Attachment #8722531 - Flags: approval-mozilla-aurora?

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e176668613b9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Updated

3 years ago
Flags: needinfo?(amarchesini)
Marking affected & tracking for aurora.
status-firefox46: --- → affected
tracking-firefox46: ? → +
tracking-firefox47: ? → +
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.
Attachment #8722531 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 20

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/ece4dcb28433
status-firefox46: affected → fixed
You need to log in before you can comment on or make changes to this bug.