Bug 176719 – WKBundlePageWillSendSubmitEventCallback is called with incorrect frame parameter

• Component: WebKit API
• Status: RESOLVED FIXED

[Michael Catanzaro]

The definition for WKBundlePageWillSendSubmitEventCallback is this:

typedef void (*WKBundlePageWillSendSubmitEventCallback)(WKBundlePageRef page, WKBundleNodeHandleRef htmlFormElementHandle, WKBundleFrameRef frame, WKBundleFrameRef sourceFrame, WKDictionaryRef values, const void* clientInfo);

It's clearly intended to parallel WKBundlePageWillSubmitFormCallback, since almost all the parameters are the same. Now, in WKBundlePageWillSubmitFormCallback, the first WKBundleFrameRef, "frame", is the frame of the form target, and the second WKBundleFrameRef, sourceFrame, is the frame containing the form. That's correct. But in WKBundlePageWillSendSubmitEventCallback, both frame and sourceFrame are always identical. The problem is that the FrameLoaderClient delegate is called on the wrong FrameLoaderClient. It should be called on the FrameLoaderClient of the target frame, but HTMLFormElement calls it on the FrameLoaderClient of the source frame instead.

The forthcoming patch fixes this and adds a test. Since the change is in WebCore, it *technically* doesn't require owner approval, but it would certainly be good for someone from Apple to review it please.

[Michael Catanzaro]

Created attachment 320455 [details]
Patch

[Michael Catanzaro]

I think it broke fast/forms/input-image-submit.html... waiting for the EWS results.

[Build Bot]

Comment on attachment 320455 [details]
Patch

Attachment 320455 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4514623

New failing tests:
fast/forms/input-image-submit.html

[Build Bot]

Created attachment 320463 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320455 [details]
Patch

Attachment 320455 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4514876

New failing tests:
fast/forms/input-image-submit.html

[Build Bot]

Created attachment 320464 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Michael Catanzaro]

Created attachment 320465 [details]
Patch

[Chris Dumez]

Comment on attachment 320465 [details]
Patch

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

Good fix but I have a suggestion to make the fix a bit cleaner.

> Source/WebCore/html/HTMLFormElement.cpp:281
> +    auto temporarySubmission = FormSubmission::create(*this, m_attributes, &event, LockHistory::Yes, NotSubmittedByJavaScript);

I do not like this part. I would suggest:
1. Add an effectiveTarget() method to HTMLFormElement which uses the submitButton's formtarget attribute is available, falls back to HTMLFormElement::target() and then falls back to document().baseTarget().
2. Use  effectiveTarget() here instead of constructing a FormSubmission
3. Use effectiveTarget() in FormSubmission::create() to avoid code duplication.

[Michael Catanzaro]

Thanks for the review, Chris, and sorry for the delay. I think/hope I've properly implemented your suggestion. Let's see if it passes EWS.

[Michael Catanzaro]

Created attachment 324777 [details]
Patch

[Build Bot]

Comment on attachment 324777 [details]
Patch

Attachment 324777 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4979053

New failing tests:
fast/forms/formtarget-attribute-button-html.html
fast/forms/formtarget-attribute-input-html.html
fast/forms/formtarget-attribute-input-2.html

[Build Bot]

Created attachment 324784 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #9)
> Thanks for the review, Chris, and sorry for the delay. I think/hope I've
> properly implemented your suggestion. Let's see if it passes EWS.

Signs point to "no"

[Build Bot]

Comment on attachment 324777 [details]
Patch

Attachment 324777 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4979047

New failing tests:
fast/forms/formtarget-attribute-button-html.html
fast/forms/formtarget-attribute-input-html.html

[Build Bot]

Created attachment 324788 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Build Bot]

Comment on attachment 324777 [details]
Patch

Attachment 324777 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4979226

New failing tests:
fast/forms/formtarget-attribute-button-html.html
fast/forms/formtarget-attribute-input-html.html
fast/forms/formtarget-attribute-input-2.html

[Build Bot]

Created attachment 324789 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 324777 [details]
Patch

Attachment 324777 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4984334

New failing tests:
fast/forms/formtarget-attribute-button-html.html
fast/forms/formtarget-attribute-input-html.html
fast/forms/formtarget-attribute-input-2.html

[Build Bot]

Created attachment 324840 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Michael Catanzaro]

Created attachment 324891 [details]
Patch

[Michael Catanzaro]

OK, looks better now. mac-debug is still orange, but the failures there appear to be unrelated.

[Build Bot]

Comment on attachment 324891 [details]
Patch

