Bug 132148

Summary: REGRESSION (r125251): wrapper lifetimes of SVGElementInstance are incorrect; assertion seen in svg/custom/use-instanceRoot-event-listeners.xhtml
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: WebCore JavaScriptAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, commit-queue, darin, dino, eric.carlson, glenn, jer.noble, kondapallykalyan, ossy, philipj, roger_fong, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Mark Lam
Reported 2014-04-24 15:16:11 PDT
Build a debug build of ToT (any revision after r167772), and run the following: $ JSC_slowPathAllocsBetweenGCs=1 DumpRenderTree LayoutTests/svg/custom/use-instanceRoot-event-listeners.xhtml You'll see the following assertion failure: (lldb) r LayoutTests/svg/custom/use-instanceRoot-event-listeners.xhtml Process 65778 launched: '/Volumes/Data/ws4/OpenSource/WebKitBuild/Debug/DumpRenderTree' (x86_64) ASSERTION FAILED: !m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction /Volumes/Data/ws4/OpenSource/Source/WebCore/bindings/js/JSEventListener.h(96) : JSC::JSObject *WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext *) const 1 0x100a69fc0 WTFCrash 2 0x102ec8b60 WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) const 3 0x103187e1b WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) 4 0x10295982f WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow>&) 5 0x1029590fe WebCore::EventTarget::fireEventListeners(WebCore::Event*) 6 0x1037fcdec WebCore::Node::handleLocalEvents(WebCore::Event&) 7 0x1029234c1 WebCore::EventContext::handleLocalEvents(WebCore::Event&) const 8 0x1029237d7 WebCore::MouseOrFocusEventContext::handleLocalEvents(WebCore::Event&) const 9 0x102924ad4 WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&, WebCore::WindowEventContext&) 10 0x10292450f WebCore::EventDispatcher::dispatchEvent(WebCore::Node*, WTF::PassRefPtr<WebCore::Event>) 11 0x1037fce6d WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event>) 12 0x1028f9664 WebCore::Element::dispatchMouseEvent(WebCore::PlatformMouseEvent const&, WTF::AtomicString const&, int, WebCore::Element*) 13 0x102935685 WebCore::EventHandler::updateMouseEventTargetNode(WebCore::Node*, WebCore::PlatformMouseEvent const&, bool) 14 0x1029333fc WebCore::EventHandler::dispatchMouseEvent(WTF::AtomicString const&, WebCore::Node*, bool, int, WebCore::PlatformMouseEvent const&, bool) 15 0x102934816 WebCore::EventHandler::handleMouseMoveEvent(WebCore::PlatformMouseEvent const&, WebCore::HitTestResult*, bool) 16 0x102933ee6 WebCore::EventHandler::mouseMoved(WebCore::PlatformMouseEvent const&) 17 0x102946854 WebCore::EventHandler::mouseMoved(NSEvent*) ... (lldb) up frame #1: 0x0000000102ec8b60 WebCore`WebCore::JSEventListener::jsFunction(this=0x000000010f92ea80, scriptExecutionContext=0x000000010c02a0a0) const + 432 at JSEventListener.h:96 93 // Verify that we have a valid wrapper protecting our function from 94 // garbage collection. That is except for when we're not in the normal 95 // world and can have zombie m_jsFunctions. -> 96 ASSERT(!m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction); 97 98 // If m_wrapper is 0, then m_jsFunction is zombied, and should never be accessed. 99 if (!m_wrapper) (lldb) p m_isolatedWorld->isNormal() (bool) $3 = true (lldb) p !!m_wrapper (bool) $4 = false (lldb) p !m_jsFunction (bool) $6 = false (lldb) bt * thread #1: tid = 0xe9faec, 0x0000000100a69fca JavaScriptCore`WTFCrash + 42 at Assertions.cpp:333, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef) frame #0: 0x0000000100a69fca JavaScriptCore`WTFCrash + 42 at Assertions.cpp:333 * frame #1: 0x0000000102ec8b60 WebCore`WebCore::JSEventListener::jsFunction(this=0x000000010f92ea80, scriptExecutionContext=0x000000010c02a0a0) const + 432 at JSEventListener.h:96 frame #2: 0x0000000103187e1b WebCore`WebCore::JSEventListener::handleEvent(this=0x000000010f92ea80, scriptExecutionContext=0x000000010c02a0a0, event=0x000000010f927cc0) + 171 at JSEventListener.cpp:81 frame #3: 0x000000010295982f WebCore`WebCore::EventTarget::fireEventListeners(this=0x000000010b710280, event=0x000000010f927cc0, d=0x000000010b70fff0, entry=0x000000010f9283f0) + 1503 at EventTarget.cpp:246 frame #4: 0x00000001029590fe WebCore`WebCore::EventTarget::fireEventListeners(this=0x000000010b710280, event=0x000000010f927cc0) + 334 at EventTarget.cpp:197 frame #5: 0x00000001037fcdec WebCore`WebCore::Node::handleLocalEvents(this=0x000000010b710280, event=0x000000010f927cc0) + 156 at Node.cpp:2013 frame #6: 0x00000001029234c1 WebCore`WebCore::EventContext::handleLocalEvents(this=0x000000010f91e3b0, event=0x000000010f927cc0) const + 177 at EventContext.cpp:54 frame #7: 0x00000001029237d7 WebCore`WebCore::MouseOrFocusEventContext::handleLocalEvents(this=0x000000010f91e3b0, event=0x000000010f927cc0) const + 343 at EventContext.cpp:86 frame #8: 0x0000000102924ad4 WebCore`WebCore::dispatchEventInDOM(event=0x000000010f927cc0, path=0x00007fff5fbfaa08, windowEventContext=0x00007fff5fbfa9e8) + 356 at EventDispatcher.cpp:302 frame #9: 0x000000010292450f WebCore`WebCore::EventDispatcher::dispatchEvent(origin=0x000000010b710280, prpEvent=0x00007fff5fbfab60) + 815 at EventDispatcher.cpp:358 frame #10: 0x00000001037fce6d WebCore`WebCore::Node::dispatchEvent(this=0x000000010b710280, event=<unavailable>) + 45 at Node.cpp:2027 frame #11: 0x00000001028f9664 WebCore`WebCore::Element::dispatchMouseEvent(this=0x000000010b710280, platformEvent=0x00007fff5fbfb680, eventType=0x0000000114003bf0, detail=0, relatedTarget=0x000000010b70b120) + 484 at Element.cpp:232 frame #12: 0x0000000102935685 WebCore`WebCore::EventHandler::updateMouseEventTargetNode(this=0x000000010f8134d0, targetNode=0x000000010b70b120, mouseEvent=0x00007fff5fbfb680, fireMouseOverOut=true) + 3301 at EventHandler.cpp:2418 frame #13: 0x00000001029333fc WebCore`WebCore::EventHandler::dispatchMouseEvent(this=0x000000010f8134d0, eventType=0x0000000114003be8, targetNode=0x000000010b70b120, =false, clickCount=0, mouseEvent=0x00007fff5fbfb680, setUnder=true) + 124 at EventHandler.cpp:2446 frame #14: 0x0000000102934816 WebCore`WebCore::EventHandler::handleMouseMoveEvent(this=0x000000010f8134d0, mouseEvent=0x00007fff5fbfb680, hoveredNode=0x00007fff5fbfb590, onlyUpdateScrollbars=false) + 1910 at EventHandler.cpp:1859 frame #15: 0x0000000102933ee6 WebCore`WebCore::EventHandler::mouseMoved(this=0x000000010f8134d0, event=0x00007fff5fbfb680) + 134 at EventHandler.cpp:1725 frame #16: 0x0000000102946854 WebCore`WebCore::EventHandler::mouseMoved(this=0x000000010f8134d0, event=0x000000010b53dcb0) + 132 at EventHandlerMac.mm:603 frame #17: 0x0000000101c5a97c WebKit`-[WebHTMLView(self=0x000000010b703460, _cmd=0x00007fff88b8243e, event=0x000000010b53dcb0) _updateMouseoverWithEvent:] + 1356 at WebHTMLView.mm:1790 frame #18: 0x0000000101c67e84 WebKit`-[WebHTMLView mouseMovedNotification:](self=0x000000010b703460, _cmd=0x00007fff88b83293, notification=0x000000010b540b70) + 116 at WebHTMLView.mm:4105 ...
Attachments
Patch (42.35 KB, patch)
2014-04-25 00:20 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Patch (15.21 KB, patch)
2015-01-18 09:03 PST, Darin Adler 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2014-04-24 15:36:07 PDT
Seems closely related to bug 93812.
Radar WebKit Bug Importer
Comment 2 2014-04-24 16:29:13 PDT
<rdar://problem/16719461>
Darin Adler
Comment 3 2014-04-25 00:20:53 PDT
Created attachment 230145 [details] Patch
Andreas Kling
Comment 4 2014-04-25 00:46:26 PDT
Comment on attachment 230145 [details] Patch r=me. I'm so happy you did this! You'll probably need to run "run-bindings-tests --reset-results" before landing this.
Darin Adler
Comment 5 2014-04-25 00:54:21 PDT
(In reply to comment #4) > You'll probably need to run "run-bindings-tests --reset-results" before landing this. I did. Surprisingly, none of the bindings tests cover JSCustomMarkFunction.
Darin Adler
Comment 6 2014-04-25 00:55:44 PDT
Committed r167794: <http://trac.webkit.org/changeset/167794>
Alexey Proskuryakov
Comment 7 2014-05-01 22:20:45 PDT
*** Bug 93812 has been marked as a duplicate of this bug. ***
Darin Adler
Comment 8 2015-01-17 08:02:43 PST
Mark says this bug still happens; apparently I was testing without JSC_slowPathAllocsBetweenGCs.
Darin Adler
Comment 9 2015-01-17 08:59:29 PST
When I tried to do some logging by also setting JSC_logGC to 2, I started getting a different assertion, ASSERT(m_opaqueRoots.isEmpty()) failed in SlotVisitor::didStartMarking at the start of one collection.
Darin Adler
Comment 10 2015-01-17 19:55:14 PST
OK, I figured out what the second problem is. The bindings for addEventListener and removeEventListener assume that the EventTarget object itself is the object which has a wrapper that keeps alive the listener’s wrapper. But for an SVGElementInstance, the instance itself simply attaches the listener to the target, therefore the SVGElementInstance object doesn’t keep the listener’s wrapper alive, its target does. We have to fix the addEventListener binding.
Darin Adler
Comment 11 2015-01-17 19:59:31 PST
There’s event code in CodeGeneratorJS.pm that knows how to generate this code correctly. However, two years ago, <http://trac.webkit.org/changeset/125251> broke this by removing the code to generate the add/removeEventListener function.
Darin Adler
Comment 12 2015-01-17 20:02:14 PST
I can see two ways to fix this. The obvious way to do it would be to re-add the addEventListener/removeEventListener functions to SVGElementInstance somehow. But that would be dangerous since there is nothing that would prevent someone from calling the base EventTarget addEventListener/removeEventListener function and passing it an SVGElementInstance. I guess no fix like this can ever be entirely complete. I suppose the better way to fix this would be to change all addEventListener function calls to call a virtual function, which in the case of SVGElementInstance would return the correspondingElement.
Darin Adler
Comment 13 2015-01-18 09:03:53 PST
Created attachment 244861 [details] Patch
Darin Adler
Comment 14 2015-01-18 09:05:15 PST
*** Bug 93812 has been marked as a duplicate of this bug. ***
WebKit Commit Bot
Comment 15 2015-01-18 12:57:24 PST
Comment on attachment 244861 [details] Patch Clearing flags on attachment: 244861 Committed r178633: <http://trac.webkit.org/changeset/178633>
WebKit Commit Bot
Comment 16 2015-01-18 12:57:31 PST
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 17 2015-01-18 22:53:50 PST
This broke bindings generation tests, <https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/2072/steps/bindings-generation-tests/logs/stdio>. All the changes seems expected, so I'll re-generate results and land them.
Note You need to log in before you can comment on or make changes to this bug.