Bug 103592 – DRT - crashed in WebCore::SearchFieldCancelButtonElement::defaultEventHandler

• Component: UI Events
• Status: CLOSED FIXED

[Xiaobo Wang]

Got a crash when running DRT test fast/forms/search-delete-while-cancel-button-clicked.html on BlackBerry platform. It doesn't reproduce 100%, but it did happen from time to time.

Back Trace
==========
#0  WebCore::SearchFieldCancelButtonElement::defaultEventHandler
(this=0x4fca4d0, event=0x4eba738)
    at
/home/yanbin/workspace/playbook/webkit/Source/WebCore/html/shadow/TextControlInnerElements.cpp:206
#1  0x7a4f304e in dispatchEventPostProcess (preDispatchEventHandlerResult=0x0,
event=..., this=0xdfee38)
    at
/home/yanbin/workspace/playbook/webkit/Source/WebCore/dom/EventDispatcher.cpp:340
#2  WebCore::EventDispatcher::dispatchEvent (this=0xdfee38, prpEvent=...)
    at
/home/yanbin/workspace/playbook/webkit/Source/WebCore/dom/EventDispatcher.cpp:255
#3  0x7a4f9596 in dispatchEvent (dispatcher=0xdfee38, this=0x4e66e00) at
/home/yanbin/workspace/playbook/webkit/Source/WebCore/dom/MouseEvent.cpp:215
#4  WebCore::MouseEventDispatchMediator::dispatchEvent (this=0x4e66e00,
dispatcher=0xdfee38)
    at
/home/yanbin/workspace/playbook/webkit/Source/WebCore/dom/MouseEvent.cpp:204
#5  0x7a4f2770 in WebCore::EventDispatcher::dispatchEvent (node=<optimized
out>, mediator=...)
    at
/home/yanbin/workspace/playbook/webkit/Source/WebCore/dom/EventDispatcher.cpp:128
#6  0x79daab66 in WebCore::Node::dispatchMouseEvent (this=0x4fca4d0, event=...,
eventType=..., detail=<optimized out>, relatedTarget=0x3a06618)
    at /home/yanbin/workspace/playbook/webkit/Source/WebCore/dom/Node.cpp:2664
#7  0x79f6d504 in WebCore::EventHandler::updateMouseEventTargetNode
(this=0x1cddd8, targetNode=<optimized out>, mouseEvent=..., 
    fireMouseOverOut=<optimized out>) at
/home/yanbin/workspace/playbook/webkit/Source/WebCore/page/EventHandler.cpp:2235
#8  0x79f6d6ae in WebCore::EventHandler::dispatchMouseEvent (this=0x1cddd8,
eventType=..., targetNode=0x3a06618, clickCount=0, mouseEvent=..., 
    setUnder=true) at
/home/yanbin/workspace/playbook/webkit/Source/WebCore/page/EventHandler.cpp:2252
#9  0x79f6e312 in WebCore::EventHandler::handleMouseMoveEvent (this=<optimized
out>, mouseEvent=..., hoveredNode=<optimized out>, 
    onlyUpdateScrollbars=<optimized out>) at
/home/yanbin/workspace/playbook/webkit/Source/WebCore/page/EventHandler.cpp:1827
#10 0x79f6f7f0 in WebCore::EventHandler::mouseMoved (this=0x1cddd8, event=...)
    at
/home/yanbin/workspace/playbook/webkit/Source/WebCore/page/EventHandler.cpp:1699
#11 0x79c909c6 in BlackBerry::WebKit::WebPagePrivate::handleMouseEvent
(this=0x1446d8, mouseEvent=...)
    at
/home/yanbin/workspace/playbook/webkit/Source/WebKit/blackberry/Api/WebPage.cpp:4021
#12 0x79c90bae in BlackBerry::WebKit::WebPage::mouseEvent (this=0x18a938,
mouseEvent=..., wheelDeltaAccepted=0x0)
    at
/home/yanbin/workspace/playbook/webkit/Source/WebKit/blackberry/Api/WebPage.cpp:3965
#13 0x79ce098c in mouseMoveToCallback (context=0x8e6090, function=<optimized
out>, thisObject=<optimized out>, argumentCount=<optimized out>, 
    arguments=0xdff49c, exception=0xdff510) at
/home/yanbin/workspace/playbook/webkit/Tools/DumpRenderTree/blackberry/EventSender.cpp:99
#14 0x7a8fe7b6 in JSC::JSCallbackFunction::call (exec=0x8e6090)
    at
/home/yanbin/workspace/playbook/webkit/Source/JavaScriptCore/API/JSCallbackFunction.cpp:73
#15 0x7a877f22 in JSC::JITStubThunked_op_call_NotJSFunction (args=0xdff578)
    at
/home/yanbin/workspace/playbook/webkit/Source/JavaScriptCore/jit/JITStubs.cpp:2275
#16 0x7a874b64 in cti_op_call_NotJSFunction () from libwebkit.so.0
#17 0x0011519a in ?? ()
#18 0x0011519a in ?? ()

