Bug 176719

Summary: WKBundlePageWillSendSubmitEventCallback is called with incorrect frame parameter
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKit APIAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, bugs-noreply, buildbot, cdumez, cgarcia, mcatanzaro, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 173915    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Patch
cdumez: review+
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 none

Description Michael Catanzaro 2017-09-11 12:08:01 PDT
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.
Comment 1 Michael Catanzaro 2017-09-11 12:30:50 PDT
Created attachment 320455 [details]
Patch
Comment 2 Michael Catanzaro 2017-09-11 13:12:56 PDT
I think it broke fast/forms/input-image-submit.html... waiting for the EWS results.
Comment 3 Build Bot 2017-09-11 13:20:50 PDT
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
Comment 4 Build Bot 2017-09-11 13:20:51 PDT
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
Comment 5 Build Bot 2017-09-11 13:39:31 PDT
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
Comment 6 Build Bot 2017-09-11 13:39:33 PDT
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
Comment 7 Michael Catanzaro 2017-09-11 13:41:14 PDT
Created attachment 320465 [details]
Patch
Comment 8 Chris Dumez 2017-10-13 11:20:18 PDT
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.
Comment 9 Michael Catanzaro 2017-10-24 19:10:40 PDT
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.
Comment 10 Michael Catanzaro 2017-10-24 19:12:47 PDT
Created attachment 324777 [details]
Patch
Comment 11 Build Bot 2017-10-24 20:21:45 PDT
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
Comment 12 Build Bot 2017-10-24 20:21:47 PDT
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
Comment 13 Michael Catanzaro 2017-10-24 20:35:44 PDT
(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"
Comment 14 Build Bot 2017-10-24 20:39:52 PDT
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
Comment 15 Build Bot 2017-10-24 20:39:54 PDT
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
Comment 16 Build Bot 2017-10-24 21:04:01 PDT
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
Comment 17 Build Bot 2017-10-24 21:04:02 PDT
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
Comment 18 Build Bot 2017-10-25 10:15:01 PDT
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
Comment 19 Build Bot 2017-10-25 10:15:02 PDT
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
Comment 20 Michael Catanzaro 2017-10-25 13:59:34 PDT
Created attachment 324891 [details]
Patch
Comment 21 Michael Catanzaro 2017-10-25 16:44:41 PDT
OK, looks better now. mac-debug is still orange, but the failures there appear to be unrelated.
Comment 22 Build Bot 2017-10-25 17:27:29 PDT
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.
Comment 23 Build Bot 2017-10-25 17:27:30 PDT
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
Comment 24 Michael Catanzaro 2017-10-25 18:16:18 PDT
Comment on attachment 324891 [details]
Patch

Seems the mac-wk2 EWS is having some unrelated problems with Apache.
Comment 25 Michael Catanzaro 2017-10-30 07:22:03 PDT
Pinging cdumez :)
Comment 26 Chris Dumez 2017-10-30 09:45:20 PDT
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;
Comment 27 Michael Catanzaro 2017-10-30 14:16:08 PDT
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?
Comment 28 Chris Dumez 2017-10-30 14:20:19 PDT
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.
Comment 29 Chris Dumez 2017-10-30 14:21:00 PDT
(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.
Comment 30 Michael Catanzaro 2017-10-30 15:41:43 PDT
Does downcast not do any type checking? I guess that's why it's not dynamic_cast. Foolish me!
Comment 31 Michael Catanzaro 2017-10-30 15:42:27 PDT
Committed r224206: <https://trac.webkit.org/changeset/224206>
Comment 32 Radar WebKit Bug Importer 2017-11-15 13:13:27 PST
<rdar://problem/35569037>