Bug 197696 – [iOS Debug] ASSERTION FAILED: !m_originalNode in WebCore::JSLazyEventListener::checkValidityForEventTarget(WebCore::EventTarget &)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryan Haddad]

The following assertion failure is seen on iOS Simulator debug bots with layout tests fast/dom/event-handler-attributes.html and legacy-animation-engine/fast/dom/event-handler-attributes.html:

ASSERTION FAILED: !m_originalNode
./bindings/js/JSLazyEventListener.cpp(90) : virtual void WebCore::JSLazyEventListener::checkValidityForEventTarget(WebCore::EventTarget &)
1   0x46b3bd3b9 WTFCrash
2   0x46fe24cdb WTFCrashWithInfo(int, char const*, char const*, int)
3   0x471caa79e WebCore::JSLazyEventListener::checkValidityForEventTarget(WebCore::EventTarget&)
4   0x4722c0499 WebCore::EventTarget::addEventListener(WTF::AtomicString const&, WTF::Ref<WebCore::EventListener, WTF::DumbPtrTraits<WebCore::EventListener> >&&, WebCore::EventTarget::AddEventListenerOptions const&)
5   0x472ce04df WebCore::DOMWindow::addEventListener(WTF::AtomicString const&, WTF::Ref<WebCore::EventListener, WTF::DumbPtrTraits<WebCore::EventListener> >&&, WebCore::EventTarget::AddEventListenerOptions const&)
6   0x47233b0ad WebCore::tryAddEventListener(WebCore::Node*, WTF::AtomicString const&, WTF::Ref<WebCore::EventListener, WTF::DumbPtrTraits<WebCore::EventListener> >&&, WebCore::EventTarget::AddEventListenerOptions const&)
7   0x47233ae98 WebCore::Node::addEventListener(WTF::AtomicString const&, WTF::Ref<WebCore::EventListener, WTF::DumbPtrTraits<WebCore::EventListener> >&&, WebCore::EventTarget::AddEventListenerOptions const&)
8   0x4722c1137 WebCore::EventTarget::setAttributeEventListener(WTF::AtomicString const&, WTF::RefPtr<WebCore::EventListener, WTF::DumbPtrTraits<WebCore::EventListener> >&&, WebCore::DOMWrapperWorld&)
9   0x47226de46 WebCore::Element::setAttributeEventListener(WTF::AtomicString const&, WebCore::QualifiedName const&, WTF::AtomicString const&)
10  0x4725eb7f2 WebCore::HTMLElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomicString const&)
11  0x4722693dd WebCore::Element::attributeChanged(WebCore::QualifiedName const&, WTF::AtomicString const&, WTF::AtomicString const&, WebCore::Element::AttributeModificationReason)
12  0x4723e0eac WebCore::StyledElement::attributeChanged(WebCore::QualifiedName const&, WTF::AtomicString const&, WTF::AtomicString const&, WebCore::Element::AttributeModificationReason)
13  0x47226f812 WebCore::Element::didAddAttribute(WebCore::QualifiedName const&, WTF::AtomicString const&)
14  0x47226f753 WebCore::Element::addAttributeInternal(WebCore::QualifiedName const&, WTF::AtomicString const&, WebCore::Element::SynchronizationOfLazyAttribute)
15  0x472268b81 WebCore::Element::setAttributeInternal(unsigned int, WebCore::QualifiedName const&, WTF::AtomicString const&, WebCore::Element::SynchronizationOfLazyAttribute)
16  0x472268e3d WebCore::Element::setAttribute(WTF::AtomicString const&, WTF::AtomicString const&)
17  0x4708107ce WebCore::jsElementPrototypeFunctionSetAttributeBody(JSC::ExecState*, WebCore::JSElement*, JSC::ThrowScope&)
18  0x4707f79d0 long long WebCore::IDLOperation<WebCore::JSElement>::call<&(WebCore::jsElementPrototypeFunctionSetAttributeBody(JSC::ExecState*, WebCore::JSElement*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*)
19  0x4707f76bc WebCore::jsElementPrototypeFunctionSetAttribute(JSC::ExecState*)
20  0x424bcf00116b
21  0x424bcf02fe01
22  0x424bcf0300fc
23  0x424bcf03a921
24  0x424bcf039f65
25  0x46b785d94 llint_entry
26  0x46b7729e3 vmEntryToJavaScript
27  0x46c0fa0b7 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
28  0x46c0f5870 JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*)
29  0x46c0f404b JSC::eval(JSC::ExecState*)
30  0x46c19337d operationCallEval
31  0x424bcf028ac5
LEAK: 1 WebPageProxy


https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/r245052%20(3616)/results.html

[Radar WebKit Bug Importer]

<rdar://problem/50586956>

[Chris Dumez]

Oh great...

#if PLATFORM(IOS_FAMILY)
    if (targetNode == &targetNode->document() && eventType == eventNames().scrollEvent)
        targetNode->document().domWindow()->incrementScrollEventListenersCount();

    // FIXME: Would it be sufficient to special-case this code for <body> and <frameset>?
    //
    // This code was added to address <rdar://problem/5846492> Onorientationchange event not working for document.body.
    // Forward this call to addEventListener() to the window since these are window-only events.
    if (eventType == eventNames().orientationchangeEvent || eventType == eventNames().resizeEvent)
        targetNode->document().domWindow()->addEventListener(eventType, WTFMove(listener), options);

#if ENABLE(TOUCH_EVENTS)
    if (eventNames().isTouchRelatedEventType(targetNode->document(), eventType))
        targetNode->document().addTouchEventListener(*targetNode);
#endif
#endif // PLATFORM(IOS_FAMILY)

So we create a LazyEventListener for an Element (e.g. body) and then add it to the window. This explains the crashes we've seen on iOS with the reziseEvent lazy listeners.

[Chris Dumez]

Per spec, if you set the onresize or onorientationchange attribute on the body, it will also set the event listener on the window. This is implemented in HTMLBodyElement::parseAttribute().

The guilty code, tryAddEventListener() in Node.cpp was meant to make it so that setting an event listener via body.addEventListener('resize') or body.addEventListener('orientationchange') would also set the event listener on the Window. As far as I can tell, this is not standard behavior and is a weird WebKit quirk. The implementation is also wrong and is the source of crashes like <rdar://problem/24314027>. You cannot take a JSLazyEventListener from the body and then simply set it on the Window without consequences since the JSLazyEventListener has a raw pointer to its element.

Personally, I'd be tempted to drop this quirk altogether.

[Chris Dumez]

Blink does not have this non-standard quirk so I will simply drop it from WebKit.

[Chris Dumez]

Created attachment 369429 [details]
Patch

[Chris Dumez]

Created attachment 369448 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 369448 [details]
Patch

Clearing flags on attachment: 369448

Committed r245086: <https://trac.webkit.org/changeset/245086>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Created attachment 369993 [details]
Test that is a millions times better or more fun IMO

Here's a way more 🎉 test case that I wrote. In my opinion it is a million times better than the test that landed with this patch, but I could be biased haha 😀

[Shawn Roberts]

(In reply to Daniel Bates from comment #9)
> Created attachment 369993 [details]
> Test that is a millions times better or more fun IMO
> 
> Here's a way more 🎉 test case that I wrote. In my opinion it is a million
> times better than the test that landed with this patch, but I could be
> biased haha 😀

Agree, huge fan of blue.. a lots more better...