The issue is when we handle the mouseout event in the search field cancel button, the input element has already been removed. See code below.

void SearchFieldCancelButtonElement::defaultEventHandler(Event* event)
{
    // If the element is visible, on mouseup, clear the value, and set
selection
    RefPtr<HTMLInputElement>
input(static_cast<HTMLInputElement*>(shadowHost()));
    /**********BUG: input is null if the input element has been removed.*****/
    if (input->disabled() || input->readOnly()) {
        if (!event->defaultHandled())
            HTMLDivElement::defaultEventHandler(event);
        return;
    }
    ...
}

Further investigation shows we're trying to send mouseout event to the old node AFTER it has
been detached, see log below. Note the log below is from a DRT run with NO crash.

Output of instrumented code
============================
Debug - EventHandler::handleMouseMoveEvent()
Debug - newSubframe = 0
Debug - EventHandler::updateMouseEventTargetNode(): m_nodeUnderMouse is 0
Debug - EventHandler::handleMouseMoveEvent()
Debug - newSubframe = 0
Debug - EventHandler::updateMouseEventTargetNode(): m_nodeUnderMouse=5db2d8
(DIV, , 1)
Debug - SearchFieldCancelButtonElement::defaultEventHandler: input is 4e0300
Debug - SearchFieldCancelButtonElement::defaultEventHandler: input is 4e0300
Debug - EventHandler::updateMouseEventTargetNode(): m_nodeUnderMouse=5db2d8
(DIV, , 1)
Debug - SearchFieldCancelButtonElement::defaultEventHandler: input is 4e0300
Debug - EventHandler::setCapturingMouseEventsNode: 0x5db2d8
Debug - SearchFieldCancelButtonElement::detach(): m_capturing=1  <====== Cancel button detached.
Debug - EventHandler::setCapturingMouseEventsNode: 0x0
Debug - frame->eventHandler()->setCapturingMouseEventsNode(0)
Debug - EventHandler::handleMouseMoveEvent()
Debug - newSubframe = 0
Debug - EventHandler::updateMouseEventTargetNode(): m_nodeUnderMouse=1dd840
(INPUT, , 1)
Debug - SearchFieldCancelButtonElement::defaultEventHandler: input is 4e0300 <====== Called after detached, because it is the m_lastNodeUnderMouse.
Debug - EventHandler::updateMouseEventTargetNode(): m_lastNodeUnderMouse=5db2d8
(DIV, , 1)
Debug - EventHandler::updateMouseEventTargetNode(): m_nodeUnderMouse=1dd840
(INPUT, , 1)
Debug - EventHandler::updateMouseEventTargetNode(): m_nodeUnderMouse=1dd840
(INPUT, , 1)
Debug - EventHandler::updateMouseEventTargetNode(): m_nodeUnderMouse=1dd840
(INPUT, , 1)
This tests that events don't continue to target a search cancel button if it is
deleted while mouse is down. 

clicking in cancel

deleting search input

clicking button
button click!

[Xiaobo Wang]

Created attachment 176644 [details]
patch

The crash happened when we try to send mouseout event to the old node which has been detached. We should forget the detached old node.

[WebKit Review Bot]

Comment on attachment 176644 [details]
patch

Attachment 176644 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15023708

New failing tests:
fast/events/mouseout-dead-node.html
fast/events/mouseover-mouseout2.html
fast/events/mouseover-mouseout.html

[Xiaobo Wang]

Oops, looks like we still expect to dispatch message to the detached nodes. That's why the 3 tests failed. See https://bugs.webkit.org/show_bug.cgi?id=12918

Need another patch.

[Xiaobo Wang]

Created attachment 176687 [details]
patch - checking pointer

So that the other 3 tests can pass.

[Kent Tamura]

Comment on attachment 176687 [details]
patch - checking pointer

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

> Source/WebCore/ChangeLog:19
> +        (WebCore::SearchFieldCancelButtonElement::defaultEventHandler):
> +        (WebCore::InputFieldSpeechButtonElement::defaultEventHandler):

