Created attachment 98392[details]
Testcase
BUILD: WebKit nightly
STEPS TO REPRODUCE: Load attached XHTML file
EXPECTED RESULTS: Black square
ACTUAL RESULTS: Red square
ADDITIONAL DETAILS: It looks like the handling of URL references in SVG is just completely and utterly broken: it ignores everything before the '#' and treats any URL as a same-document reference to the element whose ID is in the fragment identifier.

Created attachment 100244[details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Is comparing to the document->baseURI() the right thing? It doesn't seem to match other implementations. In particular, this sort of thing:
<base href="some-other-document">
....
<rect style="fill(#something)">
has the rect fill with some-other-document#something in both Gecko and Presto.

(In reply to comment #4)
> Is comparing to the document->baseURI() the right thing? It doesn't seem to match other implementations. In particular, this sort of thing:
>
> <base href="some-other-document">
> ....
> <rect style="fill(#something)">
>
> has the rect fill with some-other-document#something in both Gecko and Presto.
I don't think we support external references like that at all, that is why we still have the external <use> bug. My patch does not try to address that however :) Unfortunately this could mean this patch only makes a small step in the right direction, not a complete fix. On the other hand I do not know SVGs that use external references for fill? Do you know any or have testcases?
Cheers,
Rob.

(In reply to comment #5)
> It also doesn't match WebKit behavior for <a href>.
Can you explain some more? Do you mean that external references are allowed for <a> (obviously) but not for fill property? Note that <a href> handling uses baseURI internally as well.
Cheers,
Rob.

Comment on attachment 100243[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=100243&action=review
In general this looks fine, some comments though:
> Source/WebCore/svg/SVGURIReference.cpp:47
> -String SVGURIReference::getTarget(const String& url)
> +String SVGURIReference::getTarget(const String& funcUri, Document* document, bool allowExternal)
How about naming it, extractTargetURI, if you're at it?
A boolean parameter like "allowExternal" should be avoided. Use an enum.
enum ExtractTargetMode {
AllowExternalReferences,
ForbidExternalReferences
};
...
> Source/WebCore/svg/SVGURIReference.cpp:49
> + if (funcUri.find('#') == notFound)
This code path is _very_ hot. If you're at it, I'd propose to make it more efficient (like your old work years ago where you made the path parsing more efficient, by directly operating on the const UChar* characters buffer).
> Source/WebCore/svg/SVGURIReference.cpp:50
> + return String();
s/String()/emptyString()/ - use the shared empty string for more efficiency.
> Source/WebCore/svg/SVGURIReference.cpp:53
> if (url.startsWith("url(")) { // URI References, ie. fill:url(#target)
The initial decision here should be:
first character is 'u' or '#'.
Remember to always optimize for the common path, so checking wheter funcUri.find('#') returns notFound as first thing is inefficient, as you're doing avoidable extra-work for valid references.

Comment on attachment 100243[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=100243&action=review
In general this looks fine, some comments though:
> Source/WebCore/svg/SVGURIReference.cpp:47
> -String SVGURIReference::getTarget(const String& url)
> +String SVGURIReference::getTarget(const String& funcUri, Document* document, bool allowExternal)
How about naming it, extractTargetURI, if you're at it?
A boolean parameter like "allowExternal" should be avoided. Use an enum.
enum ExtractTargetMode {
AllowExternalReferences,
ForbidExternalReferences
};
...
> Source/WebCore/svg/SVGURIReference.cpp:49
> + if (funcUri.find('#') == notFound)
This code path is _very_ hot. If you're at it, I'd propose to make it more efficient (like your old work years ago where you made the path parsing more efficient, by directly operating on the const UChar* characters buffer).
> Source/WebCore/svg/SVGURIReference.cpp:50
> + return String();
s/String()/emptyString()/ - use the shared empty string for more efficiency.
> Source/WebCore/svg/SVGURIReference.cpp:53
> if (url.startsWith("url(")) { // URI References, ie. fill:url(#target)
The initial decision here should be:
first character is 'u' or '#'.
Remember to always optimize for the common path, so checking wheter funcUri.find('#') returns notFound as first thing is inefficient, as you're doing avoidable extra-work for valid references.

> I don't think we support external references like that at all
Yes, but with your patch you would treat this as an internal reference was my point.
> Do you know any or have testcases?
Testcase is trivial to construct from the attached testcase. Just move the <defs> to a separate SVG file.
> Note that <a href> handling uses baseURI internally as well.
That's clearly false in terms of determining whether the reference is same-document (as opposed to resolving the URI). Consider this HTML document:
<!DOCTYPE html>
<base href="http://www.example.com">
<a href="#foo">Click me</a>
<div style="height: 5000px"></div>
<div id="foo">Do you see me?</div>
Clicking the link will load example.com, not scroll to the "Do you see me?" text as it would for a same-document reference. Your patch, however, would treat a URI like that for an SVG fill as same-document in this case.

Hi Niko,
(In reply to comment #9)
> (From update of attachment 100243[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100243&action=review
>
> In general this looks fine, some comments though:
>
> > Source/WebCore/svg/SVGURIReference.cpp:47
> > -String SVGURIReference::getTarget(const String& url)
> > +String SVGURIReference::getTarget(const String& funcUri, Document* document, bool allowExternal)
>
> How about naming it, extractTargetURI, if you're at it?
I left the name open still, how about extractFragmentIdentifier? It is all about the fragmentIdentifer after all.
> A boolean parameter like "allowExternal" should be avoided. Use an enum.
> enum ExtractTargetMode {
> AllowExternalReferences,
> ForbidExternalReferences
> };
> ...
Good idea, fixed.
> > Source/WebCore/svg/SVGURIReference.cpp:49
> > + if (funcUri.find('#') == notFound)
>
> This code path is _very_ hot. If you're at it, I'd propose to make it more efficient (like your old work years ago where you made the path parsing more efficient, by directly operating on the const UChar* characters buffer).
>
> > Source/WebCore/svg/SVGURIReference.cpp:50
> > + return String();
>
> s/String()/emptyString()/ - use the shared empty string for more efficiency.
I keep forgetting this one :) Fixed.
> > Source/WebCore/svg/SVGURIReference.cpp:53
> > if (url.startsWith("url(")) { // URI References, ie. fill:url(#target)
>
> The initial decision here should be:
> first character is 'u' or '#'.
> Remember to always optimize for the common path, so checking wheter funcUri.find('#') returns notFound as first thing is inefficient, as you're doing avoidable extra-work for valid references.
The optimize thing was made a lot easier by an observation: most of the users of getTarget are of the <uri>, not <funcUri> kind! Also the css parser does actual parsing of url() so all call sites have url() removed before calling getTarget. In fact we were too tolerant because xlink:href="url(#foo)" on for example <use> was allowed, doesn't work in Opera and FF.
Note that this does not completely fix the testcases Boris gave. I think this is a step in the right direction though, let's decide whether to keep this bug or open a new one for the problem of referencing external fragment identifiers.
Cheers,
Rob.

Created attachment 100416[details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

> In fact we were too tolerant because xlink:href="url(#foo)" on for example
> <use> was allowed
That's not "tolerant"; it's just buggy.
In general, I would really prefer it if you'd err on the side of making things that should not work per spec actually not work, even if you don't make things like external references work. That reduces the amount of well-poisoning caused by WebKit (especially on mobile sites) and eases later implementation of the missing features.
And note that the behavior implemented here is _still_ inconsistent with how <a href> is handled in WebKit...

(In reply to comment #15)
> > In fact we were too tolerant because xlink:href="url(#foo)" on for example
> > <use> was allowed
>
> That's not "tolerant"; it's just buggy.
I agree, we've been very "liberal" over the past years - which is not a good thing. Recently (a year ago) I've started with making our SVG DOM primitive types very strict, aka. we throw a lot more exceptions, just like you do - if eg. arguments are wrong, or missing,, etc..
Revisiting the fragment reference evaluation code and making it strict is a logical next step in that regard.
So I agree, we should be much more strict about these things.
>
> In general, I would really prefer it if you'd err on the side of making things that should not work per spec actually not work, even if you don't make things like external references work.
Just to get us right: we're willing to add support for external references - we'll definately come to that this year.
> That reduces the amount of well-poisoning caused by WebKit (especially on mobile sites) and eases later implementation of the missing features.
True.
>
> And note that the behavior implemented here is _still_ inconsistent with how <a href> is handled in WebKit...
I've checked your testcase in WebKit trunk:
<!DOCTYPE html>
<base href="http://www.example.com">
<a href="#foo">Click me</a>
<div style="height: 5000px"></div>
<div id="foo">Do you see me?</div>
Clicking this link goes to example.com#foo as expected. Robs patch does not change this behaviour in any way - he just alters the way SVG*Elements deriving from SVGURIReference resolve the uris.
You've convinced me that the approach by just checking whether the passed target URI starts with "#" to determine whether it's a same-document origin is insufficient.
http://www.w3.org/TR/SVG/linking.html#IRIReference says:
"An IRI can also address a particular element within an XML document by including an IRI fragment identifier as part of the IRI. An IRI which includes an IRI fragment identifier consists of an optional base IRI, followed by a "#" character, followed by the IRI fragment identifier. For example, the following IRI can be used to specify the element whose ID is "Lamppost" within file someDrawing.svg:
http://example.com/someDrawing.svg#Lamppost"
Most interessting is: An IRI which includes an IRI fragment identifier consists of an optional base IRI, followed by a "#" character, followed by the IRI fragment identifier.
I think what we have to do is:
- Check whether target contains a '#' character.
If true, and the position is > 0, extract the base uri string from position (0 .. PosOf#-1).
Otherwhise use the <base> elements URI as base uri.
That will always form an uri of type "baseURI#fragment". Then we can compare baseURI with our documents URI, if that matches the fragment reference is within the document, and the target element can be looked up with getElementById.
- If there's no '#' character in the IRI string, bail out.
The SVGURIReference::getTarget currently is mostly used in conjunction with document()->getElementById(SVGURIReference::getTarget(myXlinkHrefAttr)) - this will not work together with external documents, in future.
I'm questioning the whole method now. We should rather have sth. like
"SVGElement* targetElementFromIRIString(SVGElement* contextElement, onst String& iriString)".
It will do the checks I described above (lookup '#' char, extract base uri (or use from <base> element)) and:
If mode == AllowExternalReferences: ask document->getElementById(fragmentIdentifier), if the (synthesized) baseURI of the fragment string and the document base uri match, otherwise return 0 (FIXME: This would be the place than to request external documents in future).
If mode == ForbidExternalReferences: if baseURIs match, lookup element, otherwise return 0.
(This mode is only used by SVGSMILElement, as we only want to animate local elements).
This should unify the handling of <a href> and SVGURIReference, and prepares us further to support external references.

Hi Niko,
(In reply to comment #16)
>
> I'm questioning the whole method now. We should rather have sth. like
> "SVGElement* targetElementFromIRIString(SVGElement* contextElement, onst String& iriString)".
>
> It will do the checks I described above (lookup '#' char, extract base uri (or use from <base> element)) and:
> If mode == AllowExternalReferences: ask document->getElementById(fragmentIdentifier), if the (synthesized) baseURI of the fragment string and the document base uri match, otherwise return 0 (FIXME: This would be the place than to request external documents in future).
> If mode == ForbidExternalReferences: if baseURIs match, lookup element, otherwise return 0.
> (This mode is only used by SVGSMILElement, as we only want to animate local elements).
>
> This should unify the handling of <a href> and SVGURIReference, and prepares us further to support external references.
I agree with this, right now every time we use getTarget we just want to use it to find an Element in the doc (using getElementById). I am happy this is cleared up now, plus the fact that we do not have to worry about funcUri, just iri. Will work on a new patch.
Cheers,
Rob.

> Clicking this link goes to example.com#foo as expected.
Right, so it's not treated as a same-document reference.
> Robs patch does not change this behaviour in any way
Right. But it changes the way URI references are handled in SVG in a way that makes xlink:href="#foo" in SVG in the same situation be treated as a same-document reference (which it was already, of course, so the fact that it's treated same-document is not a change, but as long as you're changing the code anyway....).
In any case, your description of what probably needs to be done here (starting "I think what we have to do") sounds about right. Note that the "optional base IRI" can be relative.
> and the document base uri match
Except this isn't what <a href> does. It compares to the document URI, not the document base URI.

Created attachment 100625[details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 100660[details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

> The <html:a> example with the <base> uri works for <svg:a> as well -
> the <base> URI is respected for both types of anchor elements.
> Robs patch fixes IRI reference resolution in the same way
The _resolution_ is the same. The decision about whether it's a same-document reference is not.
<html:a> treats same-document references by scrolling and other-document references by loading a new page.
SVG effects should be treating same-document references by using getElementById and other-document references by loading the other document and then calling getElementById on it.
So to be clear, my issue is with this line:
+ || equalIgnoringFragmentIdentifier(kurl, document->baseURI()))
For <html:a> the equivalent code uses the document URI, not the base URI, as the second argument.

(In reply to comment #29)
> SVG effects should be treating same-document references by using getElementById and other-document references by loading the other document and then calling getElementById on it.
We haven't implemented support yet for loading external references at all? (TODO)
Are you concerned that Robs patch is not correct for external referenced iris?
> So to be clear, my issue is with this line:
>
> + || equalIgnoringFragmentIdentifier(kurl, document->baseURI()))
>
> For <html:a> the equivalent code uses the document URI, not the base URI, as the second argument.
Which equivalent code? What code are you looking at?

> We haven't implemented support yet for loading external references at all?
Yes, but the attached patch makes references that are same-document in Gecko and Presto not be same-document in WebKit and vice versa.
> What code are you looking at?
I'm not that familiar with the WebKit codebase, but FrameLoader::shouldReload and NavigationScheduler::scheduleLocationChange look related. There's a similar check in FrameLoader::loadInSameDocument too, but at that point you already know you're doing a same-document scroll.

(In reply to comment #31)
> > We haven't implemented support yet for loading external references at all?
>
> Yes, but the attached patch makes references that are same-document in Gecko and Presto not be same-document in WebKit and vice versa.
Can you construct a testcase? I think you're mixing up something.... but if you'd have a testcase Rob could easily check it. I'd rather get this right and cleared up before landing anything.
>
> > What code are you looking at?
>
> I'm not that familiar with the WebKit codebase, but FrameLoader::shouldReload and NavigationScheduler::scheduleLocationChange look related. There's a similar check in FrameLoader::loadInSameDocument too, but at that point you already know you're doing a same-document scroll.
Um, that's not really related to SVGs handling of IRI references and or anchors. (No worries, I know it's not your codebase :-)

Created attachment 102051[details]
Testcase showing difference in SVG same-document reference behavior
> Can you construct a testcase?
Here's a testcase that shows a black square in Presto and Gecko but will show a red square in WebKit with the proposed patch because Presto and Gecko treat this reference as an external reference while WebKit will treat it as a same-document reference.

I can construct a testcase going in the other direction too (where WebKit will treat a reference as not same-document while Presto and Gecko will treat it as same-document), but that involves knowing the URI of the testcase while writing it, so makes it hard to attach to Bugzilla. Let me know if you absolutely need it for some reason.
> Um, that's not really related to SVGs handling of IRI references and or
> anchors.
It's not related to the former, but it looks to me like it might be related to the latter. If not, where does WebKit decide whether following an anchor should scroll or do a new load?

(In reply to comment #39)
> > The bug is that we're comparing against the document->baseURI() instead of
> > the documentURI itself.
>
> Yes, that's what comment 4 said. ;)
Fun! If you'd have used the review tool to annotate your comment to the right line (equalIgnroingFragmentIdentifier) I would have noticed it earlier! :-)
Anyhow, great that we've reached conclusion here...

Comment on attachment 102137[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=102137&action=review
Next round of comments :-)
> LayoutTests/svg/custom/invalid-url-reference.svg:3
> + There should be no red
Better make this an inline comment, this way we can potentially share the expected.png
> LayoutTests/svg/custom/invalid-url-reference.svg:16
> + <rect x="0" y="0" width="100" height="100" style="fill: url(http://www.example.com/something#orange_red)"/>
The name of this test is misleading, after all the url is valid! Can you come up with a better name? (After all this is about not picking up the same-document reference...)
> LayoutTests/svg/custom/linking-base-external-reference.xhtml:4
> + There should be no red
Ditto.
> LayoutTests/svg/custom/linking-base-external-reference.xhtml:11
> + stop-opacity:1"/>
The stop opacity is useless in all cases just whip it everywhere.
> LayoutTests/svg/custom/uri-reference-handling.svg:12
> + <stop offset="0%" style="stop-color:rgb(255,0, 0);"/>
How about using red everywhere instead of this rgb(255,0,0) ?
> LayoutTests/svg/custom/uri-reference-handling.svg:18
> +<rect width="50" height="50" fill="url(../custom/uri-reference-handling.svg#green)"/>
> +<rect x="50" width="50" height="50" fill="url(http://www.example.org/test#red) green"/>
> +<rect id="rect3" y="50" width="50" height="50" fill="red"/>
> +<rect x="50" y="50" width="50" height="50" fill="url(no_fragment_identifier) green"/>
Can you make each of them have a padding of 10px?
0x0 - 50x50
60x60 - 110x110
...
Easier to see that the final image is composed of several rects.
> Source/WebCore/ChangeLog:8
> + Change getTarget to be more strict about iri resolving. In particular, test for same-document or
s/getTarget/SVGURIReference::getTarget/
> Source/WebCore/ChangeLog:11
> + external referencing, the latter case we do not handle for now. Accept as same-document if the
> + iri when absolute equals the document url, or if the iri is relative when the relative part combined
> + with the base uri equals the document url.
If the iri when? Please rephrase :-)
> Source/WebCore/ChangeLog:12
> + For convenience a method is added to get the target of the iri, if found.
s/to get the target/to lookup the element/
> Source/WebCore/ChangeLog:14
> + funcIRI handling is not needed since CSS parser strips this for us.
> +
If you mention funcIRI, you should include the definition of SVG iris, otherwhise this is confusing.
> Source/WebCore/css/CSSCursorImageValue.cpp:75
> + if (SVGCursorElement* cursorElement = resourceReferencedByCursorElement(SVGURIReference::fragmentIdentifierFromIRIString(url, referencedElement->document()), referencedElement->treeScope()))
Can't you encapsulate this call right within resourceReferencedByCursorElement, to avoid repeating this? (you just have to pass the Element instead of the TreeScope to that method).
> Source/WebCore/css/CSSFontFaceSource.cpp:135
> + m_externalSVGFontElement = m_font->getSVGFontById(SVGURIReference::fragmentIdentifierFromIRIString(m_string, fontSelector->m_document, AllowExternalReferences));
Why are external refs allowed here, and not in cursors? If there's a reason it should be mentioned in the ChangeLog and in a comment.
> Source/WebCore/css/CSSFontSelector.h:70
> + friend class CSSFontFaceSource;
How about adding a public Document* accessor instead of adding friendship?
> Source/WebCore/css/SVGCSSStyleSelector.cpp:364
> + svgstyle->setMarkerStartResource(SVGURIReference::fragmentIdentifierFromIRIString(s, m_element->document()));
External refs are forbidden here?
I think we should change the default to be "AllowExternalReferences", that would make the exceptions easier to spot (aka. those who disallow external refs).
Can you make a list of forbidden ones, with spec refs? :-)
> Source/WebCore/svg/SVGURIReference.cpp:58
> + if ((mode == AllowExternalReferences && kurl.isValid()) || equalIgnoringFragmentIdentifier(kurl, document->url()))
How about you add a comment here, as in the ChangeLog, what these checks are for.
> Source/WebCore/svg/SVGURIReference.cpp:70
> + // FIXME: handle external references properly
Handle external references.
We're supposed to use full phrases in comments.
Ideally you'd include a bug link here, to the relevant bug.
> Source/WebCore/svg/SVGURIReference.h:48
> + static String fragmentIdentifierFromIRIString(const String&, Document*, ExtractTargetMode = ForbidExternalReferences);
> + static Element* targetElementFromIRIString(const String&, Document*, String* = 0, ExtractTargetMode = ForbidExternalReferences);
I think we should assure that we're using the right default here from the beginning.
Even if we don't support external references right now...

Comment on attachment 102137[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=102137&action=review> Source/WebCore/css/SVGCSSStyleSelector.cpp:396
> - svgstyle->setMarkerEndResource(SVGURIReference::getTarget(s));
> + svgstyle->setMarkerEndResource(SVGURIReference::fragmentIdentifierFromIRIString(s, m_element->document()));
Oops, we silently assume same-document references here! If we just extract the fragment identifier here, we're going to ignore any external URL.
We should store the whole IRI string here, and lookup the targetelmenetForIRIString later in rendering/ when we actually use the marker end resource.
That affects all usages of fragmentIdentifierFromIRIString - thery're all assuming same-doc refs, which is wrong.
(Parts from our IRC discussion)

(In reply to comment #44)
> (From update of attachment 102137[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102137&action=review
>
> > Source/WebCore/css/SVGCSSStyleSelector.cpp:396
> > - svgstyle->setMarkerEndResource(SVGURIReference::getTarget(s));
> > + svgstyle->setMarkerEndResource(SVGURIReference::fragmentIdentifierFromIRIString(s, m_element->document()));
>
> Oops, we silently assume same-document references here! If we just extract the fragment identifier here, we're going to ignore any external URL.
> We should store the whole IRI string here, and lookup the targetelmenetForIRIString later in rendering/ when we actually use the marker end resource.
>
> That affects all usages of fragmentIdentifierFromIRIString - thery're all assuming same-doc refs, which is wrong.
> (Parts from our IRC discussion)
I tried to incorporate it in my patch, but it is quite tricky. I propose to leave it out for now and do it in bug 65344, I can move the comment to that bug as well as reminder.
Cheers,
Rob.

(In reply to comment #46)
> > That affects all usages of fragmentIdentifierFromIRIString - thery're all assuming same-doc refs, which is wrong.
> > (Parts from our IRC discussion)
>
> I tried to incorporate it in my patch, but it is quite tricky. I propose to leave it out for now and do it in bug 65344, I can move the comment to that bug as well as reminder.
Also note that it would change a lot of the expected results.
Cheers,
Rob.

(In reply to comment #46)
>
> I tried to incorporate it in my patch, but it is quite tricky. I propose to leave it out for now and do it in bug 65344, I can move the comment to that bug as well as reminder.
Oops, I missed that comment, sure if you want to work on it soon, no problem! r=me then!