Bug 37798 - svg/custom/use-instanceRoot-as-event-target.xhtml crashes randomly
Summary: svg/custom/use-instanceRoot-as-event-target.xhtml crashes randomly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL: http://build.webkit.org/results/Windo...
Keywords: LayoutTestFailure
: 36645 (view as bug list)
Depends on:
Blocks: 33305
  Show dependency treegraph
 
Reported: 2010-04-19 07:26 PDT by Adam Roben (:aroben)
Modified: 2010-05-07 11:25 PDT (History)
5 users (show)

See Also:


Attachments
Patch (15.64 KB, patch)
2010-05-07 05:31 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (15.98 KB, patch)
2010-05-07 11:06 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2010-04-19 07:26:27 PDT
svg/custom/use-instanceRoot-as-event-target.xhtml crashed once on the Windows Debug bot.
Comment 1 Nikolas Zimmermann 2010-05-04 07:54:39 PDT
Reproducable on Mac, using guardmalloc:

(gdb) set env DYLD_INSERT_LIBRARIES /usr/lib/libgmalloc.dylib
(gdb) run
Starting program: /Users/nikolaszimmermann/Coding/WebKit/WebKitBuild/Debug/DumpRenderTree LayoutTests/svg/custom/use-instanceRoot-as-event-target.xhtml
GuardMalloc: Allocations will be placed on 16 byte boundaries.
GuardMalloc:  - Some buffer overruns may not be noticed.
GuardMalloc:  - Applications using vector instructions (e.g., SSE or Altivec) should work.
GuardMalloc: GuardMalloc version 18
GuardMalloc: Allocations will be placed on 16 byte boundaries.
GuardMalloc:  - Some buffer overruns may not be noticed.
GuardMalloc:  - Applications using vector instructions (e.g., SSE or Altivec) should work.
GuardMalloc: GuardMalloc version 18
Reading symbols for shared libraries . done
Reading symbols for shared libraries . done
GuardMalloc: Allocations will be placed on 16 byte boundaries.
GuardMalloc:  - Some buffer overruns may not be noticed.
GuardMalloc:  - Applications using vector instructions (e.g., SSE or Altivec) should work.
GuardMalloc: GuardMalloc version 18
Reading symbols for shared libraries ++ done
Reading symbols for shared libraries ..... done
Reading symbols for shared libraries + done
ASSERTION FAILED: m_wrapper || !m_jsFunction
(/Users/nikolaszimmermann/Coding/WebKit/WebCore/bindings/js/JSEventListener.h:83 JSC::JSObject* WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) const)

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x00000000 in ?? ()
(gdb) bt
#0  0x00000000 in ?? ()
#1  0x0452f6f7 in WebCore::JSEventListener::jsFunction (this=0xdea91fe0, scriptExecutionContext=0xd995f144) at JSEventListener.h:83
#2  0x045d0afc in WebCore::JSEventListener::handleEvent (this=0xdea91fe0, scriptExecutionContext=0xd995f144, event=0xe2ddaf80) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/bindings/js/JSEventListener.cpp:68
#3  0x0434f842 in WebCore::EventTarget::fireEventListeners (this=0xdd62af00, event=0xe2ddaf80, d=0xdd664fa0, entry=@0xdea97fe0) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/EventTarget.cpp:315
#4  0x0434ff16 in WebCore::EventTarget::fireEventListeners (this=0xdd62af00, event=0xe2ddaf80) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/EventTarget.cpp:276
#5  0x048359d1 in WebCore::Node::handleLocalEvents (this=0xdd62af00, event=0xe2ddaf80) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/Node.cpp:2588
#6  0x0483611d in WebCore::Node::dispatchGenericEvent (this=0xdd62af00, prpEvent=@0xbfffd2ac) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/Node.cpp:2731
#7  0x0483661f in WebCore::Node::dispatchEvent (this=0xdd62af00, prpEvent=@0xbfffd38c) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/Node.cpp:2651
#8  0x048347f5 in WebCore::Node::dispatchMouseEvent (this=0xdd62af00, eventType=@0xd400cdcc, button=-1, detail=0, pageX=50, pageY=50, screenX=-9950, screenY=-9950, ctrlKey=false, altKey=false, shiftKey=false, metaKey=false, isSimulated=false, relatedTargetArg=0xddd13fc0, underlyingEvent=@0xbfffd454) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/Node.cpp:2943
#9  0x04834d01 in WebCore::Node::dispatchMouseEvent (this=0xdd62af00, event=@0xbfffd754, eventType=@0xd400cdcc, detail=0, relatedTarget=0xddd13fc0) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/Node.cpp:2852
#10 0x0433e7be in WebCore::EventHandler::updateMouseEventTargetNode (this=0xd2f65e70, targetNode=0xdd62af00, mouseEvent=@0xbfffd754, fireMouseOverOut=true) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/page/EventHandler.cpp:1792
#11 0x0433e868 in WebCore::EventHandler::dispatchMouseEvent (this=0xd2f65e70, eventType=@0xd400cdbc, targetNode=0xdd62af00, clickCount=0, mouseEvent=@0xbfffd754, setUnder=true) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/page/EventHandler.cpp:1806
#12 0x04342fc3 in WebCore::EventHandler::handleMouseMoveEvent (this=0xd2f65e70, mouseEvent=@0xbfffd754, hoveredNode=0xbfffd6dc) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/page/EventHandler.cpp:1480
#13 0x04343061 in WebCore::EventHandler::mouseMoved (this=0xd2f65e70, event=@0xbfffd754) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/page/EventHandler.cpp:1367
#14 0x043491e5 in WebCore::EventHandler::mouseMoved (this=0xd2f65e70, event=0xe2dbefc0) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/page/mac/EventHandlerMac.mm:610
#15 0x00c1d822 in -[WebHTMLView(WebPrivate) _updateMouseoverWithEvent:] (self=0xd3b1dfa0, _cmd=0xd029b2, event=0xe2dbefc0) at /Users/nikolaszimmermann/Coding/WebKit/WebKit/mac/WebView/WebHTMLView.mm:1617

