Closed Bug 1187157 Opened 4 years ago Closed 4 years ago

new FormData(HTMLFormElement).getAll(…) or .get(…) crashes Firefox if the key passed to get/getAll belongs to an HTMLInputElement of type="file" (mozilla::dom::GetOrCreateDOMReflectorHelper<T>::GetOrCreate)

Categories

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

39 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox43 + wontfix
firefox44 - wontfix
firefox45 + fixed
firefox46 + fixed

People

(Reporter: sebastian.simon1602, Assigned: baku)

References

Details

Attachments

(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150629114917

Steps to reproduce:

<form id="a">
  <input name="b" type="file"/>
</form>
<script>
  new FormData(document.getElementById('a')).getAll('b');
</script>


Actual results:

This crashes Firefox. The same applies to .get('b');

https://crash-stats.mozilla.com/report/index/1e6276b9-2e8c-44d6-950e-41c5f2150724


Expected results:

It should have returned the value of the file input (or an Array), according to the spec and it should not have crashed.
Duplicate of this bug: 1187160
This specifically seems to occur when no file has been specified for the form input yet.
Confirmed to occur with the current Nightly.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: Firefox → Core
Target Milestone: --- → mozilla41
Firefox 64 bits 43.0.1 Windows

Works fine even if file input is empty.

But crashes with:
- form embedded in a jQuery dialog
- AND file input is empty.
Component: Untriaged → DOM
Can't reproduce on nightly, aurora or beta, but can on release.
Would be great to get un-regression range to figure out what has fixed this.

https://crash-stats.mozilla.com/report/index/dc4ae136-de60-4798-b215-d4ee72160104 is what I got, and
looks like a null+offset crash deep in bindings code (it is accessing null wrapper cache which then checks for some flags offset from null)
Has Regression Range: --- → no
So on trunk this is throwing an uncatchable exception when I do get('b').

That's happening because the mType of the union is eUninitialized when we land in OwningFileOrUSVString::ToJSVal, which causes it to return false without setting an exception on the JSContext.

This has been the ToJSVal behavior for the eUninitialized case for a good long while, so if something changed it changed in the way the union is set up in the formdata/form code.

But the behavior is clearly bogus, right?  The value should be empty string, per https://html.spec.whatwg.org/multipage/forms.html#constructing-form-data-set step 7, and there should be no exception.
Flags: needinfo?(amarchesini)
So what happens here is that we end up in http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFormData.cpp#180 with null aFile, called from HTMLInputElement::SubmitNamesValues in the mType == NS_FORM_INPUT_FILE case when files.IsEmpty().

This calls SetNameFilePair, which does:

67     aData->name = aName;
68     if (aFile) {
69       aData->value.SetAsFile() = aFile;
70     }

and otherwise leaves aData->value uninitialized.  That null-check was basically added in bug 1127703; see <http://hg.mozilla.org/mozilla-central/rev/512b90a0f35f>.  My apologies for not realizing the implications of it not being present before.  :(

So on trunk the right fix is to add an else clause setting aData->value to an empty string.  On branches the right fix is to add a null check and set valueIsFile to false if aFile is null, while setting aData->stringValue to empty string....

Andrea, do you have time to write this up, with tests?
And in particular, the uncatchable exception behavior here is pretty bad too, and we really should fix it asap.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch crash.patchSplinter Review
I'll submit a patch for older branches once this patch is reviewed.
Attachment #8704605 - Flags: review?(bugs)
Attachment #8704605 - Flags: review?(bugs) → review+
Attached patch m-b/m-aSplinter Review
Approval Request Comment
[Feature/regressing bug #]: bug 1085293 - WebIDL supporting iterable<>
[User impact if declined]: a crash
[Describe test coverage new/current, TreeHerder]: a test is included
[Risks and why]: none
[String/UUID change made/needed]: none
Attachment #8704676 - Flags: approval-mozilla-beta?
Attachment #8704676 - Flags: approval-mozilla-aurora?
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/43f10b226ee1 for failing test_formSubmission.html on Mulet Linux x64 opt

Backout pus: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=43f10b226ee1
Failing push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=23974693ffc5
https://treeherder.mozilla.org/logviewer.html#?job_id=19370515&repo=mozilla-inbound

16:16:41     INFO -  1168 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Correct name in send form using XHR and FormData - got "form-data; name=\"file_2\"", expected "form-data; name=\"file_2\"; filename=\"\""
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:627:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  Not taking screenshot here: see the one that was previously logged
16:16:41     INFO -  1169 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Correct content type in send form using XHR and FormData - got undefined, expected "application/octet-stream"
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:631:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  Not taking screenshot here: see the one that was previously logged
16:16:41     INFO -  1170 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Wrong number of headers in send form using XHR and FormData - got 1, expected 2
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:634:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  1171 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct value in send form using XHR and FormData
16:16:41     INFO -  1172 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct name in send form using XHR and FormData
16:16:41     INFO -  1173 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct content type in send form using XHR and FormData
16:16:41     INFO -  1174 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Wrong number of headers in send form using XHR and FormData
16:16:41     INFO -  1175 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct value in send form using XHR and FormData
16:16:41     INFO -  1176 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct name in send form using XHR and FormData
16:16:41     INFO -  1177 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct content type in send form using XHR and FormData
16:16:41     INFO -  1178 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Wrong number of headers in send form using XHR and FormData
16:16:41     INFO -  1179 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct value in send form using XHR and FormData
16:16:41     INFO -  1180 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct name in send form using XHR and FormData
16:16:41     INFO -  1181 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct content type in send form using XHR and FormData
16:16:41     INFO -  1182 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Wrong number of headers in send form using XHR and FormData
16:16:41     INFO -  1183 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct value in send form using XHR and FormData
16:16:41     INFO -  Not taking screenshot here: see the one that was previously logged
16:16:41     INFO -  1184 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Correct name in send form using XHR and FormData - got "form-data; name=\"file_5\"", expected "form-data; name=\"file_5\"; filename=\"\""
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:627:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  Not taking screenshot here: see the one that was previously logged
16:16:41     INFO -  1185 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Correct content type in send form using XHR and FormData - got undefined, expected "application/octet-stream"
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:631:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  Not taking screenshot here: see the one that was previously logged
16:16:41     INFO -  1186 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Wrong number of headers in send form using XHR and FormData - got 1, expected 2
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:634:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  1187 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct value in send form using XHR and FormData
Flags: needinfo?(amarchesini)
Attached patch patch (obsolete) — Splinter Review
Flags: needinfo?(amarchesini)
Attachment #8704762 - Flags: review?(bugs)
Comment on attachment 8704762 [details] [diff] [review]
patch

So how did this test use to work?  Was it just happening to work because we reused a union from a previous iteration or something?
Attachment #8704762 - Flags: review?(bugs) → review+
Attached patch testSplinter Review
A separate patch for m-i/m-a/m-b.

bz, I'll answer to your question soon.
Attachment #8704762 - Attachment is obsolete: true
The reason why before the test was working is because of this:

https://mxr.mozilla.org/mozilla-central/source/dom/html/nsFormSubmission.cpp#451

where we were forcing this contentType in case aFile was null as the spec says: "If there are no selected files, then append an entry to the form data set with the name as the name, the empty string as the value, and application/octet-stream as the type."

https://mxr.mozilla.org/mozilla-central/source/dom/html/nsFormSubmission.cpp#498

With the current changes, we don't support null files anymore, because we replace them with an empty string.
So, the current solution is not the right one.

I also think that the spec is not clear enough, because we don't have any setter for name+string+type. What should we return from a |get(that name)|? an empty string? or a blob with no content and a type?
If we want the latter, probably a good solution should be to create a new blob with no content and type and store it in the formData directly.
Flags: needinfo?(annevk)
My recommendation is that we use an empty Blob for this case, similar to what we transmit to the server.
Flags: needinfo?(annevk)
I'm filing a follow up for this next step.
Blocks: 1237595
Comment on attachment 8704676 [details] [diff] [review]
m-b/m-a

Please renominate once test failures are addressed.
Attachment #8704676 - Flags: approval-mozilla-beta?
Attachment #8704676 - Flags: approval-mozilla-beta-
Attachment #8704676 - Flags: approval-mozilla-aurora?
Attachment #8704676 - Flags: approval-mozilla-aurora-
https://hg.mozilla.org/mozilla-central/rev/f149d988f78e
https://hg.mozilla.org/mozilla-central/rev/524b2e90d33f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla41 → mozilla46
Too late to fix this for 43.
Tracking for 45+ since this sounds like either a regression or a crash we really don't want to have.
I don't see a lot of crashes with this signature currently though.
We should probably track for 44 too, yes?

And we presumably need branch approval requests here?
Flags: needinfo?(lhenry)
Flags: needinfo?(amarchesini)
Bz, I am not inclined to take any fixes in Fx44 now unless they are critical (recent) regressions, sec and stability fixes. While this fixes a crash, but to me the likelihood of getting into this situation is low. Please let me know if that is not the case. Thanks.!
I can live with this, I guess; this is not in widespread use yet and the crash is at least a safe null-deref...

On the other hand, it also seems like we could create a pretty minimally targeted crash fix for 44, if that were desired.
Comment on attachment 8704605 [details] [diff] [review]
crash.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1085293 - WebIDL supporting iterable<>
[User impact if declined]: a crash
[Describe test coverage new/current, TreeHerder]: a test is included
[Risks and why]: none
[String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8704605 - Flags: approval-mozilla-beta?
Attachment #8704605 - Flags: approval-mozilla-aurora?
Thanks bz, looks like this missed 44 but we can probably uplift this at least to 45 (even if it ends up landing after merge day)
Flags: needinfo?(lhenry)
Comment on attachment 8704605 [details] [diff] [review]
crash.patch

Taking it in 45 before the merge.
Attachment #8704605 - Flags: approval-mozilla-beta?
Attachment #8704605 - Flags: approval-mozilla-aurora?
Attachment #8704605 - Flags: approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.