Closed Bug 1237595 Opened 5 years ago Closed 5 years ago

FormData ctor and form submission should create empty Blob/File when a input type=file is not set.

Categories

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

39 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch crash3.patch (obsolete) — Splinter Review
+++ This bug was initially created as a clone of Bug #1187157 +++

https://github.com/whatwg/html/issues/476

Also if this issue is not accepted, the patch restores the previous version of the testSubmission.html test and it makes everything cleaner.
Attachment #8705109 - Flags: review?(bugs)
Comment on attachment 8705109 [details] [diff] [review]
crash3.patch

I must update a couple of tests.
Attachment #8705109 - Attachment is obsolete: true
Attachment #8705109 - Flags: review?(bugs)
Attached patch crash3.patch (obsolete) — Splinter Review
Attachment #8705126 - Flags: review?(bugs)
Comment on attachment 8705126 [details] [diff] [review]
crash3.patch


> ////////////////////////////////////////////////////////////////////////////
>+// BlobImplEmptyFile implementation
>+
>+NS_IMPL_ISUPPORTS_INHERITED0(BlobImplEmptyFile, BlobImpl)
>+
>+already_AddRefed<BlobImpl>
>+BlobImplEmptyFile::CreateSlice(uint64_t aStart, uint64_t aLength,
>+                               const nsAString& aContentType,
>+                               ErrorResult& aRv)
>+{
>+  if (aStart || aLength) {
>+    aRv.Throw(NS_ERROR_UNEXPECTED);
>+    return nullptr;
>+  }
>+
Couldn't we just assert here that aStart and aLength are 0.
BlobImpl::Slice calls this method and and should pass 0,0 here.



>+
>+  nsAutoCString str;
>+  aRv = strStream->SetData(str.BeginReading(), 0 /* length */);
Can't you use EmptyCString here?

> <script type="text/javascript">
>-  is(new FormData(document.getElementById('a')).get('b'), "", "This should return an empty string.");
>+  var obj = new FormData(document.getElementById('a')).get('b');
>+
>+  ok(obj instanceof File, "This should return an empty File");
>+  is(obj.size, 0, "Size should be 0");
>+  is(obj.name, "", "Name should be an empty string");
>+  is(obj.type, "application/octet-stream", "Type should be application/octet-stream");
Could you also add a test which calls slice with some unexpected values.


>     if (files.IsEmpty()) {
>-      // If no file was selected, pretend we had an empty file with an
>-      // empty filename.
>-      aFormSubmission->AddNameFilePair(name, nullptr);
>+      RefPtr<BlobImpl> blobImpl =
>+        new BlobImplEmptyFile(NS_LITERAL_STRING("application/octet-stream"));
>+      RefPtr<File> file = File::Create(OwnerDoc()->GetInnerWindow(), blobImpl);
>+      aFormSubmission->AddNameFilePair(name, file);
So in the spec bug annevk suggested Blob and not File.


Did you verify the submitted data stays the same with these changes?
(In reply to Olli Pettay [:smaug] from comment #3)
> So in the spec bug annevk suggested Blob and not File.
I can't now recall what was the issue with Blobs and Files in FormData. There was some spec issue, or Gecko bug, do you have a link to that?
> So in the spec bug annevk suggested Blob and not File.

Right, but our current implementation is not in sync with the spec. For instance, the spec says that FormDataEntryValue is (Blob or USVString) but we are still using (File or USVString).

> Did you verify the submitted data stays the same with these changes?

test_formSubmission checks that.
(In reply to Andrea Marchesini (:baku) from comment #5)
> Bug 1162658

ok, and the spec has changed since that bug was filed. Spec isn't anymore too backwards incompatible.
I wonder what to do with that and with this bug.
Would it be simple change to Blob and not File, and in append call
"If the filename argument is given, set value to a new File object whose contents are value and name is filename. "
Hmm, I guess the most recent patch in that bug is pretty much enough, no? and after that we could use Blob here?
What about if we finish this and then I fix File-to-Blob in bug 1162658?
I don't know how much of the patch for bug 1162658 is still valid.
Blocks: 1237674
Attached patch crash3.patchSplinter Review
Attachment #8705126 - Attachment is obsolete: true
Attachment #8705126 - Flags: review?(bugs)
Attachment #8705259 - Flags: review?(bugs)
I guess that approach works assuming  bug 1162658 gets done before the next merge.
Attachment #8705259 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/d607fcbd12d4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Is it really necessary to set the filename to "blob"?

When the filename isn't an empty string, PHP (and Rack, and probably other multipart decoders) seems to treat it as an actual file upload (it creates the temp file, sets the error code to OK, etc...).

Submitting the form normally (without FormData/xhr) does use an empty string for the filename though, which PHP ignores as it should.
Maxime, by spec, this is what we should implement. But I filed a bug last week just to change this 'blob' filename issue.
Actually, I also implemented the new version of the spec removing this 'blob' filename: bug 1241171.
I'm going to land that patch on monday.
Setting the filename to "blob" breaks sites existing sites since there is no longer a way of distinguishing between no file having been selected and an empty file named "blob".

From https://boards.4chan.org/qa/thread/418502:
> On nightly, if you post without an image using quick reply, it complains about the file size.
Depends on: 1241171
Depends on: 1303999
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.