Bug 176659 – Finish off the FormData implementation

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Looks like we have a partial implementation of FormData (https://xhr.spec.whatwg.org/#interface-formdata). Pretty easy spec, so let's finish it off.

[Sam Weinig]

Created attachment 320360 [details]
Patch

[Sam Weinig]

Created attachment 320366 [details]
Patch

[Sam Weinig]

Created attachment 320370 [details]
Patch

[Darin Adler]

Comment on attachment 320370 [details]
Patch

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

Is it OK that all these functions are inefficient if there are a huge numbers of elements, because this is a vector and not an associative data structure?

> Source/WebCore/fileapi/Blob.cpp:82
> +Blob::Blob(const Blob& blob)

Where is this new function used? Given that it seems that a Blob is both reference counted and immutable, I don’t understand when we need to copy one. Nor can I spot it reading the code.

> Source/WebCore/html/DOMFormData.cpp:72
> +}
> +auto DOMFormData::get(const String& name) -> std::optional<FormDataEntryValue>

Missing blank line.

> Source/WebCore/html/DOMFormData.cpp:126
> +    return WTF::KeyValuePair<String, FormDataEntryValue> { item.key(), item.data() };

Should we add a makeKeyValuePair function so we don’t have to write out types in a case like this?

> Source/WebCore/html/DOMFormData.idl:33
> +// FIXME: Should be Exposed=(Window,Worker)

Would be nice to have the comment explain what this is waiting on.

> Source/WebCore/html/FormDataList.cpp:44
> +        auto file = File::create(blob.get(), filename.isNull() ? ASCIILiteral("blob") : filename);
> +        return { key, WTFMove(file) };

Do this as a one-liner? It would only be 4 characters longer than the File::create line.

> Source/WebCore/html/FormDataList.cpp:49
> +        auto file = File::create(downcast<File>(blob.get()), filename);
> +        return { key, WTFMove(file) };

Ditto.

> Source/WebCore/html/FormDataList.cpp:71
> +template<typename EntryMaker>
> +void FormDataList::set(const String& key, const EntryMaker& entryMaker)

Looking at the logic below, it seems that EntryMaker is always called, and there is no ordering issue due to side effects that I can see. Thus, I think this function could take an Item&& instead and would not need to be a template.

> Source/WebCore/html/FormDataList.cpp:73
> +    size_t initialMatchLocation = WTF::notFound;

I suggest we use std::optional for clarity instead of WTF::notFound.

> Source/WebCore/html/FormDataList.cpp:79
> +            m_items[i] = entryMaker();

This would be easier to read if the loop was kept as a pure "findKey" operation.

Could move this one line of code into the if statement below instead of putting it inside the loop. I can’t think of a good reason to have some of work inside the loop and some outside.

