Bug 190252 – Regression(r236779): Crash when changing the input element type from inside an 'input' event listener

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Crash when changing the input element type from inside an 'input' event listener:
Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Termination Signal:    Segmentation fault: 11
Termination Reason:    Namespace SIGNAL, Code 0xb
Terminating Process:   exc handler [7691]

VM Regions Near 0:
--> 
    __TEXT                 000000010eb67000-000000010eb69000 [    8K] r-x/rwx SM=COW  /Volumes/VOLUME/*/WebKit.framework/Versions/A/XPCServices/com.apple.WebKit.WebContent.xpc/Contents/MacOS/com.apple.WebKit.WebContent.Development

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000006ca50e32e WebCore::HTMLTextFormControlElement::dispatchFormControlChangeEvent() + 30 (HTMLTextFormControlElement.cpp:211)
1   com.apple.WebCore             	0x00000006ca35c518 WebCore::BaseCheckableInputType::fireInputAndChangeEvents() + 200 (BaseCheckableInputType.cpp:133)
2   com.apple.WebCore             	0x00000006ca361bcc WebCore::CheckboxInputType::didDispatchClick(WebCore::Event&, WebCore::InputElementClickState const&) + 252 (CheckboxInputType.cpp:92)
3   com.apple.WebCore             	0x00000006ca4281a1 WebCore::HTMLInputElement::didDispatchClickEvent(WebCore::Event&, WebCore::InputElementClickState const&) + 81 (HTMLInputElement.cpp:1128)
4   com.apple.WebCore             	0x00000006ca0f2a06 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) + 614 (EventDispatcher.cpp:165)
5   com.apple.WebCore             	0x00000006ca15d2dd WebCore::Node::dispatchEvent(WebCore::Event&) + 29 (Node.cpp:2383)
6   com.apple.WebCore             	0x00000006ca1dbfa5 WebCore::simulateMouseEvent(WTF::AtomicString const&, WebCore::Element&, WebCore::Event*, WebCore::SimulatedClickSource) + 149 (SimulatedClick.cpp:79)
7   com.apple.WebCore             	0x00000006ca1dbe94 WebCore::simulateClick(WebCore::Element&, WebCore::Event*, WebCore::SimulatedClickMouseEventOptions, WebCore::SimulatedClickVisualOptions, WebCore::SimulatedClickSource) + 436 (SimulatedClick.cpp:101)
8   com.apple.WebCore             	0x00000006ca3d8112 WebCore::HTMLElement::click() + 34 (HTMLElement.cpp:695)
9   com.apple.WebCore             	0x00000006c8ab80ea WebCore::jsHTMLElementPrototypeFunctionClickBody(JSC::ExecState*, WebCore::JSHTMLElement*, JSC::ThrowScope&) + 42 (JSHTMLElement.cpp:3118)
10  com.apple.WebCore             	0x00000006c8a7c906 long long WebCore::IDLOperation<WebCore::JSHTMLElement>::call<&(WebCore::jsHTMLElementPrototypeFunctionClickBody(JSC::ExecState*, WebCore::JSHTMLElement*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*) + 742 (JSDOMOperation.h:53)

[Chris Dumez]

Created attachment 351529 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 351529 [details]
Patch

Clearing flags on attachment: 351529

Committed r236803: <https://trac.webkit.org/changeset/236803>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44980354>

[Chris Dumez]

Reverted r236803 for reason:

Caused crashes on some bots

Committed r236837: <https://trac.webkit.org/changeset/236837>

[Chris Dumez]

Created attachment 351602 [details]
Patch

[Chris Dumez]

Comment on attachment 351602 [details]
Patch

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

> Source/WebCore/html/BaseCheckableInputType.cpp:130
> +    auto protectedThis = makeRef(*this);

Previous iteration was missing this protectedThis.

[Alex Christensen]

Comment on attachment 351602 [details]
Patch

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

> Source/WebCore/html/BaseCheckableInputType.cpp:134
> +    if (auto* element = this->element())
> +        element->dispatchFormControlChangeEvent();

It seems like this is a general JavaScript re-entry problem.  Maybe this should be done on the next runloop iteration?

[Chris Dumez]

(In reply to Alex Christensen from comment #8)
> Comment on attachment 351602 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351602&action=review
> 
> > Source/WebCore/html/BaseCheckableInputType.cpp:134
> > +    if (auto* element = this->element())
> > +        element->dispatchFormControlChangeEvent();
> 
> It seems like this is a general JavaScript re-entry problem.  Maybe this
> should be done on the next runloop iteration?

I do not think this would be Web-compliant as it would be observable.

Note that protecting |this| and null-checking element() is how the InputType code is currently dealing with this so my patch is consistent with existing code.

[Chris Dumez]

Comment on attachment 351602 [details]
Patch

Clearing flags on attachment: 351602

Committed r236841: <https://trac.webkit.org/changeset/236841>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 351602 [details]
Patch

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

>>> Source/WebCore/html/BaseCheckableInputType.cpp:134
>>> +        element->dispatchFormControlChangeEvent();
>> 
>> It seems like this is a general JavaScript re-entry problem.  Maybe this should be done on the next runloop iteration?
> 
> I do not think this would be Web-compliant as it would be observable.
> 
> Note that protecting |this| and null-checking element() is how the InputType code is currently dealing with this so my patch is consistent with existing code.

Though some parts of WebCore -- like layout -- forbid JavaScript execution, the default condition in WebCore is that it is generally possible, acceptable, and required to allow JavaScript execution.