SearchFieldResultsButtonElement::defaultEventHandler should have a similar change.

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:200
> -    if (input->disabled() || input->readOnly()) {
> +    if (!input || input->disabled() || input->readOnly()) {

You have to update SearchFieldCancelButtonElement::willRespondToMouseClickEvents too.

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:285
> -    if (input->disabled() || input->readOnly()) {
> +    if (!input || input->disabled() || input->readOnly()) {

You have to update InputFieldSpeechButtonElement::willRespondToMouseClickEvents too.

[Kent Tamura]

Comment on attachment 176687 [details]
patch - checking pointer

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

> Source/WebCore/ChangeLog:3
> +        [BlackBerry] DRT - crashed on WebCore::SearchFieldCancelButtonElement

The change is platform-independent.  We should remove [BlackBerry] prefix from the summary.

[Xiaobo Wang]

Thanks for the good suggestions!

[Xiaobo Wang]

Created attachment 176718 [details]
patch - checking pointer in more places

[Xiaobo Wang]

platform=All

[Antonio Gomes]

Comment on attachment 176718 [details]
patch - checking pointer in more places

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

> Source/WebCore/ChangeLog:13
> +        The crash happened when the search field cancel button handles mouseout
> +        event after the search input was detached. When it happens the input
> +        element returned from shadowHost() is null, need to check the pointer
> +        before dereferencing.
> +        InputFieldSpeechButton and SearchFieldResultsButtonElement have the
> +        similar issue.

The patch itself looks good, but I would feel confident if you find out and described why it passes for other platforms without your fix.

[Alexey Proskuryakov]

Per bug 97395, it also crashes on Mac. Please dupe accordingly.

[Xiaobo Wang]

(In reply to comment #10)
> (From update of attachment 176718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176718&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        The crash happened when the search field cancel button handles mouseout
> > +        event after the search input was detached. When it happens the input
> > +        element returned from shadowHost() is null, need to check the pointer
> > +        before dereferencing.
> > +        InputFieldSpeechButton and SearchFieldResultsButtonElement have the
> > +        similar issue.
> 
> The patch itself looks good, but I would feel confident if you find out and described why it passes for other platforms without your fix.

Good question, I need more investigation for the answer:)

[George Staikos]

(In reply to comment #10)
> (From update of attachment 176718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176718&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        The crash happened when the search field cancel button handles mouseout
> > +        event after the search input was detached. When it happens the input
> > +        element returned from shadowHost() is null, need to check the pointer
> > +        before dereferencing.
> > +        InputFieldSpeechButton and SearchFieldResultsButtonElement have the
> > +        similar issue.
> 
> The patch itself looks good, but I would feel confident if you find out and described why it passes for other platforms without your fix.

Per Alexey it crashes Mac too.  It is flakey... it takes some work to trigger it.

[Xiaobo Wang]

Yes, it's very flaky. When we remove the input element with JavaScript "search.parentNode.removeChild(search)", the search type input control is detached from DOM tree. It's shadow nodes are also detached, but they're not removed. So in SearchFieldCancelButtonElement::defaultEventHandler(), we can still get the shadowRoot and ShadowHost. The shadow roots are removed on JavaScript GC, see back trace below. How and when GC is performed is platform dependent. This might be the reason why it didn't crash on other platforms.

Back trace when removing ShadowRoots for the input element
===========================================================
WebCore::ElementShadow::removeAllShadowRoots() at ElementShadow.cpp:102 0x794ae1a8	
WebCore::Element::~Element() at Element.cpp:142 0x794a8ed2	
WebCore::StyledElement::~StyledElement() at StyledElement.cpp:141 0x794cf650	
~HTMLElement() at HTMLElement.h:45 0x795750dc	
WebCore::LabelableElement::~LabelableElement() at LabelableElement.cpp:41 0x795750dc	
WebCore::HTMLFormControlElement::~HTMLFormControlElement() at HTMLFormControlElement.cpp:70 0x795460ca	
WebCore::HTMLFormControlElementWithState::~HTMLFormControlElementWithState() at HTMLFormControlElementWithState.cpp:42 0x79cdc9a6	
WebCore::HTMLTextFormControlElement::~HTMLTextFormControlElement() at HTMLTextFormControlElement.cpp:66 0x7956d874	
WebCore::HTMLInputElement::~HTMLInputElement() at HTMLInputElement.cpp:159 0x7954dc2c	
WebCore::HTMLInputElement::~HTMLInputElement() at HTMLInputElement.cpp:159 0x7954dd0c	
WebCore::Node::removedLastRef() at Node.cpp:2,884 0x794b8816	
deref() at TreeShared.h:81 0x798f7a3e	
releaseImpl() at JSNode.h:69 0x798f7a3e	
WebCore::JSNodeOwner::finalize() at JSNodeCustom.cpp:144 0x798f7a3e	
finalize() at WeakSetInlines.h:52 0x7a021284	
JSC::WeakBlock::sweep() at WeakBlock.cpp:80 0x7a021284	
JSC::WeakSet::sweep() at WeakSet.cpp:47 0x7a020fca	
JSC::MarkedBlock::sweep() at MarkedBlock.cpp:106 0x7a01d24a	
sweepNextBlock() at IncrementalSweeper.cpp:130 0x7a01bf3a	
JSC::IncrementalSweeper::doSweep() at IncrementalSweeper.cpp:104 0x7a01bf3a	
JSC::HeapTimer::timerDidFire() at HeapTimer.cpp:119 0x7a01bc84	
fired() at BlackBerryPlatformTimer.h:118 0x7a01bdee	

For the flakiness of GC, it's reasonable to do point checking in these places.

[Kent Tamura]

Comment on attachment 176718 [details]
patch - checking pointer in more places

ok

[WebKit Review Bot]

Comment on attachment 176718 [details]
patch - checking pointer in more places

Clearing flags on attachment: 176718

Committed r136309: <http://trac.webkit.org/changeset/136309>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 103819

[Mike West]

(In reply to comment #18)
> Re-opened since this is blocked by bug 103819

Sorry, I was mistaken! Closing this again.

[Xiaobo Wang]

close it.