Will investigate soon...
Comment 3 Nikolas Zimmermann 2010-05-06 01:30:55 PDT
*** Bug 36645 has been marked as a duplicate of this bug. ***
Comment 4 Nikolas Zimmermann 2010-05-06 01:51:29 PDT
Oh my, I know what's going on, a pretty evil race. Here are some notes from my last gdb session with breakpoints on SVGElementInstance & JSSVGElementInstance constructor/destructors.

#1) Initial creation of SVGElementInstance root object

Breakpoint 7, WebCore::SVGElementInstance::SVGElementInstance (this=0x1bd08a50, useElement=0x1bd5ee40, originalElement=@0xbfffdfcc) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGElementInstance.cpp:49

#2) Initial association between the shadowTreeElement & the SVGElementInstance

Breakpoint 5, WebCore::SVGElementInstance::setShadowTreeElement (this=0x1bd08a50, element=0x1bd5f030) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGElementInstance.cpp:81

#3) Initial creation of the JS wrapper used from the test

Breakpoint 12, WebCore::JSSVGElementInstance::JSSVGElementInstance (this=0xf50380, structure=@0xbfffda84, globalObject=0xf4a940, impl=@0xbfffda88) at /Users/nikolaszimmermann/Coding/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/JSSVGElementInstance.cpp:214

....

At some point, removeAttribute() is called on the correspondingElement of the SVGElementInstance (aka. the original element that the <use> element is referencing), which causes a reclone of the <use shadow tree (lazily! as soon as updateFromElement is called on RenderSVGShadowTreeRootContainer). This is expected, as we're not trying to synchronize the trees, as it's too hard to get right (would require lots of changes in core methods like addAttribute/setAttribute/appendChild/etc..)

#4) Recreation of a SVGElementInstance (note that the OLD SVGElementInstance has NOT been destructed yet, as the JSSVGElementInstance is still holding a RefPtr on it)

Breakpoint 7, WebCore::SVGElementInstance::SVGElementInstance (this=0x1bd07f90, useElement=0x1bd5ee40, originalElement=@0xbfffb0bc) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGElementInstance.cpp:49

#5) Reassociation of the SVGElementInstance with its shadowTreeElement

Breakpoint 5, WebCore::SVGElementInstance::setShadowTreeElement (this=0x1bd07f90, element=0x1bd85370) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/svg/SVGElementInstance.cpp:81

#6) Destruction of the OLD JSSVGElementInstance object (with injected hacks to force GC at exactly this point)

Breakpoint 14, WebCore::JSSVGElementInstance::~JSSVGElementInstance (this=0xf50380) at /Users/nikolaszimmermann/Coding/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/JSSVGElementInstance.cpp:218

#7) Assertion fires...

ASSERTION FAILED: m_wrapper || !m_jsFunction
(/Users/nikolaszimmermann/Coding/WebKit/WebCore/bindings/js/JSEventListener.h:83 JSC::JSObject* WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) const)

