Bug 68679 - REGRESSION(87010): elements in ECMA-cloud neither filled nor blurred - crash on Chromium
Summary: REGRESSION(87010): elements in ECMA-cloud neither filled nor blurred - crash ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL: http://ejohn.org/files/ecma-cloud.svg
Keywords: InRadar
Depends on: 61183
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-22 23:29 PDT by Dirk Schulze
Modified: 2011-09-29 00:22 PDT (History)
4 users (show)

See Also:


Attachments
Reference by xlink:href fails if xlink ns was set to another name (383 bytes, image/svg+xml)
2011-09-23 09:51 PDT, Dirk Schulze
no flags Details
patch (48.49 KB, patch)
2011-09-28 17:22 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch + style fix (48.50 KB, patch)
2011-09-28 17:27 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2011-09-22 23:29:31 PDT
The ECMA-cloud is not filled with gradients, filters don't get applied. Have not tried to reduce the problem right now.
Comment 1 Dirk Schulze 2011-09-22 23:33:21 PDT
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.
Comment 2 Tim Horton 2011-09-23 09:03:31 PDT
A quick bisection says that this regression occurred in http://trac.webkit.org/changeset/87010/trunk
Comment 3 Tim Horton 2011-09-23 09:04:00 PDT
(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.
Comment 4 Dirk Schulze 2011-09-23 09:07:01 PDT
(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.
Comment 5 Tim Horton 2011-09-23 09:46:45 PDT
It seems that SVGURIReference::addSupportedAttributes (incorrectly) assumes that the prefix of "href" will always be "xlink".
Comment 6 Dirk Schulze 2011-09-23 09:51:03 PDT
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.
Comment 7 Tim Horton 2011-09-23 10:18:06 PDT
Also, your testcase still crashes Chrome.
Comment 8 Nikolas Zimmermann 2011-09-24 01:02:35 PDT
(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.
Comment 9 Tim Horton 2011-09-27 17:59:21 PDT
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.
Comment 10 Tim Horton 2011-09-27 18:06:13 PDT
I'd like to hear from an XML person on this actually, does anybody know who one might be?
Comment 11 Nikolas Zimmermann 2011-09-28 11:34:21 PDT
(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?
Comment 12 Tim Horton 2011-09-28 13:57:03 PDT
I've found (I think!) the requisite magic. I'll take this one.
Comment 13 Tim Horton 2011-09-28 14:28:35 PDT
(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!!
Comment 14 Radar WebKit Bug Importer 2011-09-28 17:04:45 PDT
<rdar://problem/10204649>
Comment 15 Tim Horton 2011-09-28 17:22:11 PDT
Created attachment 109101 [details]
patch
Comment 16 WebKit Review Bot 2011-09-28 17:25:14 PDT
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.
Comment 17 Tim Horton 2011-09-28 17:27:03 PDT
Created attachment 109102 [details]
patch + style fix
Comment 18 Nikolas Zimmermann 2011-09-28 23:13:57 PDT
(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!
Comment 19 Nikolas Zimmermann 2011-09-28 23:15:28 PDT
Comment on attachment 109102 [details]
patch + style fix

Superb, perfect catch! r=me.
Comment 20 WebKit Review Bot 2011-09-29 00:22:20 PDT
Comment on attachment 109102 [details]
patch + style fix

Clearing flags on attachment: 109102

Committed r96307: <http://trac.webkit.org/changeset/96307>
Comment 21 WebKit Review Bot 2011-09-29 00:22:26 PDT
All reviewed patches have been landed.  Closing bug.