Bug 227718 – Add support for FormDataEvent

• Component: New Bugs
• Status: RESOLVED FIXED

[Qiaosong Zhou]

FormData event handler addition

[Chris Dumez]

(In reply to Qiaosong Zhou from comment #0)
> FormData event handler addition

Please provide a better bug title / description. This is way too vague.

[Qiaosong Zhou]

Created attachment 432970 [details]
Patch

[Chris Dumez]

Comment on attachment 432970 [details]
Patch

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

Also missing test or test rebaseline.

> Source/WebCore/ChangeLog:7
> +

Please add detailed change log description.

> Source/WebCore/platform/network/FormData.cpp:96
> +    scheduleEvent(eventNames().formdataEvent)

Spec says we should fire an FormDataEvent, not a generic Event.

[Qiaosong Zhou]

Sorry I'm just creating a ChangeLog stub. Haven't finalized the changes yet.

[Chris Dumez]

(In reply to Qiaosong Zhou from comment #4)
> Sorry I'm just creating a ChangeLog stub. Haven't finalized the changes yet.

Then please do not set r? flag, which indicates that you are requesting a review.

[Qiaosong Zhou]

Created attachment 433304 [details]
Patch

[Chris Dumez]

Comment on attachment 433304 [details]
Patch

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

> Source/WebCore/dom/FormDataEvent.cpp:43
> +    : Event(eventType, initializer, IsTrusted::Yes)

This doesn't seem right. Events constructed by JavaScript should NOT be trusted. Only the events constructed by the browser in native code should be trusted.
As far as I can tell, the JS constructed ends up calling this constructor and is incorrectly marked as trusted.

[Qiaosong Zhou]

Created attachment 433355 [details]
Patch

[Qiaosong Zhou]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 433304 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433304&action=review
> 
> > Source/WebCore/dom/FormDataEvent.cpp:43
> > +    : Event(eventType, initializer, IsTrusted::Yes)
> 
> This doesn't seem right. Events constructed by JavaScript should NOT be
> trusted. Only the events constructed by the browser in native code should be
> trusted.
> As far as I can tell, the JS constructed ends up calling this constructor
> and is incorrectly marked as trusted.

Thanks for the feedback. I'll change this on the next upload. Currently for some reason DOMFormData constructor is called repeatedly when a FormDataEvent is dispatched to HTMLFormElement, causing a stack overflow.

[Chris Dumez]

Comment on attachment 433355 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:60
> +    auto formDataEvent = FormDataEvent::create(eventNames().formdataEvent, Event::CanBubble::No, Event::IsCancelable::No, Event::IsComposed::No, this);

You probably need to move the event creation / dispatch to DOMFormData::create(), *after* calling the constructor.

Calling this from inside the destructor is a bad idea because constructing the FormDataEvent will ref() |this| before it is done constructing.

[Qiaosong Zhou]

Created attachment 433367 [details]
Patch

[Chris Dumez]

Comment on attachment 433367 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:64
> +    EventDispatcher::dispatchEvent(*form, formDataEvent.get());

form can be null so *form is wrong here.

[Chris Dumez]

Comment on attachment 433367 [details]
Patch

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

> Source/WebCore/dom/FormDataEvent.h:46
> +    static Ref<FormDataEvent> create(const AtomString&, CanBubble, IsCancelable, IsComposed, DOMFormData*);

Should take in a Ref<DOMFormData>&&, not a DOMFormData* since it cannot be null.

> Source/WebCore/dom/FormDataEvent.h:48
> +    DOMFormData* formData() const { return m_formData.get(); }

Should return a DOMFormData& since it cannot be null.

> Source/WebCore/dom/FormDataEvent.h:53
> +    RefPtr<DOMFormData> m_formData;

Should use Ref<> since it cannot be null.

> Source/WebCore/html/DOMFormData.cpp:63
> +    auto formDataEvent = FormDataEvent::create(eventNames().formdataEvent, Event::CanBubble::No, Event::IsCancelable::No, Event::IsComposed::No, formData.ptr());

formData.copyRef()

[Qiaosong Zhou]

Created attachment 433373 [details]
Patch

[Chris Dumez]

Comment on attachment 433373 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:5526
> +		FD3160BF12B0272A00C1A359 /* (null) in Headers */ = {isa = PBXBuildFile; };

Suspicious change.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:31622
> +				FD3160BF12B0272A00C1A359 /* (null) in Headers */,

Ditto.

> Source/WebCore/dom/FormDataEvent.h:35
> +struct FormDataEventInit : public EventInit {

Should probably be moved inside the FormDataEvent class.

> Source/WebCore/dom/FormDataEvent.h:53
> +    RefPtr<DOMFormData> m_formData;

Please leave a blank line between the functions and the data members.

> Source/WebCore/dom/FormDataEvent.idl:28
> +  constructor(DOMString type, FormDataEventInit eventInitDict);

We use 4 space indentation in WebKit (not 2 space).

> Source/WebCore/html/DOMFormData.cpp:61
> +    Ref<DOMFormData> formData = adoptRef(*new DOMFormData(form));

auto formData

> Source/WebCore/html/DOMFormData.cpp:62
> +    auto formDataEvent = FormDataEvent::create(eventNames().formdataEvent, Event::CanBubble::No, Event::IsCancelable::No, Event::IsComposed::No, formData.ptr());

Should be inside the if scope below. Why construct an event if you're not going to dispatch it?

> Source/WebCore/html/DOMFormData.cpp:64
> +        form->dispatchEvent(formDataEvent.get());

form->dispatchEvent(FormDataEvent::create(eventNames().formdataEvent, Event::CanBubble::No, Event::IsCancelable::No, Event::IsComposed::No, formData.ptr()));

[Radar WebKit Bug Importer]

<rdar://problem/80536177>

[Qiaosong Zhou]

Created attachment 433561 [details]
Patch

[Qiaosong Zhou]

Created attachment 433563 [details]
Patch

[Qiaosong Zhou]

Created attachment 433565 [details]
Patch

[Chris Dumez]

Comment on attachment 433565 [details]
Patch

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

Note that some tests still need rebaselining and there are a new real-looking crashes as well.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:5527
> +		FD3160BF12B0272A00C1A359 /* (null) in Headers */ = {isa = PBXBuildFile; };

Bad change, please drop.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:31611
> +				FD3160BF12B0272A00C1A359 /* (null) in Headers */,

ditto.

> Source/WebCore/dom/FormDataEvent.cpp:38
> +    if (!init.formData)

Should not be needed. The generated bindings code should already do this check for you, if it is marked as required in the IDL, then you can assume the pointer is non-null here.

> Source/WebCore/dom/FormDataEvent.cpp:57
> +    , m_formData(formData)

WTFMove(formData) to avoid ref-counting churn.

> Source/WebCore/dom/FormDataEvent.cpp:66
> +}

} // namespace WebCore

is our usual pattern.

> Source/WebCore/dom/FormDataEvent.h:38
> +    struct FormDataEventInit : public EventInit {

Just name this struct "Init". Also, no need for "public" since inheritance is public by default for struct (unlike class).

> Source/WebCore/dom/FormDataEvent.h:42
> +    using Init = FormDataEventInit;

Then drop this alias.

> Source/WebCore/dom/FormDataEvent.h:44
> +    static ExceptionOr<Ref<FormDataEvent>> create(const AtomString&, FormDataEventInit&&);

Shouldn't need the ExceptionOr<>

> Source/WebCore/dom/FormDataEvent.idl:26
> +[Exposed=Window]

The format we usually use in our IDL is:
[
    Exposed=Window
] interface FormDataEvent : Event {

> Source/WebCore/html/DOMFormData.cpp:63
> +        return dispatchFormDataEvent(adoptRef(*new DOMFormData(form)), *form);

Why aren't you using DOMFormData::create(form) ?

Also, if we should to leave the cloning up to the caller, then it would look like:
if (form) {
    auto formData = DOMFormData::create(form);
    auto dispatchResult = dispatchFormDataEvent(formData, *form);
    if (dispatchResult.hasException())
        return dispatchResult.releaseException();
    return formData->clone();
}

> Source/WebCore/html/DOMFormData.cpp:71
> +    if (!DOMFormData::formDataDispatched) {

Seems like this should be an early return instead:
if (isDispatchingFormDataEvent)
    return Exception { InvalidStateError, "Cannot call \"new FormData()\" within FormDataEvent handler"_s };

> Source/WebCore/html/DOMFormData.cpp:77
> +        return DOMFormData::create(WTFMove(formData));

Given that not all call sites seem to want cloning, we should probably return { }; return and have the return type be ExceptionOr<void>.

> Source/WebCore/html/DOMFormData.cpp:88
> +Ref<DOMFormData> DOMFormData::create(Ref<DOMFormData>&& other)

It feels weird to have a factory function on DOMFormData that takes in a Ref<DOMFormData>&& in. Shouldn't we simply add a "clone()" member function to DOMFormData?

> Source/WebCore/html/DOMFormData.h:58
> +    inline static bool formDataDispatched = false;

No inline here. I don't think using a static variable makes sense either. Imagine 2 tabs sharing the same process, I don't think you'd want the formdata dispatch for one page to be impacted by the formdata dispatch in another.

Also, the name should be something like "isDispatchingFormDataEvent".

I am also unclear why this is public.

> Source/WebCore/loader/FormSubmission.cpp:230
> +    DOMFormData::dispatchFormDataEvent(domFormData.copyRef(), form);

How come we're not using the returned DOMFormData here?

If this call site doesn't need to clone, then dispatchFormDataEvent() should probably not do the cloning and we should leave it up to the caller.

[Alex Christensen]

Comment on attachment 433565 [details]
Patch

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

> Source/WebCore/dom/GlobalEventHandlers.idl:59
>      // FIXME: Implement 'onformdata'.

This line should be removed.

[Alex Christensen]

Comment on attachment 433565 [details]
Patch

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

>> Source/WebCore/html/DOMFormData.h:58
>> +    inline static bool formDataDispatched = false;
> 
> No inline here. I don't think using a static variable makes sense either. Imagine 2 tabs sharing the same process, I don't think you'd want the formdata dispatch for one page to be impacted by the formdata dispatch in another.
> 
> Also, the name should be something like "isDispatchingFormDataEvent".
> 
> I am also unclear why this is public.

This could be a function-scoped static bool instead of a class-scoped static bool because it is only used in one function.

[Chris Dumez]

(In reply to Alex Christensen from comment #22)
> Comment on attachment 433565 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433565&action=review
> 
> >> Source/WebCore/html/DOMFormData.h:58
> >> +    inline static bool formDataDispatched = false;
> > 
> > No inline here. I don't think using a static variable makes sense either. Imagine 2 tabs sharing the same process, I don't think you'd want the formdata dispatch for one page to be impacted by the formdata dispatch in another.
> > 
> > Also, the name should be something like "isDispatchingFormDataEvent".
> > 
> > I am also unclear why this is public.
> 
> This could be a function-scoped static bool instead of a class-scoped static
> bool because it is only used in one function.

As I said, it should not be a static at all. It doesn't make sense.

[Qiaosong Zhou]

(In reply to Chris Dumez from comment #23)
> As I said, it should not be a static at all. It doesn't make sense.

I have removed the static variable. Instead I'm using a threadGlobalData member variable to address the problem of threads sharing static variables. I hope that's the optimal solution here?

[Qiaosong Zhou]

(In reply to Chris Dumez from comment #20)
> > Source/WebCore/html/DOMFormData.cpp:63
> > +        return dispatchFormDataEvent(adoptRef(*new DOMFormData(form)), *form);
> 
> Why aren't you using DOMFormData::create(form) ?
> 
> Also, if we should to leave the cloning up to the caller, then it would look
> like:
> if (form) {
>     auto formData = DOMFormData::create(form);
>     auto dispatchResult = dispatchFormDataEvent(formData, *form);
>     if (dispatchResult.hasException())
>         return dispatchResult.releaseException();
>     return formData->clone();
> }
> 
> > Source/WebCore/html/DOMFormData.cpp:77
> > +        return DOMFormData::create(WTFMove(formData));
> 
> Given that not all call sites seem to want cloning, we should probably
> return { }; return and have the return type be ExceptionOr<void>.

The reason that I have to clone for in this case is that if the handler for FormDataEvent stores the DOMFormData object in the outer scope, the correct behavior is that changing the outer scope reference outside of the event handler should not change the original DOMFormData object. To fall in line with that, I clone the DOMFormData after the event is dispatched.


> > Source/WebCore/html/DOMFormData.cpp:88
> > +Ref<DOMFormData> DOMFormData::create(Ref<DOMFormData>&& other)
> 
> It feels weird to have a factory function on DOMFormData that takes in a
> Ref<DOMFormData>&& in. Shouldn't we simply add a "clone()" member function
> to DOMFormData?

Yes I will apply that change. 

> 
> > Source/WebCore/loader/FormSubmission.cpp:230
> > +    DOMFormData::dispatchFormDataEvent(domFormData.copyRef(), form);
> 
> How come we're not using the returned DOMFormData here?
> 
> If this call site doesn't need to clone, then dispatchFormDataEvent() should
> probably not do the cloning and we should leave it up to the caller.

Same as above, I feel like cloning should be mandatory to prevent illegal tampering with DOMFormData from outside of the event handler.

[Qiaosong Zhou]

Specifically, the last two asserts for test "Newly created FormData contains entries added to "formData" IDL attribute of FormDataEvent." on http://wpt.live/xhr/formdata.html seems to mandate cloning.

[Kent Tamura]

*** Bug 193231 has been marked as a duplicate of this bug. ***

[Qiaosong Zhou]

Created attachment 433772 [details]
Patch

[Qiaosong Zhou]

Created attachment 433773 [details]
Patch

[Qiaosong Zhou]

Created attachment 433775 [details]
Patch

[Qiaosong Zhou]

Created attachment 433782 [details]
Patch

[Chris Dumez]

Comment on attachment 433782 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:71
> +    if (threadGlobalData().isFormDataEventDispatched())

This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions.

> Source/WebCore/platform/network/FormData.cpp:278
> +        // FIXME: Rmove the following if statement when fixed.

Typo: Remove

[Chris Dumez]

Comment on attachment 433782 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:92
> +    for (auto& item : this->items())

Any reason we cannot simply do:
```
newFormData->m_items = m_items;
```
?

[Chris Dumez]

Comment on attachment 433782 [details]
Patch

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

>> Source/WebCore/html/DOMFormData.cpp:71
>> +    if (threadGlobalData().isFormDataEventDispatched())
> 
> This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions.

We likely want a flag on the HTMLFormElement or FormSubmission instead.

[Alex Christensen]

Comment on attachment 433782 [details]
Patch

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

>>> Source/WebCore/html/DOMFormData.cpp:71
>>> +    if (threadGlobalData().isFormDataEventDispatched())
>> 
>> This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions.
> 
> We likely want a flag on the HTMLFormElement or FormSubmission instead.

It looks like this needs to move to DOMFormData::create, but I don't think this can be accomplished with a flag on HTMLFormElement or FormSubmission.  This needs to change what DOMFormData::create returns if it is called inside the stack trace of dispatching the FormDataEvent.

[Chris Dumez]

Comment on attachment 433782 [details]
Patch

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

>>>> Source/WebCore/html/DOMFormData.cpp:71
>>>> +    if (threadGlobalData().isFormDataEventDispatched())
>>> 
>>> This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions.
>> 
>> We likely want a flag on the HTMLFormElement or FormSubmission instead.
> 
> It looks like this needs to move to DOMFormData::create, but I don't think this can be accomplished with a flag on HTMLFormElement or FormSubmission.  This needs to change what DOMFormData::create returns if it is called inside the stack trace of dispatching the FormDataEvent.

The form is passed as parameter here so I don't see why a flag on the form wouldn't work. Note that as far as I can tell, Blink is using a flag on the form.

[Chris Dumez]

Comment on attachment 433782 [details]
Patch

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

>>>>> Source/WebCore/html/DOMFormData.cpp:71
>>>>> +    if (threadGlobalData().isFormDataEventDispatched())
>>>> 
>>>> This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions.
>>> 
>>> We likely want a flag on the HTMLFormElement or FormSubmission instead.
>> 
>> It looks like this needs to move to DOMFormData::create, but I don't think this can be accomplished with a flag on HTMLFormElement or FormSubmission.  This needs to change what DOMFormData::create returns if it is called inside the stack trace of dispatching the FormDataEvent.
> 
> The form is passed as parameter here so I don't see why a flag on the form wouldn't work. Note that as far as I can tell, Blink is using a flag on the form.

For the record, this is the section of the spec where this is specified:
https://html.spec.whatwg.org/#constructing-form-data-set:formdataevent
"Set form's firing submission events to true." / "Set form's firing submission events to false."

In particular this flag:
https://html.spec.whatwg.org/#constructing-entry-list

Which is a flag on the form.

Please match the spec exactly.

[Alex Christensen]

Oh, I see.  Not all FormData constructors will fail inside of that event, just the ones using that FormData with the flag.  You're right.

[Chris Dumez]

Comment on attachment 433782 [details]
Patch

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

>>>>>> Source/WebCore/html/DOMFormData.cpp:71
>>>>>> +    if (threadGlobalData().isFormDataEventDispatched())
>>>>> 
>>>>> This is not any different than a static and still incorrect behavior. I don't think 2 pages in the same process should impact each other's for submissions.
>>>> 
>>>> We likely want a flag on the HTMLFormElement or FormSubmission instead.
>>> 
>>> It looks like this needs to move to DOMFormData::create, but I don't think this can be accomplished with a flag on HTMLFormElement or FormSubmission.  This needs to change what DOMFormData::create returns if it is called inside the stack trace of dispatching the FormDataEvent.
>> 
>> The form is passed as parameter here so I don't see why a flag on the form wouldn't work. Note that as far as I can tell, Blink is using a flag on the form.
> 
> For the record, this is the section of the spec where this is specified:
> https://html.spec.whatwg.org/#constructing-form-data-set:formdataevent
> "Set form's firing submission events to true." / "Set form's firing submission events to false."
> 
> In particular this flag:
> https://html.spec.whatwg.org/#constructing-entry-list
> 
> Which is a flag on the form.
> 
> Please match the spec exactly.

And from the spec, it looks like this flag is being checked when submitting a form, not when constructing a DOMFormData, so this is definitely the wrong place. See https://html.spec.whatwg.org/#form-submission-algorithm:constructing-entry-list

[Qiaosong Zhou]

Created attachment 433817 [details]
Patch

[Chris Dumez]

Comment on attachment 433817 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:71
> +    if (form.formDataEventDispatched())

As mentioned earlier, this is not the right place to check this. The name of the flag is also wrong. Please match the spec text.

[Chris Dumez]

(In reply to Chris Dumez from comment #41)
> Comment on attachment 433817 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433817&action=review
> 
> > Source/WebCore/html/DOMFormData.cpp:71
> > +    if (form.formDataEventDispatched())
> 
> As mentioned earlier, this is not the right place to check this. The name of
> the flag is also wrong. Please match the spec text.

Looking at the spec for the FormData constructor here:
- https://xhr.spec.whatwg.org/#dom-formdata

It does appear that it calls "construct an entry-list" (https://html.spec.whatwg.org/#constructing-entry-list), which does check the flag.

So I think we need to try and match the spec a bit more closely for clarity.

Ideally, we'd have a constructEntryList() function which is called both from the FormData constructor and form submission and this function would return early if the flag is set.

And we would rename the flag to match the spec more exactly. While the behavior now is properly more or less correct, it will be much more obvious once our implementation matches for directly the spec language.

[Qiaosong Zhou]

Created attachment 433833 [details]
Patch

[Qiaosong Zhou]

Created attachment 433845 [details]
Patch

[Qiaosong Zhou]

Created attachment 433882 [details]
Patch

[Qiaosong Zhou]

Created attachment 433894 [details]
Patch

[Qiaosong Zhou]

Created attachment 433895 [details]
Patch

[Chris Dumez]

Comment on attachment 433895 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:71
> +    if (form.formDataEventDispatched())

Still not matching the spec. There is no such flag in the spec, as mentioned earlier.

There is a flag in the spec but it is not about the form data event being dispatched, it is about constructing an entry list.

[Qiaosong Zhou]

Created attachment 433980 [details]
Patch

[Chris Dumez]

Comment on attachment 433980 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:59
> +

May be good to have a link to the corresponding spec section here as a comment:
// https://html.spec.whatwg.org/C/#constructing-the-form-data-set

> Source/WebCore/html/DOMFormData.cpp:60
> +ExceptionOr<Ref<DOMFormData>> DOMFormData::constructEntryList(HTMLFormElement& form, StringPairVector* formValues, bool isMultiPartForm, bool& containsPasswordData)

Could we move this to HTMLFormElement ? This way, we don't need to pass in a form.

Also, we probably won't need the public isConstructingEntryList() / setIsConstructingEntryList() on HTMLFormElement.

> Source/WebCore/html/DOMFormData.cpp:64
> +    

I think this function is supposed to:
1. Construct a new DOMFormData object
2. Do your loop below which populates the DOMFormData object
3. dispatch the FormDataEvent
4. Return the DOMFormData object

> Source/WebCore/html/DOMFormData.cpp:65
> +    form.setIsConstructingEntryList(true);

Once you move this to HTMLFormElement, please use WTF::SetForScope to change the value of m_isConstructingEntryList.

> Source/WebCore/html/DOMFormData.cpp:97
> +void DOMFormData::dispatchFormDataEvent(Ref<DOMFormData>&& formData, HTMLFormElement& form)

We probably don't need a whole function for this. We can probably just merge into constructEntryList() as a one liner:
dispatchEvent(FormDataEvent::create(eventNames().formdataEvent, Event::CanBubble::Yes, Event::IsCancelable::No, Event::IsComposed::No, formData.copyRef()));

> Source/WebCore/html/DOMFormData.cpp:113
> +    

Unnecessary blank line.

> Source/WebCore/html/DOMFormData.h:56
> +    static void dispatchFormDataEvent(Ref<DOMFormData>&&, HTMLFormElement&);

Not needed.

> Source/WebCore/html/HTMLFormElement.cpp:402
> +    auto result = FormSubmission::create(*this, submitter, m_attributes, event, shouldLockHistory, trigger);

I don't think HTMLFormElement::submit() is supposed to throw an exception. I also don't think that it is possible that the isConstructingEntryList flag is true. I think constructEntryListList() should just return a RefPtr<> and let the call-site decide if they want to throw or not. In the DOMFormData constructor case, we'd throw if constructEntryList() returns null. In the form submission case, we'd RELEASE_ASSERT() that the value is non-null.

> Source/WebCore/loader/FormSubmission.cpp:217
> +    if (entryListResult.hasException()) {

I don't think this case is possible, I think we should just RELEASE_ASSERT() that constructEntryList returned a valid domFormData.

[Chris Dumez]

Comment on attachment 433980 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:63
> +        return Exception { InvalidStateError, "Already constructing Form entry list."_s };

I think we should just return null here, like in the spec text and let the DOMFormData constructor throw the exception if it get null.

>> Source/WebCore/html/DOMFormData.cpp:64
>> +    
> 
> I think this function is supposed to:
> 1. Construct a new DOMFormData object
> 2. Do your loop below which populates the DOMFormData object
> 3. dispatch the FormDataEvent
> 4. Return the DOMFormData object

To clarify, for step 4, I meant return a *clone* of the DOMFormData, as per https://html.spec.whatwg.org/C/#constructing-the-form-data-set

[Qiaosong Zhou]

Created attachment 434044 [details]
Patch

[Qiaosong Zhou]

Created attachment 434057 [details]
Patch

[Qiaosong Zhou]

Created attachment 434080 [details]
Patch

[Alex Christensen]

Comment on attachment 434080 [details]
Patch

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

Looks like you addressed Chris's comments.  I think it looks good.  A few style things and I'd say it's ready.

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

nit: space below

> Source/WebCore/html/DOMFormData.cpp:36
> +#include "ThreadGlobalData.h"

This isn't used any more.

> Source/WebCore/html/HTMLFormElement.h:128
> +    RefPtr<DOMFormData> constructEntryList(Ref<DOMFormData>&&, StringPairVector*, bool, bool&);

Style nit:
It would be nicer if this didn't have an out-param and used 2-state enums instead of bools to make the call sites more readable.  Like this:
enum class IsMultiPartForm : bool { No,  Yes };
enum class ContainsPasswordData : bool { No, Yes };
std::pair<RefPtr<DOMFormData>, ContainsPasswordData> constructEntryList(Ref<DOMFormData>&&, StringPairVector*, IsMultiPartForm);

[Chris Dumez]

Comment on attachment 434080 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:52
> +    auto result = form->constructEntryList(WTFMove(domFormData), nullptr, true, temp);

Why is it correct to unconditionally pass isMultiPartForm=true here? I might be, I just don't know why.

> Source/WebCore/html/HTMLFormElement.cpp:973
> +    SetForScope<bool> isConstructingEntryListRestorer(m_isConstructingEntryList, true);

I'd name this: "isConstructingEntryListScope". We usually use "Scope" in the name for such things.

> Source/WebCore/loader/FormSubmission.cpp:215
> +    auto result = form.constructEntryList(WTFMove(domFormData), &formValues, isMultiPartForm, containsPasswordData);

Could constructEntryList() call fomFormData->setContainsPasswordData() by itself instead of requiring an out-parameter?

[Chris Dumez]

Comment on attachment 434080 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:51
> +    bool temp;

FYI, temp is a terrible name and I would have used to actual parameter name here. That said, I think we should be able to get rid of this out-parameter entirely with my earlier suggestion.

>> Source/WebCore/html/DOMFormData.cpp:52
>> +    auto result = form->constructEntryList(WTFMove(domFormData), nullptr, true, temp);
> 
> Why is it correct to unconditionally pass isMultiPartForm=true here? I might be, I just don't know why.

It is probably fine. Looks like it is only used by FileInputType::appendFormData() for something that is likely obsolete. Passing true is likely OK but I'd switch to an enum class for clarity, like Alex mentioned.

[Qiaosong Zhou]

Created attachment 434114 [details]
Patch

[Qiaosong Zhou]

Created attachment 434115 [details]
Patch

[Alex Christensen]

Comment on attachment 434115 [details]
Patch

Looks good to me.  I'll let Chis double check it.

[Geoffrey Garen]

Comment on attachment 434115 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.h:132
> +    bool containsPasswordData() const { return m_containsPasswordData; }

As far as I can tell, nobody ever consults the value of containsPasswordData. (And, the value is lost accidentally when serializing across processes in FormData::encode(), and when serializing across threads in FormData::isolatedCopy(), consistent with a theory that it may be vestigial and unused.)

I think you can remove containsPasswordData, and the associated code you need to modify in order to maintain it. Why don't you give that a shot and see if it builds.

> Source/WebCore/platform/network/FormData.cpp:278
> +        // FIXME: DOMFormData created in FormData Event callback with text/plain enctype will crash here because values can be file. The expected behavior is to convert files to string for enctype "text/plain". Conversion may be added at "void DOMFormData::set(const String& name, Blob& blob, const String& filename)" or here.
> +        // FIXME: Remove the following if statement when fixed.

I would say just:

// FIXME: if the item is a file, we are expected to converted the file to a string.

No need to mention that a crash is how you started debugging; what you've learned since then is most helpful to the next reader.

[Qiaosong Zhou]

Created attachment 434149 [details]
Patch

[Chris Dumez]

Comment on attachment 434149 [details]
Patch

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

r=me with one comment.

> Source/WebCore/platform/network/FormData.cpp:278
> +        if (!WTF::holds_alternative<String>(item.data))

Is this really still needed? I thought my proposal to do `newFormData->m_items = m_items;` would avoid this.

[Qiaosong Zhou]

(In reply to Chris Dumez from comment #63)
> Comment on attachment 434149 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434149&action=review
> 
> r=me with one comment.
> 
> > Source/WebCore/platform/network/FormData.cpp:278
> > +        if (!WTF::holds_alternative<String>(item.data))
> 
> Is this really still needed? I thought my proposal to do
> `newFormData->m_items = m_items;` would avoid this.

It is still needed. It is related to another issue, where the current formdata does not process forms with enctype="text/plain" properly. It's supposed to automatically convert files to string if the form is "text/plain". Currently it is possible to receive a file here, causing a crash when the variant is unparsed as a string. (further down the line)

The specific test case that causes a crash is http://wpt.live/html/semantics/forms/form-submission-0/text-plain.window.html

[Chris Dumez]

(In reply to Qiaosong Zhou from comment #64)
> (In reply to Chris Dumez from comment #63)
> > Comment on attachment 434149 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=434149&action=review
> > 
> > r=me with one comment.
> > 
> > > Source/WebCore/platform/network/FormData.cpp:278
> > > +        if (!WTF::holds_alternative<String>(item.data))
> > 
> > Is this really still needed? I thought my proposal to do
> > `newFormData->m_items = m_items;` would avoid this.
> 
> It is still needed. It is related to another issue, where the current
> formdata does not process forms with enctype="text/plain" properly. It's
> supposed to automatically convert files to string if the form is
> "text/plain". Currently it is possible to receive a file here, causing a
> crash when the variant is unparsed as a string. (further down the line)
> 
> The specific test case that causes a crash is
> http://wpt.live/html/semantics/forms/form-submission-0/text-plain.window.html

Ok.

[EWS]

Committed r280310 (239960@main): <https://commits.webkit.org/239960@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434149 [details].

[Darin Adler]

Comment on attachment 434149 [details]
Patch

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

> Source/WebCore/dom/FormDataEvent.h:45
> +    DOMFormData& formData() const { return m_formData.get(); }

Seems like this should return a const& for conceptual const-ness.

> Source/WebCore/html/DOMFormData.cpp:46
> +    auto domFormData = adoptRef(*new DOMFormData());

I think the local variable should just be named formData or data. Also no need for the ().

> Source/WebCore/html/DOMFormData.cpp:48
> +        return domFormData;

This should have a WTFMove() because the return value optimization doesn’t work if we don’t have types that match exactly.

> Source/WebCore/html/DOMFormData.h:33
> +#include "ExceptionOr.h"

Should not need to include this. A forward declaration should be sufficient.

> Source/WebCore/html/DOMFormData.h:35
> +#include "FormState.h"

Should not need to include this. Not clear why it’s needed.

> Source/WebCore/html/DOMFormData.h:68
> +    Ref<DOMFormData> clone();

Should be const.

[Qiaosong Zhou]

Reopening to attach new patch.

[Qiaosong Zhou]

Created attachment 434254 [details]
Patch

[Chris Dumez]

Comment on attachment 434254 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:46
> +    auto data = adoptRef(*new DOMFormData);

I'd much prefer "formData" to the overly generic "data" here.

[Qiaosong Zhou]

Created attachment 434255 [details]
Patch

[EWS]

Committed r280332 (239979@main): <https://commits.webkit.org/239979@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434255 [details].

[Darin Adler]

Comment on attachment 434149 [details]
Patch

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

>> Source/WebCore/html/DOMFormData.cpp:48
>> +        return domFormData;
> 
> This should have a WTFMove() because the return value optimization doesn’t work if we don’t have types that match exactly.

According to r280376, I was wrong about this. I would like to find out more about why I was wrong!

[Qiaosong Zhou]

(In reply to Darin Adler from comment #73)
> Comment on attachment 434149 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434149&action=review
> 
> >> Source/WebCore/html/DOMFormData.cpp:48
> >> +        return domFormData;
> > 
> > This should have a WTFMove() because the return value optimization doesn’t work if we don’t have types that match exactly.
> 
> According to r280376, I was wrong about this. I would like to find out more
> about why I was wrong!

I think return optimization implicitly moves the rvalue. This link explains it better:  https://stackoverflow.com/questions/17473753/c11-return-value-optimization-or-move