Bug 69284 – Null dereference in SVGDocumentExtensions::removePendingResource when updating <use>'s href

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

[Tim Horton]

Created attachment 109513 [details]
reduced repro

<rdar://problem/10213035>

Most likely candidate for regression is http://trac.webkit.org/changeset/85413 but I'm not 100% on that.

Hits the assert in StringImpl::existingHash if in debug mode.

[Tim Horton]

Created attachment 109514 [details]
backtrace

Err, actually I'm not seeing that assert get hit. Attaching backtrace.

[Florin Malita]

Here's what appears to be going on:

* because the href target doesn't have a valid fragment (no '#'), SVGUseElement::buildPendingResource() doesn't set m_resourceId (nor does it mark the element as having pending resources)
* the filter attribute OTOH does have a valid (missing) target, and it marks the element as having a pending resources on attach
* then SVGUseElement::svgAttributeChanged() only checks whether there are pending resources (appears to assume that the only pending resource can be its href target), and proceeds to attempting to remove the uninitialized m_resourceId from the pending resources list

This simple guard takes care of the crash:

--- a/Source/WebCore/svg/SVGUseElement.cpp
+++ b/Source/WebCore/svg/SVGUseElement.cpp
@@ -198,7 +198,7 @@ void SVGUseElement::svgAttributeChanged(const QualifiedName& attrName)
         return;
 
     if (SVGURIReference::isKnownAttribute(attrName)) {
-        if (hasPendingResources()) {
+        if (hasPendingResources() && !m_resourceId.isEmpty()) {
             OwnPtr<SVGDocumentExtensions::SVGPendingElements> clients(document()->accessSVGExtensions()->re
             ASSERT(!clients->isEmpty());


But there seems to be a more fundamental problem: unless I'm missing something, SVGUseElement assumes that it has only one possible pending resource (xlink:href) - which is not the case. 

Can someone more familiar with the area weigh in? Also, do you think it's worth submitting the immediate fix (plus a todo note maybe) at this point?

[Florin Malita]

Created attachment 123373 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 123373 [details]
Patch

Hairy patch, but looks good. r=me!

[WebKit Review Bot]

Comment on attachment 123373 [details]
Patch

Clearing flags on attachment: 123373

Committed r105573: <http://trac.webkit.org/changeset/105573>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.