> Source/WebCore/html/FormDataList.h:32
>      class Item {

Seems like this should be a struct instead of a class now. Unless we think Variant is so inconvenient to work with that we want to hide it behind "private".

> Source/WebCore/html/FormDataList.h:66
> +    CString normalizeString(const String&) const;

Seems a little strange now that we hold a TextEncoding just so that clients can explicitly call normalizeString. Is m_encoding used for anything else? Is there a more elegant way to arrange things?

> Source/WebCore/html/HTMLKeygenElement.cpp:126
> +    encoded_values.appendData(name(), value);

Could save one little reference count churn if a version that took String&& existed. Probably not worth it.

Does this fix a bug because we use the proper TextEncoding now rather than always UTF-8? Or is this always going to be an ASCII string and so it doesn’t matter what encoding is used?

> Source/WebCore/platform/network/FormData.cpp:209
> +                        String generatedFileName;
> +                        shouldGenerateFile = page->chrome().client().shouldReplaceWithGeneratedFileForUpload(path, generatedFileName);

Should come back and change the ChromeClient function to use a return value instead of an out argument. A boolean plus a String out argument seems out of style in our code these days. Not sure what is ideal, though. Maybe std::optional<String>? Or even just String using null string or empty string to mean false, but we always could have done that.

> Source/WebCore/platform/network/FormData.cpp:231
> +                auto file = WTF::get<RefPtr<File>>(item.data());

I would have written "auto& file = *WTF::get ..." here and had the local variable be a File& instead of a guaranteed non-null RefPtr<File>.

> Source/WebCore/platform/network/FormData.cpp:233
> +                if (!file->path().isEmpty())
> +                    appendFile(file->path(), shouldGenerateFile);

It is a bit awkward that we have to check holds_alternative<RefPtr<File>> and then file->path().isEmpty() again here, and then rely on the fact that the side effect of calling shouldReplaceWithGeneratedFileForUpload is in sync with this code. I wonder if there’s any way to tie the code above to the code down here in a tighter way. Annoying to re-do all the same conditionals after just two lines of unconditional code.

It’s mainly because the append functions have to be in a certain order because they have side effects, I think.

Similar problem with the various isMultipartForm. I think there is some improvement possible here at some point with some additional refactoring.

> Source/WebCore/platform/network/FormData.cpp:237
> +                auto normalizedStingData = list.normalizeString(WTF::get<String>(item.data()));

Typo here: "Sting".

> Source/WebCore/platform/network/FormData.cpp:244
> +            auto normalizedStingData = list.normalizeString(WTF::get<String>(item.data()));

Typo here: "Sting".

> Source/WebCore/platform/network/FormData.cpp:260
> +    auto& e = m_elements.last();

Amazed that you resisted the impulse to rename this lastElement.

> Source/WebCore/platform/network/FormData.cpp:262
>      e.m_data.grow(oldSize + size);

This math seems like it needs an overflow check or an explanation of why it doesn’t need one.

> Source/WebCore/platform/network/FormData.cpp:275
>      }
> +    
> +    return data;

Seems a bit strange to have this one blank line in this function.