(gdb) p m_wrapper
$4 = {
  <WTFNoncopyable::Noncopyable> = {
    <WTF::FastAllocBase> = {<No data fields>}, <No data fields>}, 
  members of JSC::WeakGCPtr<JSC::JSObject>: 
  m_ptr = 0xf50380

(gdb) p wrapper()
$3 = (class JSC::JSObject *) 0x0

Backtrace is equal to the one I've posted before. Here is the analysis:
Stepping up to Node::handleLocalEvents, reveals:

#4  0x047213bd in WebCore::Node::handleLocalEvents (this=0x1bd85370, event=0x1bd79f30) at /Users/nikolaszimmermann/Coding/WebKit/WebCore/dom/Node.cpp:2588

handleLocalEvents has been called on the _NEW_ shadowTreeElement (lookup the this pointer above).
The JSEventListener is associated with the JSSVGEementInstance object that has just been destructed.

A side-note before: SVGElementInstance just pretends to be an EventTarget, but in fact it shares an event listener list with the original correspondingElement (as per SVG spec) -> addEventListener is in fact forwarded to the correspondingElement. But the JSEventListeners (created through JSSVGElementInstance):
    imp->addEventListener(ustringToAtomicString(args.at(0).toString(exec)), JSEventListener::create(asObject(listener), castedThis, false, currentWorld(exec)), args.at(2).toBoolean(exec));
are created using the _OLD_ JSSVGElementInstance object (which has just been destructed by GC above).

The offending code is:

useElement.instanceRoot.addEventListener(eventHandler, ...);
rectElement.removeAttribute("foo") <-- causes reclone, new SVGElementInstances are born
hackToForceGC(); <-- will destruct JSSVGElementInstance
moveMouseOverRectInstance(); <-- should fire eventHandler, but hits assertion, because there is no JSSVGElementInstance object anymore

So the problem basically exists because we have JSEventListeners floating around which have associated wrappers that are destructed. Glad the assertion actually exists, otherwhise I would have never found it...
 
Not yet sure how to fix this, but I wanted to share this analysis, so it's clear what happens.
Comment 5 Alexey Proskuryakov 2010-05-06 08:31:54 PDT
Removing PlatformOnly keyword - this happens on all platforms.
Comment 6 Nikolas Zimmermann 2010-05-07 05:31:50 PDT
Created attachment 55368 [details]
Patch
Comment 7 Eric Seidel (no email) 2010-05-07 08:49:52 PDT
Comment on attachment 55368 [details]
Patch

WebCore/ChangeLog:14
 +          it, but residing in the correspondingElement() event listener list (and thus existant!).
The last sentence isn't entirely english.  registrated and existant aren't words.

WebCore/svg/SVGElementInstance.cpp:92
 +      if (!element || !element->document())
I can never remember if this is the right ->document() pointer or ownerDocument().  Should this be element->inDocument()?

WebCore/svg/SVGElementInstance.cpp:150
 +      ASSERT_NOT_REACHED();
Seems we should add comments in teh code to explain *why* these are ASSERT_NOT_REACHED()

LayoutTests/ChangeLog:8
 +          Make use-instanceRoot-as-event-target.xhtml behave more correctly. Due a copy&paste problem test #7 was not executed. Fixed.
copy & paste

Seems we need TestObject.idl changes to correspond with your codegenerator changes.

Otherwise this looks fantastic!
Comment 8 Nikolas Zimmermann 2010-05-07 10:52:39 PDT
(In reply to comment #7)
> (From update of attachment 55368 [details])
> WebCore/ChangeLog:14
>  +          it, but residing in the correspondingElement() event listener list
> (and thus existant!).
> The last sentence isn't entirely english.  registrated and existant aren't
> words.
Fixed.
 
> WebCore/svg/SVGElementInstance.cpp:92
>  +      if (!element || !element->document())
> I can never remember if this is the right ->document() pointer or
> ownerDocument().  Should this be element->inDocument()?
I used || !element->inDocument(). Didn't know about this shortcut even.

> 
> WebCore/svg/SVGElementInstance.cpp:150
>  +      ASSERT_NOT_REACHED();
> Seems we should add comments in teh code to explain *why* these are
> ASSERT_NOT_REACHED()
Done.

> 
> LayoutTests/ChangeLog:8
>  +          Make use-instanceRoot-as-event-target.xhtml behave more correctly.
> Due a copy&paste problem test #7 was not executed. Fixed.
> copy & paste
Done.

> 
> Seems we need TestObject.idl changes to correspond with your codegenerator
> changes.
No this only affects SVGElementInstance.idl generation, nothing else.

Going to upload a new patch...
Comment 9 Nikolas Zimmermann 2010-05-07 11:06:21 PDT
Created attachment 55397 [details]
Patch
Comment 10 Dirk Schulze 2010-05-07 11:18:45 PDT
Comment on attachment 55397 [details]
Patch

Great work and realy tricky bug. r=me.

Reviewed this patch to get bots green again.
Comment 11 Nikolas Zimmermann 2010-05-07 11:25:30 PDT
Committed r58960: <http://trac.webkit.org/changeset/58960>