Constructed FormData object should not contain an entry for the submit button that was used to submit the form

submit() in submit event handler should not contains a value of a submit button.

Created attachment 457255 [details]
Patch

[Darin Adler]

Comment on attachment 457255 [details]
Patch

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

> Source/WebCore/loader/FormSubmission.cpp:174
> -    RefPtr submitter = overrideSubmitter ? overrideSubmitter : form.findSubmitter(event);
> +    HTMLFormControlElement* submitter = overrideSubmitter ? overrideSubmitter : form.findSubmitter(event);

This does not seem like a good change. Why not use RefPtr for this local variable?

Created attachment 457519 [details]
Patch

[Chris Dumez]

Comment on attachment 457519 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.cpp:422
> +            if (element->form() == this && is<HTMLFormControlElement>(element)) {

Is the form check really useful? Doesn't a form associated element have this form as form by definition?

> Source/WebCore/html/HTMLFormElement.cpp:423
> +                HTMLFormControlElement& control = downcast<HTMLFormControlElement>(element.get());

auto&

> Source/WebCore/html/HTMLFormElement.cpp:432
>      auto firstSuccessfulSubmitButton = findSubmitButton(submitter, activateSubmitButton);

This looks unused now? or am I missing something?

> Source/WebCore/html/HTMLFormElement.cpp:1027
> +RefPtr<DOMFormData> HTMLFormElement::constructEntryList(HTMLFormControlElement* submitter, Ref<DOMFormData>&& domFormData, StringPairVector* formValues)

I would feel better if the submitter parameter was a RefPtr<>&&, given that you use it at the beginning of the function, then again at the end while running script in between.
I am particularly worried the submitter could get destroyed while running this function.

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

submitter.copyRef()

Created attachment 457604 [details]
Patch

Created attachment 457608 [details]
Patch

Created attachment 457610 [details]
Patch

[Chris Dumez]

Comment on attachment 457610 [details]
Patch

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

r=me with improvement suggestion.

> Source/WebCore/html/HTMLFormElement.cpp:422
> +            if (is<HTMLFormControlElement>(element)) {

if (auto* control = dynamicDowncast<HTMLFormControlElement>(element); control.isSuccessfulSubmitButton()) {
     submitter = control;
     break;
}

[Radar WebKit Bug Importer]

<rdar://problem/91892715>

Created attachment 458255 [details]
Patch

(In reply to Chris Dumez from comment #8)
> Comment on attachment 457610 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457610&action=review
> 
> r=me with improvement suggestion.
> 
> > Source/WebCore/html/HTMLFormElement.cpp:422
> > +            if (is<HTMLFormControlElement>(element)) {
> 
> if (auto* control = dynamicDowncast<HTMLFormControlElement>(element);
> control.isSuccessfulSubmitButton()) {
>      submitter = control;
>      break;
> }

Thanks for the review! I updated the lines but not exactly as you suggested. Please take another look.

[Tim Nguyen (:ntim)]

Comment on attachment 458255 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.cpp:422
> +            if (auto control = dynamicDowncast<HTMLFormControlElement>(element.get()); control->isSuccessfulSubmitButton()) {

auto* control = dynamicDowncast<HTMLFormControlElement>

dynamicDowncast always gives a pointer.

Chris, do we need `[...]; control && control->isSuccessfulSubmitButton()` ? or is `[...]; control->isSuccessfulSubmitButton()` enough?

Created attachment 458265 [details]
Patch

[Chris Dumez]

(In reply to Tim Nguyen (:ntim) from comment #12)
> Comment on attachment 458255 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458255&action=review
> 
> > Source/WebCore/html/HTMLFormElement.cpp:422
> > +            if (auto control = dynamicDowncast<HTMLFormControlElement>(element.get()); control->isSuccessfulSubmitButton()) {
> 
> auto* control = dynamicDowncast<HTMLFormControlElement>
> 
> dynamicDowncast always gives a pointer.
> 
> Chris, do we need `[...]; control && control->isSuccessfulSubmitButton()` ?
> or is `[...]; control->isSuccessfulSubmitButton()` enough?

Definitely `control && control->isSuccessfulSubmitButton()`.

[Chris Dumez]

Comment on attachment 458265 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.cpp:422
> +            if (auto* control = dynamicDowncast<HTMLFormControlElement>(element.get()); control->isSuccessfulSubmitButton()) {

Please add missing control null check.

[Darin Adler]

Comment on attachment 458265 [details]
Patch

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

>> Source/WebCore/html/HTMLFormElement.cpp:422
>> +            if (auto* control = dynamicDowncast<HTMLFormControlElement>(element.get()); control->isSuccessfulSubmitButton()) {
> 
> Please add missing control null check.

Maybe it’s obvious, but Chris means:

...; control && control->isSuccessfulSubmitButton()) {

Created attachment 458279 [details]
Patch

[Darin Adler]

I see now I just said the same thing Chris had already said!

[Tim Nguyen (:ntim)]

Is this ready to land? If it is, please set commit-queue+

[EWS]

Committed r293444 (249999@main): <https://commits.webkit.org/249999@main>

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

[Tim Nguyen (:ntim)]

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