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.
Created attachment 191928 [details] proposed fix
Thank you for cc-ing me on this! I will use the test structure you set up there for the tests in 111380.
> 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.
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")?
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.
> 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());
Comment on attachment 191928 [details] proposed fix R+ with ASCIILiteral fix
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.
Committed <http://trac.webkit.org/changeset/145097>.
Re-opened since this is blocked by bug 111765
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.
Chromium side-change is under review at https://codereview.chromium.org/12633004/
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.
> 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/>.
Re-landed safe parts in <http://trac.webkit.org/changeset/145142>. Filed bug 111778 to track the pre-existing issue.
> 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.
> I don't think that it should be landed any more. Thanks. I've marked the code review as closed.
I appreciate your help dealing with this issue, and Michael's insight into the reasons of the assertion!
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.
Here's the umbrella bug open for that. https://bugs.webkit.org/show_bug.cgi?id=108733
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
Accidental commit of local changes, fixed that now.