Attachment 324891 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4990660

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 324928 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Michael Catanzaro]

Comment on attachment 324891 [details]
Patch

Seems the mac-wk2 EWS is having some unrelated problems with Apache.

[Michael Catanzaro]

Pinging cdumez :)

[Chris Dumez]

Comment on attachment 324891 [details]
Patch

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

r=me

> Source/WebCore/html/HTMLFormElement.cpp:671
> +        for (auto node = event->target()->toNode(); node; node = node->parentNode()) {

assuming toNode() returns a raw pointer, I'd prefer auto* than auto. But really, I think this should use:
auto* targetNode = event->target()->toNode();
return targetNode ? lineageOfType<HTMLFormControlElement>(*targetNode)).first() : nullptr;

[Michael Catanzaro]

Question!

(In reply to Chris Dumez from comment #26)
> Comment on attachment 324891 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324891&action=review
> 
> r=me
> 
> > Source/WebCore/html/HTMLFormElement.cpp:671
> > +        for (auto node = event->target()->toNode(); node; node = node->parentNode()) {
> 
> assuming toNode() returns a raw pointer, I'd prefer auto* than auto.

It's a RefPtr<Node>, so it has to be plain auto.

> But
> really, I think this should use:
> auto* targetNode = event->target()->toNode();
> return targetNode ?
> lineageOfType<HTMLFormControlElement>(*targetNode)).first() : nullptr;

I failed to get this to work, because lineageOfType requires an Element, not a Node. I messed around a bit and came up with this:

HTMLFormControlElement* HTMLFormElement::findSubmitButton(const Event* event) const
{
    if (event && event->target()) {
        if (auto targetNode = event->target()->toNode()) {
            if (auto* element = downcast<Element>(targetNode.get()))
                return lineageOfType<HTMLFormControlElement>(*element).first();
        }
    }
    return nullptr;
}

But that breaks fast/forms/form-submission-create-crash.xhtml:

ASSERTION FAILED: !source || is<Target>(*source)
../../Source/WTF/wtf/TypeCasts.h(89) : typename WTF::match_constness<Source, Target>::type* WTF::downcast(Source*) [with Target = WebCore::Element; Source = WebCore::Node; typename WTF::match_constness<Source, Target>::type = WebCore::Element]

Not sure what to do here. Is the existing version of the code OK?

[Chris Dumez]

Comment on attachment 324891 [details]
Patch

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

>>> Source/WebCore/html/HTMLFormElement.cpp:671
>>> +        for (auto node = event->target()->toNode(); node; node = node->parentNode()) {
>> 
>> assuming toNode() returns a raw pointer, I'd prefer auto* than auto. But really, I think this should use:
>> auto* targetNode = event->target()->toNode();
>> return targetNode ? lineageOfType<HTMLFormControlElement>(*targetNode)).first() : nullptr;
> 
> It's a RefPtr<Node>, so it has to be plain auto.

Oh, it is true that lineageOfType() expects an element in parameter :/ Just leave as is then. You are merely moving this code anyway.

[Chris Dumez]

(In reply to Michael Catanzaro from comment #27)
> Question!
> 
> (In reply to Chris Dumez from comment #26)
> > Comment on attachment 324891 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=324891&action=review
> > 
> > r=me
> > 
> > > Source/WebCore/html/HTMLFormElement.cpp:671
> > > +        for (auto node = event->target()->toNode(); node; node = node->parentNode()) {
> > 
> > assuming toNode() returns a raw pointer, I'd prefer auto* than auto.
> 
> It's a RefPtr<Node>, so it has to be plain auto.
> 
> > But
> > really, I think this should use:
> > auto* targetNode = event->target()->toNode();
> > return targetNode ?
> > lineageOfType<HTMLFormControlElement>(*targetNode)).first() : nullptr;
> 
> I failed to get this to work, because lineageOfType requires an Element, not
> a Node. I messed around a bit and came up with this:
> 
> HTMLFormControlElement* HTMLFormElement::findSubmitButton(const Event*
> event) const
> {
>     if (event && event->target()) {
>         if (auto targetNode = event->target()->toNode()) {
>             if (auto* element = downcast<Element>(targetNode.get()))
>                 return
> lineageOfType<HTMLFormControlElement>(*element).first();

Yes, this is an unsafe downcast as we don't know that the Node is an Element.

[Michael Catanzaro]

Does downcast not do any type checking? I guess that's why it's not dynamic_cast. Foolish me!

[Michael Catanzaro]

Committed r224206: <https://trac.webkit.org/changeset/224206>

[Radar WebKit Bug Importer]

<rdar://problem/35569037>