Bug 78700 - SVG TRef/Use NULL ptr
Summary: SVG TRef/Use NULL ptr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P1 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-15 05:23 PST by Berend-Jan Wever
Modified: 2012-05-16 00:43 PDT (History)
17 users (show)

See Also:


Attachments
Patch (1.73 KB, patch)
2012-03-01 06:11 PST, Nikolas Zimmermann
morrita: review+
morrita: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 2012-02-15 05:23:42 PST
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.
Comment 1 Nikolas Zimmermann 2012-02-15 05:31:36 PST
(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.
Comment 2 Florin Malita 2012-02-15 13:33:53 PST
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.
Comment 3 Nikolas Zimmermann 2012-02-15 15:22:16 PST
(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.
Comment 4 Nikolas Zimmermann 2012-03-01 06:11:30 PST
Created attachment 129693 [details]
Patch
Comment 5 WebKit Review Bot 2012-03-01 06:14:14 PST
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
Comment 6 WebKit Review Bot 2012-03-01 06:15:27 PST
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.
Comment 7 Nikolas Zimmermann 2012-03-01 06:26:27 PST
(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.
Comment 8 Hajime Morrita 2012-03-15 01:26:36 PDT
Comment on attachment 129693 [details]
Patch

Adding test doesn't hurt. Could you update the ChangeLog to put diff to the head?
Comment 9 Nikolas Zimmermann 2012-05-16 00:43:35 PDT
Committed r117229: <http://trac.webkit.org/changeset/117229>