Summary: | [iOS Debug] ASSERTION FAILED: !m_originalNode in WebCore::JSLazyEventListener::checkValidityForEventTarget(WebCore::EventTarget &) | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||
Component: | New Bugs | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, kangil.han, rniwa, simon.fraser, sroberts, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=197617 | ||||||||||
Attachments: |
|
Description
Ryan Haddad
2019-05-08 10:36:29 PDT
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. 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. Blink does not have this non-standard quirk so I will simply drop it from WebKit. Created attachment 369429 [details]
Patch
Created attachment 369448 [details]
Patch
Comment on attachment 369448 [details] Patch Clearing flags on attachment: 369448 Committed r245086: <https://trac.webkit.org/changeset/245086> All reviewed patches have been landed. Closing bug. 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 😀
(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... |