Bug 111687 – FormData should allow setting filename to empty

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Alexey Proskuryakov]

Per XMLHttpRequest spec, empty filename should be supported:

- Set its name to name.
- Set its value to value.
- Set its type to "text" if value is a string and "file" if it is a Blob.
- If its type is "file" set its filename to "blob".
- If its type is "file" and value is a File whose name attribute is not the empty string, set entry's filename to the attribute's value.
- If the filename parameter is not omitted set entry's filename to filename.

This makes sense, because empty form type=file controls are sent as files with empty name by normal form submission.

Also, this would be helpful for WebKit, because currently we have to simulate empty path with a File that has an empty path internally, which is an unfortunate hack. Using a blob would be nicer.

[Alexey Proskuryakov]

Created attachment 191928 [details]
proposed fix

[Victor Costan]

Thank you for cc-ing me on this! I will use the test structure you set up there for the tests in 111380.

[Alexey Proskuryakov]

> we have to simulate empty path with a File that has an empty path

This wasn't very clear, but the patch should speak louder than words.

[Benjamin Poulain]

Comment on attachment 191928 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=191928&action=review

> Source/WebCore/html/FileInputType.cpp:158
> +        encoding.appendBlob(element()->name(), BlobBuilder().getBlob("application/octet-stream"), emptyString());

ASCIILiteral("application/octet-stream")?

[Victor Costan]

Since BlobBuilder was deprecated in the spec, would it simplify the code if its implementation was moved into the Blob constructor?
 
I'm not saying that this should happen here, BTW. I'm mostly asking because I'd be interested in taking up the cleanup work, to learn more about the codebase.

[Alexey Proskuryakov]

> ASCIILiteral("application/octet-stream")?

Can do.

> Since BlobBuilder was deprecated in the spec, would it simplify the code if its implementation was moved into the Blob constructor?

Possibly. Currently, BlobBuilder interface is a bit too large to include into Blob, and it's oriented towards incremental building. But perhaps most of that is not needed any more, and can be simplified.

In this patch, I could have avoided using BlobBuilder, although I'm not sure if that's a win:

OwnPtr<BlobData> emptyBlobData = BlobData::create();
emptyBlobData->setContentType(ASCIILiteral("application/octet-stream"));
encoding.appendBlob(element()->name(), Blob::create(emptyBlobData.release(), 0), emptyString());

[Brady Eidson]

Comment on attachment 191928 [details]
proposed fix

R+ with ASCIILiteral fix

[Victor Costan]

I also don't think removing the BlobBuilder reference is a win. If that cleanup is ever desirable, I'd like to have a helper that makes this task a one-liner.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/145097>.

[WebKit Review Bot]

Re-opened since this is blocked by bug 111765

[Adam Barth]

ap and I discussed this topic on #webkit.  We're going to roll out the change for the time being.  I'm going to remove the ASSERT from the Chromium code and then re-land this patch.

[Adam Barth]

Chromium side-change is under review at https://codereview.chromium.org/12633004/

[Alexey Proskuryakov]

OK, so turns out that old and new code is equally wrong in this regard, but old code did not have an assertion to catch that.

What happens is that the object created here is deleted earlier than it should be - it is owned by DOMFormData, and that's destroyed at the end of FormSubmission::create(). So, a Blob or a File becomes unregistered in blob registry, and cannot be used when actually building form data stream in another process.

We should obviously keep uploaded objects alive while form submission takes place.

Anyway, I'm going to re-land the parts of this patch that actually implement what bug title says. This additional fix should be made separately.

[Adam Barth]

> Anyway, I'm going to re-land the parts of this patch that actually implement what bug title says. This additional fix should be made separately.

It's unclear to me whether I should land <https://codereview.chromium.org/12633004/>.

[Alexey Proskuryakov]

Re-landed safe parts in <http://trac.webkit.org/changeset/145142>. Filed bug 111778 to track the pre-existing issue.

[Alexey Proskuryakov]

> It's unclear to me whether I should land <https://codereview.chromium.org/12633004/>.

I don't think that it should be landed any more.

[Adam Barth]

> I don't think that it should be landed any more.

Thanks.  I've marked the code review as closed.

[Alexey Proskuryakov]

I appreciate your help dealing with this issue, and Michael's insight into the reasons of the assertion!

[Michael Nordman]

Fyi: I've been working towards fixing that for a while now. Its not just a problem with FormData. The changes I've got are simple enough but landing it is troublesome because I'm changing what the blob identifiers identify and I haven't come up with a great way to do that given the chromium/webkit split svn personality. 

webcore changes - https://codereview.chromium.org/11192017/
chromium changes - https://codereview.chromium.org/11410019/

I have been whittling down on the size of these changes by landing smaller parts independently. This small change awaiting review is a part of that...
   https://bugs.webkit.org/show_bug.cgi?id=108851
... but there come the time to change what the identifier identifies and I don't know how to cross that chasm quite yet.

[Michael Nordman]

Here's the umbrella bug open for that.
https://bugs.webkit.org/show_bug.cgi?id=108733

[Dean Jackson]

http/tests/misc/empty-file-formdata.html is failing on WK1 Mac after this
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r145143%20(7681)/results.html

[Alexey Proskuryakov]

Accidental commit of local changes, fixed that now.