Bug 74858

Summary: chrome.dll!WebCore::SVGTRefElement::updateReferencedText ReadAV@NULL (e85cb8e140071fa7790cad215b0109dc)
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, fmalita, rwlbuis, webkit.review.bot, zimmermann
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
Attachments:
Description Flags
Patch
none
Reduced test.
none
Patch none

Description Berend-Jan Wever 2011-12-19 05:48:46 PST
Chromium: http://code.google.com/p/chromium/issues/detail?id=108057

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <text id="text"></text>
  <tref xlink:href="#text"/>
  <script>
    document.documentElement.replaceChild(
        document.createElement('anything'), // inserted element
        document.getElementById('text'));   // removed element
  </script>
</svg>

stack:          chrome.dll!WebCore::SVGTRefElement::updateReferencedText
                chrome.dll!WebCore::SubtreeModificationEventListener::handleEvent
                chrome.dll!WebCore::EventTarget::fireEventListeners
                chrome.dll!WebCore::EventTarget::fireEventListeners
                chrome.dll!WebCore::Node::handleLocalEvents
                chrome.dll!WebCore::EventDispatcher::dispatchEvent
                chrome.dll!WebCore::EventDispatchMediator::dispatchEvent
                chrome.dll!WebCore::EventDispatcher::dispatchEvent
                chrome.dll!WebCore::ScopedEventQueue::dispatchEvent
                chrome.dll!WebCore::ScopedEventQueue::enqueueEventDispatchMediator
                chrome.dll!WebCore::EventDispatcher::dispatchScopedEvent
                chrome.dll!WebCore::Node::dispatchScopedEvent
                chrome.dll!WebCore::Node::dispatchSubtreeModifiedEvent
                chrome.dll!WebCore::ContainerNode::replaceChild
                chrome.dll!WebCore::Node::replaceChild
                chrome.dll!WebCore::V8Node::replaceChildCallback
                chrome.dll!v8::internal::HandleApiCallHelper<...>
                chrome.dll!v8::internal::Builtin_HandleApiCall
                chrome.dll!v8::internal::Invoke
                chrome.dll!v8::internal::Execution::Call
                ...
Comment 1 Florin Malita 2012-01-09 14:59:08 PST
The DOMSubtreeModifiedEvent listener is still active on the target's parent after the target is removed.

We need to catch the target removal event and deactivate any associated listeners. I have a first-pass at this following up.

Additionally:

* on target removal, mark the tref as resource-pending again, to reattach if the same id is added at a later time (I believe this is desirable)
* move the DOMSubtreeModifiedEvent to the target element itself to keep things simple (I think we're only interested in catching updates to the target element, and the event appears to be dispatched to it too - hope I'm not missing something here)
Comment 2 Florin Malita 2012-01-09 15:02:43 PST
Created attachment 121727 [details]
Patch
Comment 3 Florin Malita 2012-01-11 07:46:29 PST
Created attachment 122019 [details]
Reduced test.
Comment 4 Florin Malita 2012-02-15 13:36:43 PST
Created attachment 127225 [details]
Patch

Updated after the recent shadow tree changes.
Comment 5 Nikolas Zimmermann 2012-02-15 15:19:08 PST
Comment on attachment 127225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127225&action=review

Looks just great! r=me.

> Source/WebCore/svg/SVGTRefElement.cpp:185
> +    Node* container = shadowRootList()->oldestShadowRoot()->firstChild();

oldestShadowRoot, and shadowRootList are guaranteed non-null?
If yes, there's nothing hold back a cq+.
Comment 6 Florin Malita 2012-02-16 07:30:11 PST
(In reply to comment #5)
> > Source/WebCore/svg/SVGTRefElement.cpp:185
> > +    Node* container = shadowRootList()->oldestShadowRoot()->firstChild();
> 
> oldestShadowRoot, and shadowRootList are guaranteed non-null?

Yes, SVGTRefElement::create() calls createShadowSubtree() which initializes the shadow root plumbing.


> If yes, there's nothing hold back a cq+.

Thanks!
Comment 7 Nikolas Zimmermann 2012-02-17 07:36:26 PST
Comment on attachment 127225 [details]
Patch

r=me.
Comment 8 WebKit Review Bot 2012-02-17 08:50:30 PST
Comment on attachment 127225 [details]
Patch

Clearing flags on attachment: 127225

Committed r108082: <http://trac.webkit.org/changeset/108082>
Comment 9 WebKit Review Bot 2012-02-17 08:50:36 PST
All reviewed patches have been landed.  Closing bug.