Bug 132148 - REGRESSION (r125251): wrapper lifetimes of SVGElementInstance are incorrect; assertion seen in svg/custom/use-instanceRoot-event-listeners.xhtml
Summary: REGRESSION (r125251): wrapper lifetimes of SVGElementInstance are incorrect; ...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
Keywords: InRadar
: 93812 (view as bug list)
Depends on:
Reported: 2014-04-24 15:16 PDT by Mark Lam
Modified: 2015-01-18 22:53 PST (History)
14 users (show)

See Also:

Patch (42.35 KB, patch)
2014-04-25 00:20 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (15.21 KB, patch)
2015-01-18 09:03 PST, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 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);
   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
Comment 1 Alexey Proskuryakov 2014-04-24 15:36:07 PDT
Seems closely related to bug 93812.
Comment 2 Radar WebKit Bug Importer 2014-04-24 16:29:13 PDT
Comment 3 Darin Adler 2014-04-25 00:20:53 PDT
Created attachment 230145 [details]
Comment 4 Andreas Kling 2014-04-25 00:46:26 PDT
Comment on attachment 230145 [details]

r=me. I'm so happy you did this!

You'll probably need to run "run-bindings-tests --reset-results" before landing this.
Comment 5 Darin Adler 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.
Comment 6 Darin Adler 2014-04-25 00:55:44 PDT
Committed r167794: <http://trac.webkit.org/changeset/167794>
Comment 7 Alexey Proskuryakov 2014-05-01 22:20:45 PDT
*** Bug 93812 has been marked as a duplicate of this bug. ***
Comment 8 Darin Adler 2015-01-17 08:02:43 PST
Mark says this bug still happens; apparently I was testing without JSC_slowPathAllocsBetweenGCs.
Comment 9 Darin Adler 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.
Comment 10 Darin Adler 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.
Comment 11 Darin Adler 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.
Comment 12 Darin Adler 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.
Comment 13 Darin Adler 2015-01-18 09:03:53 PST
Created attachment 244861 [details]
Comment 14 Darin Adler 2015-01-18 09:05:15 PST
*** Bug 93812 has been marked as a duplicate of this bug. ***
Comment 15 WebKit Commit Bot 2015-01-18 12:57:24 PST
Comment on attachment 244861 [details]

Clearing flags on attachment: 244861

Committed r178633: <http://trac.webkit.org/changeset/178633>
Comment 16 WebKit Commit Bot 2015-01-18 12:57:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Alexey Proskuryakov 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.