REGRESSION(87010): elements in ECMA-cloud neither filled nor blurred - crash on Chromium

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

The ECMA-cloud is not filled with gradients, filters don't get applied. Have not tried to reduce the problem right now.

[Dirk Schulze]

Still does not work after disabling filters (removed from document). Still see a crash on Chromium but not at webkit nightly (even no assertions). All shapes remain unfilled.

[Tim Horton]

A quick bisection says that this regression occurred in http://trac.webkit.org/changeset/87010/trunk

[Tim Horton]

(In reply to comment #2)
> A quick bisection says that this regression occurred in http://trac.webkit.org/changeset/87010/trunk

The it-doesn't-look-right one, not the crash, since that doesn't happen in Safari.

[Dirk Schulze]

(In reply to comment #3)
> (In reply to comment #2)
> > A quick bisection says that this regression occurred in http://trac.webkit.org/changeset/87010/trunk
> 
> The it-doesn't-look-right one, not the crash, since that doesn't happen in Safari.

I assume that you checked it with nighties? Because right now I don't see a reason why this could be caused by this patch. There are no dynamic updates that could call svgAttrChanged IIRC.

[Tim Horton]

It seems that SVGURIReference::addSupportedAttributes (incorrectly) assumes that the prefix of "href" will always be "xlink".

[Dirk Schulze]

Created attachment 108485 [details]
Reference by xlink:href fails if xlink ns was set to another name

If another name than 'xlink' was chosen for shorthand of Xlink namespace (e.g. xmlns:xl="http://www.w3.org/1999/xlink"  ) the attribute gets not assigned to the xlink namespace. This is the case for ECMA-cloud. This is a simple test case.

[Tim Horton]

Also, your testcase still crashes Chrome.

[Nikolas Zimmermann]

(In reply to comment #5)
> It seems that SVGURIReference::addSupportedAttributes (incorrectly) assumes that the prefix of "href" will always be "xlink".

Ouch, yes my patch assumed that! Sorry for not thinking about that further :(
I have no time this weekend and next week to look into it though.

[Tim Horton]

Is there any reason we actually care about the prefix? By the time it gets to isSupportedAttribute, it's a QualifiedName, which is a triple{prefix, name, namespace}. It seems to me that we only care about the name and the namespace; something earlier than us has already taken care of mapping the prefix to the namespace.

As a test, I did two things:

Change SVGURIReference::addSupportedAttributes to just:

>    void SVGURIReference::addSupportedAttributes(HashSet<QualifiedName>& supportedAttributes)
>    {
>      supportedAttributes.add(XLinkNames::hrefAttr);
>    }

In SVGGradientElement::isSupportedAttribute (because the element with the xlink on it in our example here is a gradient):

>    QualifiedName attrNameWithoutPrefix(attrName);
>    attrNameWithoutPrefix.setPrefix(nullAtom);

And then supportedAttributes.contains(attrNameWithoutPrefix).

Effectively, only having the namespaced (non-prefixed) version of the attribute name in supportedAttributes, and dropping the prefix from the attribute before checking it. This fixes our bug, and I double checked to make sure that it is still matching on the namespaces (it is).

So that might be a valid fix.

Is there an earlier place where we could strip the prefix instead of touching every single isSupportedAttribute? I haven't decided yet.

I don't see why we should care about the prefix at all. Just the namespace.

[Tim Horton]

I'd like to hear from an XML person on this actually, does anybody know who one might be?

[Nikolas Zimmermann]

(In reply to comment #9)
> Is there any reason we actually care about the prefix? By the time it gets to isSupportedAttribute, it's a QualifiedName, which is a triple{prefix, name, namespace}. It seems to me that we only care about the name and the namespace; something earlier than us has already taken care of mapping the prefix to the namespace.
No, we don't care for the prefix - it's just buggy. The NS matters, the prefix is user defined and not relevant for us.

> Change SVGURIReference::addSupportedAttributes to just:
> 
> >    void SVGURIReference::addSupportedAttributes(HashSet<QualifiedName>& supportedAttributes)
> >    {
> >      supportedAttributes.add(XLinkNames::hrefAttr);
> >    }

That just removes the xlink prefix, that I manually added before there, right? If so, this is the right change.

> In SVGGradientElement::isSupportedAttribute (because the element with the xlink on it in our example here is a gradient):
> 
> >    QualifiedName attrNameWithoutPrefix(attrName);
> >    attrNameWithoutPrefix.setPrefix(nullAtom);
> 
> And then supportedAttributes.contains(attrNameWithoutPrefix).
> 
> Effectively, only having the namespaced (non-prefixed) version of the attribute name in supportedAttributes, and dropping the prefix from the attribute before checking it. This fixes our bug, and I double checked to make sure that it is still matching on the namespaces (it is).
> 
> So that might be a valid fix.

I remember I had it designed this way at some point, but something wasn't working. Maybe we can override contains() in a way that it automatically matches without comparing the prefix.

For that we'd need to add some magic to supply a custom HashSet comparision function - there are existing examples how to do that, that compares QNames without checking for prefix equality.

    bool matches(const QualifiedName& other) const { return m_impl == other.m_impl || (localName() == other.localName() && namespaceURI() == other.namespaceURI()); }

The QName::matches function is specifically designed for this.
To summarize:
* Your SVGURIReference::isSupportedAttr change is correct and should be done
* We need a customizable contains() implementation, that compares QNames by calling matches().

> Is there an earlier place where we could strip the prefix instead of touching every single isSupportedAttribute? I haven't decided yet.
> 
> I don't see why we should care about the prefix at all. Just the namespace.
Right.

Hope that works?

[Tim Horton]

I've found (I think!) the requisite magic. I'll take this one.

[Tim Horton]

(In reply to comment #12)
> I've found (I think!) the requisite magic. I'll take this one.

Actually, your suggestion isn't quite sufficient. We're going to *have* to strip the prefixes out somewhere because the hash function takes them into account, so if we don't strip them out, we never even *get* to the point of comparing them (be it with equals or with matches, we don't even get that far) because the hashes don't match.

I'll have a simple patch to do this making use of HashTranslators in a little bit (I'm stripping prefix just before we hash it, and *only* for the hashed copy, and using matches() to match).

Niko, if you can remember/look up what:

> I remember I had it designed this way at some point, but something wasn't working.

means, I'd love to know!!

[Radar WebKit Bug Importer]

<rdar://problem/10204649>

[Tim Horton]

Created attachment 109101 [details]
patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1

Source/WebCore/svg/SVGElement.h:134:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 60 files


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

[Tim Horton]

Created attachment 109102 [details]
patch + style fix

[Nikolas Zimmermann]

(In reply to comment #13)
> (In reply to comment #12)
> > I've found (I think!) the requisite magic. I'll take this one.
> 
> Actually, your suggestion isn't quite sufficient. We're going to *have* to strip the prefixes out somewhere because the hash function takes them into account, so if we don't strip them out, we never even *get* to the point of comparing them (be it with equals or with matches, we don't even get that far) because the hashes don't match.
Oops, I think I was a bit unspecific in my last mail:
"* We need a customizable contains() implementation, that compares QNames by calling matches()."

contains() isn't customizable, I meant the hashing needs to be customized.
But anyhow, you've got what I meant :-)
 
> I'll have a simple patch to do this making use of HashTranslators in a little bit (I'm stripping prefix just before we hash it, and *only* for the hashed copy, and using matches() to match).
Excellent, will review now.

> 
> Niko, if you can remember/look up what:
> 
> > I remember I had it designed this way at some point, but something wasn't working.
> 
> means, I'd love to know!!
Well, if your patch passes everything there's no remaining problem :-)
It's too long ago, I can't remember exactly which testcase was broken, but it was definately in svg/custom, so if your pixel tests all pass, you're done!

[Nikolas Zimmermann]

Comment on attachment 109102 [details]
patch + style fix

Superb, perfect catch! r=me.

[WebKit Review Bot]

Comment on attachment 109102 [details]
patch + style fix

Clearing flags on attachment: 109102

Committed r96307: <http://trac.webkit.org/changeset/96307>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.