Bug 78700 – SVG TRef/Use NULL ptr

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Berend-Jan Wever]

http://code.google.com/p/chromium/issues/detail?id=114358
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <g id="g">
    <animate id="animate">
    </animate>
    <tref xlink:href="#animate">
    </tref>
  </g>
  <use xlink:href="#g">
  </use>
</svg>

src\third_party\webkit\source\webcore\svg\svgtrefelement.cpp
void SVGTRefElement::buildPendingResource()
{
<<<snip>>>
    m_eventListener = SubtreeModificationEventListener::create(this, id);
    ASSERT(target->parentNode());
    target->parentNode()->addEventListener(eventNames().DOMSubtreeModifiedEvent, m_eventListener.get(), false);
}

src\third_party\webkit\source\webcore\dom\node.cpp
bool Node::addEventListener(const AtomicString& eventType, PassRefPtr<EventListener> listener, bool useCapture)
{
<<<snip>>>
    for (HashSet<SVGElementInstance*>::const_iterator it = instances.begin(); it != end; ++it) {
        ASSERT((*it)->shadowTreeElement());
        ASSERT((*it)->correspondingElement() == this);

        RefPtr<EventListener> listenerForCurrentShadowTreeElement = listenerForShadowTree;
        bool result = tryAddEventListener((*it)->shadowTreeElement(), eventType, listenerForCurrentShadowTreeElement.release(), useCapture);
<<<snip>>>

(*it) points to an SVGUseElement which doesn't have a shadowTreeElement, causing the NULL ptr.

[Nikolas Zimmermann]

(In reply to comment #0)
> <<<snip>>>
>     m_eventListener = SubtreeModificationEventListener::create(this, id);
>     ASSERT(target->parentNode());
>     target->parentNode()->addEventListener(eventNames().DOMSubtreeModifiedEvent, m_eventListener.get(), false);

> (*it) points to an SVGUseElement which doesn't have a shadowTreeElement, causing the NULL ptr.
Oh dear, apparently <tref> doesn't even check if the target is valid, just attaching its event listener. This is a bad idea. CC'ing Rob, who wrote the current <tref> implementation.

Reminds me of a similar <use> bug which is in the process of being fixed: white-list allowed targets, instead of black-listing disallowed ones.

[Florin Malita]

FWIW my patch for http://www.﷒0﷓ also fixes this particular crash (as the event listeners are moved onto the target instead of its parent). Sounds like we still need to add target validation though.

[Nikolas Zimmermann]

(In reply to comment #2)
> FWIW my patch for http://www.﷒0﷓ also fixes this particular crash (as the event listeners are moved onto the target instead of its parent). Sounds like we still need to add target validation though.

Just looked at this again, Rob did it just correct according to SVG 1.1 2nd edition: "xlink:href of <tref>: An IRI reference to an element whose character data content shall be used as character data for this ‘tref’ element." There's no white or black-listing desired, any element should work.

So probably this is just a dup of your bug.

[Nikolas Zimmermann]

Created attachment 129693 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Review Bot]

Attachment 129693 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h"
Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 176 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

(In reply to comment #6)
> If any of these errors are false positives, please file a bug against check-webkit-style.
Not mine. Bug was already filed.

[Hajime Morrita]

Comment on attachment 129693 [details]
Patch

Adding test doesn't hurt. Could you update the ChangeLog to put diff to the head?

[Nikolas Zimmermann]

Committed r117229: <http://trac.webkit.org/changeset/117229>