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

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Attachment: crash3.patch

+++ 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.

Comment 1

[Andrea Marchesini [:baku]]

Comment on attachment 8705109 [details] [diff] [review]
crash3.patch

I must update a couple of tests.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: crash3.patch

Comment 3

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

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?

Comment 4

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

(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?

Comment 5

[Andrea Marchesini [:baku]]

Bug 1162658

Comment 6

[Andrea Marchesini [:baku]]

> 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.

Comment 7

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

(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. "

Comment 8

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

Hmm, I guess the most recent patch in that bug is pretty much enough, no? and after that we could use Blob here?

Comment 9

[Andrea Marchesini [:baku]]

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.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: crash3.patch

Comment 11

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

I guess that approach works assuming  bug 1162658 gets done before the next merge.

Comment 12

[Pulsebot]

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

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Maxime]

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.

Comment 15

[Andrea Marchesini [:baku]]

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.

Comment 16

[ccd0]

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.