> Source/WebCore/platform/network/FormData.cpp:312
> +    for (const auto& element : m_elements) {

I would have just done "auto&"; I don’t think it adds much clarity to state const.

> Source/WebCore/platform/network/FormData.cpp:327
> +    for (const auto& element : m_elements) {

Ditto.

> Source/WebCore/platform/network/FormData.cpp:361
> +    for (const auto& element : m_elements) {

I would have just done auto& here; it will be const either way since this is a const member function, no need to state it explicitly.

> Source/WebCore/platform/network/FormData.cpp:370
> +    for (const auto& element : m_elements) {

Ditto.

[Sam Weinig]

(In reply to Darin Adler from comment #4)
> Comment on attachment 320370 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320370&action=review
> 
> Is it OK that all these functions are inefficient if there are a huge
> numbers of elements, because this is a vector and not an associative data
> structure?

That's an interesting question, and one I don't have a great answer for. I would guess the use case for this object would remain mostly append, and relatively small number of elements, but that often ends up being a bad assumption.

I'll think a bit more about it, and look at what converting this (and maybe also URLSearchParams which is very similar) to use a ListHashSet. 

> 
> > Source/WebCore/fileapi/Blob.cpp:82
> > +Blob::Blob(const Blob& blob)
> 
> Where is this new function used? Given that it seems that a Blob is both
> reference counted and immutable, I don’t understand when we need to copy
> one. Nor can I spot it reading the code.

This is used from the File constructor that takes a const Blob&. Blob and File are refcounted, these aren't really acting as copy constructors, but rather, this constructor creates a Blob that references the passed in Blob (or at least references it's URL, in the way Blobs do). 

> 
> > Source/WebCore/html/DOMFormData.cpp:72
> > +}
> > +auto DOMFormData::get(const String& name) -> std::optional<FormDataEntryValue>
> 
> Missing blank line.

Will fix.

> 
> > Source/WebCore/html/DOMFormData.cpp:126
> > +    return WTF::KeyValuePair<String, FormDataEntryValue> { item.key(), item.data() };
> 
> Should we add a makeKeyValuePair function so we don’t have to write out
> types in a case like this?

Yes. Will do in a follow up.

> 
> > Source/WebCore/html/DOMFormData.idl:33
> > +// FIXME: Should be Exposed=(Window,Worker)
> 
> Would be nice to have the comment explain what this is waiting on.

Indeed. I'll also file a bug and reference it.

> 
> > Source/WebCore/html/FormDataList.cpp:44
> > +        auto file = File::create(blob.get(), filename.isNull() ? ASCIILiteral("blob") : filename);
> > +        return { key, WTFMove(file) };
> 
> Do this as a one-liner? It would only be 4 characters longer than the
> File::create line.

Will do.

> 
> > Source/WebCore/html/FormDataList.cpp:49
> > +        auto file = File::create(downcast<File>(blob.get()), filename);
> > +        return { key, WTFMove(file) };
> 
> Ditto.


Will do.

> 
> > Source/WebCore/html/FormDataList.cpp:71
> > +template<typename EntryMaker>
> > +void FormDataList::set(const String& key, const EntryMaker& entryMaker)
> 
> Looking at the logic below, it seems that EntryMaker is always called, and
> there is no ordering issue due to side effects that I can see. Thus, I think
> this function could take an Item&& instead and would not need to be a
> template.

Nice. I wonder what I was thinking. 

> 
> > Source/WebCore/html/FormDataList.cpp:73
> > +    size_t initialMatchLocation = WTF::notFound;
> 
> I suggest we use std::optional for clarity instead of WTF::notFound.

Will do.

> 
> > Source/WebCore/html/FormDataList.cpp:79
> > +            m_items[i] = entryMaker();
> 
> This would be easier to read if the loop was kept as a pure "findKey"
> operation.
> 
> Could move this one line of code into the if statement below instead of
> putting it inside the loop. I can’t think of a good reason to have some of
> work inside the loop and some outside.
> 
> > Source/WebCore/html/FormDataList.h:32
> >      class Item {
> 
> Seems like this should be a struct instead of a class now. Unless we think
> Variant is so inconvenient to work with that we want to hide it behind
> "private".
> 
> > Source/WebCore/html/FormDataList.h:66
> > +    CString normalizeString(const String&) const;
> 
> Seems a little strange now that we hold a TextEncoding just so that clients
> can explicitly call normalizeString. Is m_encoding used for anything else?
> Is there a more elegant way to arrange things?

Not sure. I was hoping when I merged DOMFormData and FormDataList a solution would emerge. My guess is we can just pass the encoding to FormData with the DOMFormData, but I'll have to work that out.

> 
> > Source/WebCore/html/HTMLKeygenElement.cpp:126
> > +    encoded_values.appendData(name(), value);
> 
> Could save one little reference count churn if a version that took String&&
> existed. Probably not worth it.
> 
> Does this fix a bug because we use the proper TextEncoding now rather than
> always UTF-8? Or is this always going to be an ASCII string and so it
> doesn’t matter what encoding is used?

It was always ASCII (a base-64 encoded string), so it didn't matter.

> 
> > Source/WebCore/platform/network/FormData.cpp:209
> > +                        String generatedFileName;
> > +                        shouldGenerateFile = page->chrome().client().shouldReplaceWithGeneratedFileForUpload(path, generatedFileName);
> 
> Should come back and change the ChromeClient function to use a return value
> instead of an out argument. A boolean plus a String out argument seems out
> of style in our code these days. Not sure what is ideal, though. Maybe
> std::optional<String>? Or even just String using null string or empty string
> to mean false, but we always could have done that.

Totally. I need to change all this so it can work with Workers, so I tried not to touch it in this round.

> 
> > Source/WebCore/platform/network/FormData.cpp:231
> > +                auto file = WTF::get<RefPtr<File>>(item.data());
> 
> I would have written "auto& file = *WTF::get ..." here and had the local
> variable be a File& instead of a guaranteed non-null RefPtr<File>.

Good call.

> 
> > Source/WebCore/platform/network/FormData.cpp:233
> > +                if (!file->path().isEmpty())
> > +                    appendFile(file->path(), shouldGenerateFile);
> 
> It is a bit awkward that we have to check holds_alternative<RefPtr<File>>
> and then file->path().isEmpty() again here, and then rely on the fact that
> the side effect of calling shouldReplaceWithGeneratedFileForUpload is in
> sync with this code. I wonder if there’s any way to tie the code above to
> the code down here in a tighter way. Annoying to re-do all the same
> conditionals after just two lines of unconditional code.
> 
> It’s mainly because the append functions have to be in a certain order
> because they have side effects, I think.
> 
> Similar problem with the various isMultipartForm. I think there is some
> improvement possible here at some point with some additional refactoring.

Yeah, this code is asking for a bath :).

> 
> > Source/WebCore/platform/network/FormData.cpp:237
> > +                auto normalizedStingData = list.normalizeString(WTF::get<String>(item.data()));
> 
> Typo here: "Sting".

Will fix.

> 
> > Source/WebCore/platform/network/FormData.cpp:244
> > +            auto normalizedStingData = list.normalizeString(WTF::get<String>(item.data()));
> 
> Typo here: "Sting".
> 
> > Source/WebCore/platform/network/FormData.cpp:260
> > +    auto& e = m_elements.last();
> 
> Amazed that you resisted the impulse to rename this lastElement.

Me too. I renamed so many other single letter variables.

> 
> > Source/WebCore/platform/network/FormData.cpp:262
> >      e.m_data.grow(oldSize + size);
> 
> This math seems like it needs an overflow check or an explanation of why it
> doesn’t need one.

Hm. I will investigate it.

> 
> > Source/WebCore/platform/network/FormData.cpp:275
> >      }
> > +    
> > +    return data;
> 
> Seems a bit strange to have this one blank line in this function.

Will fix.

> 
> > Source/WebCore/platform/network/FormData.cpp:312
> > +    for (const auto& element : m_elements) {
> 
> I would have just done "auto&"; I don’t think it adds much clarity to state
> const.
> 
> > Source/WebCore/platform/network/FormData.cpp:327
> > +    for (const auto& element : m_elements) {
> 
> Ditto.
> 
> > Source/WebCore/platform/network/FormData.cpp:361
> > +    for (const auto& element : m_elements) {
> 
> I would have just done auto& here; it will be const either way since this is
> a const member function, no need to state it explicitly.
> 
> > Source/WebCore/platform/network/FormData.cpp:370
> > +    for (const auto& element : m_elements) {
> 
> Ditto.

Will fix.


Thanks so much for the review.

[Darin Adler]

Comment on attachment 320370 [details]
Patch

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

>>> Source/WebCore/fileapi/Blob.cpp:82
>>> +Blob::Blob(const Blob& blob)
>> 
>> Where is this new function used? Given that it seems that a Blob is both reference counted and immutable, I don’t understand when we need to copy one. Nor can I spot it reading the code.
> 
> This is used from the File constructor that takes a const Blob&. Blob and File are refcounted, these aren't really acting as copy constructors, but rather, this constructor creates a Blob that references the passed in Blob (or at least references it's URL, in the way Blobs do).

I still don’t understand. Why can’t we just use the original Blob in that case instead of making a new one? Do we need separate sets of custom properties when it’s web-exposed? Or some other reason?

>> Source/WebCore/html/FormDataList.h:32
>>      class Item {
> 
> Seems like this should be a struct instead of a class now. Unless we think Variant is so inconvenient to work with that we want to hide it behind "private".

I noticed you did not respond to this one in either the negative or the affirmative. I hope that’s because you decided to do it and thought it was so obvious it did not even need to be mentioned.

[Darin Adler]

(In reply to Sam Weinig from comment #5)
> I'll think a bit more about it, and look at what converting this (and maybe
> also URLSearchParams which is very similar) to use a ListHashSet. 

Sadly it seems this requires support for duplicate keys, and so I suspect ListHashSet won’t be usable as-is.

[Sam Weinig]

Created attachment 320402 [details]
Patch

[Sam Weinig]

(In reply to Darin Adler from comment #7)
> (In reply to Sam Weinig from comment #5)
> > I'll think a bit more about it, and look at what converting this (and maybe
> > also URLSearchParams which is very similar) to use a ListHashSet. 
> 
> Sadly it seems this requires support for duplicate keys, and so I suspect
> ListHashSet won’t be usable as-is.

Oh darn, good point.

[Sam Weinig]

(In reply to Darin Adler from comment #6)
> Comment on attachment 320370 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320370&action=review
> 
> >>> Source/WebCore/fileapi/Blob.cpp:82
> >>> +Blob::Blob(const Blob& blob)
> >> 
> >> Where is this new function used? Given that it seems that a Blob is both reference counted and immutable, I don’t understand when we need to copy one. Nor can I spot it reading the code.
> > 
> > This is used from the File constructor that takes a const Blob&. Blob and File are refcounted, these aren't really acting as copy constructors, but rather, this constructor creates a Blob that references the passed in Blob (or at least references it's URL, in the way Blobs do).
> 
> I still don’t understand. Why can’t we just use the original Blob in that
> case instead of making a new one? Do we need separate sets of custom
> properties when it’s web-exposed? Or some other reason?

Oh. Yeah, we need to convert the Blob into a File per https://xhr.spec.whatwg.org/#create-an-entry (specifically step 3 "If value is a Blob object and not a File object, then set value to a new File object, representing the same bytes, whose name attribute value is "blob"."). This is web visible, since you can append a Blob, but then when you go an inspect what you just appended, it needs to be a file.  

> 
> >> Source/WebCore/html/FormDataList.h:32
> >>      class Item {
> > 
> > Seems like this should be a struct instead of a class now. Unless we think Variant is so inconvenient to work with that we want to hide it behind "private".
> 
> I noticed you did not respond to this one in either the negative or the
> affirmative. I hope that’s because you decided to do it and thought it was
> so obvious it did not even need to be mentioned.

Oops, sorry. Yeah, I made it a struct.

[Sam Weinig]

Committed r221839: <http://trac.webkit.org/changeset/221839>

[Michael Catanzaro]

Hey Sam:

[2201/4301] Building CXX object Source...keFiles/WebCore.dir/fileapi/Blob.cpp.o
../../Source/WebCore/fileapi/Blob.cpp: In copy constructor ‘WebCore::Blob::Blob(const WebCore::Blob&)’:
../../Source/WebCore/fileapi/Blob.cpp:82:1: warning: base class ‘class WTF::RefCounted<WebCore::Blob>’ should be explicitly initialized in the copy constructor [-Wextra]
 Blob::Blob(const Blob& blob)
 ^~~~

[Sam Weinig]

(In reply to Michael Catanzaro from comment #12)
> Hey Sam:
> 
> [2201/4301] Building CXX object
> Source...keFiles/WebCore.dir/fileapi/Blob.cpp.o
> ../../Source/WebCore/fileapi/Blob.cpp: In copy constructor
> ‘WebCore::Blob::Blob(const WebCore::Blob&)’:
> ../../Source/WebCore/fileapi/Blob.cpp:82:1: warning: base class ‘class
> WTF::RefCounted<WebCore::Blob>’ should be explicitly initialized in the copy
> constructor [-Wextra]
>  Blob::Blob(const Blob& blob)
>  ^~~~

What compiler / port is that?

[Michael Catanzaro]

GCC 7.1.1

It's WebKitGTK+, but that shouldn't matter....

[Michael Catanzaro]

So I think the problem is that Blob and File are supposed to be noncopyable, because they are RefCounted, but you have defined copy constructors for them. I won't roll this out, but reopening for follow-up.

[Sam Weinig]

(In reply to Michael Catanzaro from comment #15)
> So I think the problem is that Blob and File are supposed to be noncopyable,
> because they are RefCounted, but you have defined copy constructors for
> them. I won't roll this out, but reopening for follow-up.

Thanks. I'll post a fix shortly.

Separately, it would be good to get this warning enabled on one (or as many as support it) of the EWS bots.

[Sam Weinig]

Fix landed in r221936.

[Sam Weinig]

Let me know if its still an issue.

[Radar WebKit Bug Importer]

<rdar://problem/34694059>