Closed
Bug 1250148
Opened 8 years ago
Closed 8 years ago
Ensure that all the formdata regressions are fixed
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: bzbarsky, Assigned: baku)
References
Details
Attachments
(1 file, 2 obsolete files)
24.64 KB,
patch
|
smaug
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
[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•8 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 1•8 years ago
|
||
I want to see if it's fully green on try first. Then I want to add a test.
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8722002 -
Attachment is obsolete: true
Attachment #8722074 -
Flags: review?(bugs)
Comment 3•8 years ago
|
||
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?
Updated•8 years ago
|
Attachment #8722074 -
Flags: review?(bugs) → review-
Comment 4•8 years ago
|
||
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•8 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•8 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?
Comment 7•8 years ago
|
||
(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•8 years ago
|
||
Sorry, I confused FF45 with current FF44. Read the previous comment applying: s/FF45/FF44/g
Comment 9•8 years ago
|
||
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•8 years ago
|
||
Attachment #8722074 -
Attachment is obsolete: true
Attachment #8722531 -
Flags: review?(bugs)
Comment 11•8 years ago
|
||
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•8 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?
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
Flags: needinfo?(amarchesini)
Comment 15•8 years ago
|
||
... and also https://treeherder.mozilla.org/logviewer.html#?job_id=22197422&repo=mozilla-inbound
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e176668613b9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(amarchesini)
Marking affected & tracking for aurora.
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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/ece4dcb28433
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•