Bug 132148 – REGRESSION (r125251): wrapper lifetimes of SVGElementInstance are incorrect; assertion seen in svg/custom/use-instanceRoot-event-listeners.xhtml

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Mark Lam]

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
...

[Alexey Proskuryakov]

Seems closely related to bug 93812.

[Radar WebKit Bug Importer]

<rdar://problem/16719461>

[Darin Adler]

Created attachment 230145 [details]
Patch

[Andreas Kling]

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]

(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]

Committed r167794: <http://trac.webkit.org/changeset/167794>

[Alexey Proskuryakov]

*** Bug 93812 has been marked as a duplicate of this bug. ***

[Darin Adler]

Mark says this bug still happens; apparently I was testing without JSC_slowPathAllocsBetweenGCs.

[Darin Adler]

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]

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]

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]

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]

Created attachment 244861 [details]
Patch

[Darin Adler]

*** Bug 93812 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 244861 [details]
Patch

Clearing flags on attachment: 244861

Committed r178633: <http://trac.webkit.org/changeset/178633>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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.