309020 - SVG references breaks when used inside XBL

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Loune]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

SVG references can't be resolved when an inline <SVG> is in the <content>
section of a XBL binding. Eg - if a SVG element (inside XBL content) has a style
of fill:url(#gradient2222), #gradient2222's URI would be the XBL and not the
owner HTML. Mozilla thinks it's an off-doc reference and ignores it.

Reproducible: Always

Steps to Reproduce:
See attachment
Actual Results:  
There is no gradient on the SVG inside the XBL while the standalone SVG shows
the gradient

Expected Results:  
Both should show the gradient

Comment 1

[Loune]

Attachment: XBL test file containing SVG

Comment 2

[Loune]

Attachment: html file that uses the XBL

Comment 3

[Loune]

Attachment: standalone SVG file

Comment 4

[Loune]

Attachment: my first patch

Patch fixes problem by testing if it's an anonymous element and using
corresponding functions to get the referenced element.

Comment 5

[Martijn Wargers (dead)]

You haven't really asked review from anyone, you need to fill in e-mail addresses. 
Maybe Boris knows who should review this.

Comment 6

[Boris Zbarsky [:bzbarsky]]

tor or jwatt, I guess, but the real problem is that the base URI of URI
references in XBL is not very well defined...  Which means I'm not convinced the
desired behavior is "correct" per the current XBL setup.

Comment 7

[Mike Schroepfer]

Comment on attachment 196505 [details] [diff] [review]
my first patch

Unsetting review flags as no requestee was set.  Please request review from
specific person.

Comment 8

[Loune]

(In reply to comment #6)
> tor or jwatt, I guess, but the real problem is that the base URI of URI
> references in XBL is not very well defined...  Which means I'm not convinced the
> desired behavior is "correct" per the current XBL setup.

I'm new to XBL, but I'm assuming that when the binding is imported into a
document that the base URI of any content inside the binding becomes that of the
document.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> that the base URI of any content inside the binding becomes that of the
> document.

It does, but that's a known bug; it should remain the base URI of the binding.

Comment 10

[Mike Schroepfer]

Please complete reviews and land on trunk before requesting 1.8b5 approval.  

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Given that our XBL is the way it is, isn't this a good idea, at least for
branch, at least for the pos==0 case?

Comment 12

[tor]

roc: I'm not familiar with how XBL is implemented in moz, so if you could review
this for correctness in that regard, it would be much appreciated.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 196505 [details] [diff] [review]
my first patch

I've tested this, and it works, and it makes sense. It also seems very low risk
since it /pretty clearly only affects SVG in XBL. We should take it, on branch
too.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196505 [details] [diff] [review]
my first patch

I can comment on those parts....

>Index: nsSVGUtils.cpp
>@@ -176,26 +180,58 @@ nsresult nsSVGUtils::GetReferencedFrame(

I needed to understand this method in general, so I started reading at the
top... some comments on the part before this patch:

1)  Is "#" supposed to be a valid value for uriSpec?  This code accepts it.
2)  Why are we hand-parsing URI strings?  This is a big no-no in Gecko code 
    in general, usually because people do it wrong.  Please create URI objects
    to parse URI strings for you.  This does cause some issues with protocols
    like data: that we need to resolve, but the problem there is that it's not
    obvious what fragment identifiers on those URIs would mean and whether
    they are even allowed.

>+  nsCOMPtr<nsIDOMElement> bindingParent = do_QueryInterface(aContent->GetBindingParent(), &rv);
>+
>+  // we should skip the whole URL checking process if it starts with hash (pos==0)
>+  // since it will be local anyways

Is that guaranteed in the presence of xml:base?  That is, what does "local"
mean when the base URI of a referencing node doesn't match that of the
document?

>+    if (NS_SUCCEEDED(rv) && bindingParent) {
>+      // is anonymous element, get the URI of binding for comparing

Note that this might be a native anonymous element (either via XTF or one of
our other native anonymous content mechanisms), in which case there is no
binding involved.

>+      rv = cssDecl->GetPropertyValue(NS_LITERAL_STRING("-moz-binding"), bindingURIS);
>+      if (NS_FAILED(rv)) return rv;

There is no guarantee that this is the actual binding bound to the node, since
there are DOM methods to attach bindings.  You probably want to pass the
binding parent to the binding manager and ask it for the XBL binding; if you
get one back, that's the one you want.

>+      // strip off url(...#yyy)
>+      bindingURIS.Right(bindingURIS2, bindingURIS.Length()-4);
>+      bindingURIS2.Left(bindingURIS, bindingURIS2.FindChar('#'));

Please do not hand-parse URIs....

>+    NS_NewURI(getter_AddRefs(targetURI), aURISName, nsnull, myDoc->GetBaseURI());

Why myDoc->GetBaseURI()? That messes up xml:base, no?  And in the non-XBL case,
since myURI is the documentURI, it messes up any document for which the
documentURI and baseURI are not the same.

>+    myURI->Equals(targetURI, &match);

So for the XBL case we're comparing the binding URI to the document base URI of
our current document?  Won't that always fail to match?  So all this only works
for URIs of the form "#..." with this code?

>+    rv = xblDoc->GetAnonymousElementByAttribute(bindingParent, NS_LITERAL_STRING("id"), aURIName, getter_AddRefs(element));

Won't this find various random content with "id" attributes?  Not all
null-namespaced attributes named "id" are IDs, and not all IDs are
null-namespaced attributes named "id" (at least assuming we actually implement
xml:id, and I'd really rather not have to remember to fix this code when we
do).

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

uriSpec is, apparently, absolute-ized when passed in here, so what actually
happens is that myDoc->GetBaseURI() is ignored and the right thing happens.
Maybe this function should be rewritten to require (and assert) that the URI is
absolute.

Comment 16

[Boris Zbarsky [:bzbarsky]]

It's absolutized for gradients?  See nsSVGGradientFrame::checkURITarget.  I'm
not sure I see where that's happening here... we seem to be just using the
animVal of xlink:href, what am I missing?  Unless we absolutize xlink:href for
internal storage for SVG?

I'll agree that nsSVGUtils::GetPaintType always passes in an absolute URI (which
it got from an nsIURI right there!).  And that markers do the same, and some
other callers...

Comment 17

[Boris Zbarsky [:bzbarsky]]

If every caller or close to it has or should have an nsIURI, this function
should just take nsIURI, fwiw.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Here's a revised patch that attempts to address all the problems. I haven't
compiled it yet, let alone tested it ... I'll get to that tomorrow.

Comment 19

[Boris Zbarsky [:bzbarsky]]

That should probably ensure that documentURI QIs to nsIURL; not all URIs do, and
this patch crashes for those that don't.

There are a few other callers of GetReferencedFrame that need fixing.

But generally, that looks much happier!

And on trunk, once we've got this in we can make MatchElementID faster.  But
that's for after we get this done.  ;)

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix #2

I fixed up the other call sites. In most cases it's easy because there's
already a URI. For the other cases I just used mContent->GetBaseURI() as the
base URI. For regular XBL content this will give the bound document, not the
binding document but IMHO GetBaseURI() implementations should be changed to
address that, and for now we just won't fix that case.

I also factored out most of the code into nsContentUtils::GetReferencedElement
so other code can use it and we don't have to expose so much content guts to
SVG!

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 199202 [details] [diff] [review]
fix #2

>Index: content/base/src/nsContentUtils.cpp
>+MatchElementId(nsIContent *aContent,
>+               const nsACString& aUTF8Id, const nsAString& aId)

Please file a followup bug on me for trunk to make this faster?

>+nsContentUtils::GetReferencedElement(nsIURI* aURI, nsIContent *aFromContent)

>+  nsCOMPtr<nsIDOMElement> bindingParent =
>+    do_QueryInterface(aFromContent->GetBindingParent(), &rv);

That looks like a carryover from the original, DOM-method-using, approach. 
Just do:

  nsIContent* bindingParent = aFromContent->GetBindingParent();

and no need for all the QIing back and forth (to bpContent, etc).

>+    bindingManager = doc->BindingManager();
>+    if (bindingManager) {

It's guaranteed non-null, hence no "Get" prefix on the accessor.

>+        // If this is an anonymous XBL element then the URI is
>+        // relative to the binding document. A full fix requires a
>+        // proper XBL2 implementation but for now URIs that are
>+        // relative to the binding document should be resolve to the
>+        // copy of the target element that has been inserted into the
>+        // bound document.

Could you add an "// XXX sXBL/XBL2 isssue" comment here?  This doesn't work
right with xml:base in the binding, but we can't fix that till we fix our
ownerDocument story.

>+  // Get a unicode string
>+  NS_ConvertUTF8toUTF16 ref(refPart);

Do we need to worry about %-escapes inside the ref?  If we do, please file a
followup bug on it, ok?

>+  // Get the domDocument
>+  nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(doc);
>+  NS_ASSERTION(domDoc, "Content doesn't reference a dom Document");

This should be in the !isXBL block, right?  We don't need domDoc if |isXBL|.

>+      if (length) {
>+        for (PRUint32 i = 0; i < length && !content; ++i) {

No need for |if (length)| -- the |for| condition handles that part.

The various places that you call NS_NewURI should probably call
nsContentUtils::NewURIWithDocumentCharset

>Index: layout/svg/base/src/nsSVGUtils.cpp

>+  *aRefFrame = aPresShell->GetPrimaryFrameFor(content);
>   NS_ASSERTION(*aRefFrame, "Get referenced SVG frame -- can't find primary frame");

While you're here, could you remove that assert?  There's no reason for a frame
to exist there.

sr=bzbarsky with those fixed.  Thanks for picking this up, roc!

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix #3

tor, you can just review the SVG bits I guess, which isn't much.

Boris, I'm not sure what this issue is:
> Do we need to worry about %-escapes inside the ref?  If we do, please file a
> followup bug on it, ok?
You mean GetRef() would return something like "foo%20bar" and we should
manually convert that to "foo bar" for ID-lookup?

Comment 23

[Boris Zbarsky [:bzbarsky]]

> You mean GetRef() would return something like "foo%20bar" and we should
> manually convert that to "foo bar" for ID-lookup?

Yep, exactly...  See what nsContentSink::ScrollToRef does, for example.  Or what
nsDocShell::ScrollIfAnchor does at
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#7099

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

but does nsIURI *guarantee* that GetRef returns something escaped, even if I
construct the URI with unescaped data?

Comment 25

[Boris Zbarsky [:bzbarsky]]

I'm afraid it doesn't guarantee much of anything past (nsIURL.idl):

   71      * Some characters may be escaped.

I'm not sure what our nsStandardURL implementation actually does (darin, do you
know offhand?) as far as the ref goes.  But do we really want to assume no one
is writing nsIURL impls per the frozen API?

Comment 26

[Darin Fisher]

nsStandardURL::GetRef returns an ASCII-valued string.  The API allows it to
return UTF-8, and it will do so if a hidden pref is enabled, but since so much
of our codebase expects ASCII-valued URL parts, we do not enable that pref by
default.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let me get this straight ... if GetRef returns "foo%20bar", should we
un-%-escape or not? I.e., does it mean "foo bar" or "foo%20bar" or "hmmm I dunno"?

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

we always escape the space, irrespective of the pref, do we not?

js> ios.nsIIOService.newURI("http://foo/bar#as df", null, null).spec
http://foo/bar#as%20df
js> ios.nsIIOService.newURI("http://foo/bar#as df", null, null).nsIURL.ref
as%20df
js> prefs.nsIPrefBranch.getBoolPref("network.standard-url.encode-utf8")
true

So, I believe you should unescape the ref part and convert it to unicode using
the origin charset of the URI. (maybe using nsITextToSubURI::unEscapeURIForUI,
though I suppose it's not really for UI purposes here)

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix #4

Does this treatment of unescaping/charset conversion look OK?

I'm not sure what happens with charsets here. Getting non-ASCII refs to work
right can be a job for another day.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

fwiw, on trunk (i.e. with the encode-url pref to false), any non-ASCII character
in a URL will be converted using the page charset, e.g.:
data:text/html;charset=iso-8859-1,<a
href="http://wintermute/&auml;&ouml;&uuml;">asdf

Clicking the link shows that they are converted to one byte each.

Comment 31

[tor]

Comment on attachment 199337 [details] [diff] [review]
fix #4

>+static PRBool EqualExceptRef(nsIURL* aURL1, nsIURL* aURL2)
>+{
>+  nsCOMPtr<nsIURI> u1;
>+  nsCOMPtr<nsIURI> u2;
>+
>+  nsresult rv = aURL1->Clone(getter_AddRefs(u1));
>+  if (NS_SUCCEEDED(rv)) {
>+    aURL1->Clone(getter_AddRefs(u2));
>+  }

Second line should refer to aURL2 instead of aURL1.  With that, r=tor.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix #5

Okay, this does the full-on charset conversion. I took the conversion logic out
of nsContentSink where it was being used to convert refs and made it a utility
function.

I also noticed that this doesn't work in the testcase in this bug. The problem
is that the attribute reference doesn't work; we take it as relative to
mContent->GetBaseURI, which in this case is the bound (HTML) document not the
binding (XBL) document. This is really a bug in GetBaseURI. I still think we
should take this patch on trunk because it contains some useful abstractions
and cleanup, and works for inline style references in anonymous content.

This has already been reviewed considerably so I just want biesi to take one
more look at it before I check in.

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 199851 [details] [diff] [review]
fix #5

+    if (aContent->HasAttr(kNameSpaceID_None, nsHTMLAtoms::id)) {

don't we have a AttrValueIs function these days?

+  nsPromiseFlatCString flatInput(aInput);

more usual would be:

const nsString& flatInput = PromiseFlatString(aInput);

However, I think you could avoid all that with .BeginReading() and .get() on
that iterator together with .Length() - no nulltermination needed

+    aOutput.Assign(ustr, dstLen);

fwiw this does not require nulltermination (if you pass the length like you do)

This could maybe be optimized using BeginWriting() if you felt like it :)

+    aURL2->Clone(getter_AddRefs(u2));

why not assign this return value to rv too?

+  url->GetOriginCharset(charset);
+  nsAutoString ref;
+  nsresult rv = ConvertStringFromCharset(charset, refPart, ref);

Hm... does this deal with an empty string as charset? For URIs, empty origin
charsets mean UTF-8.

+  if (NS_FAILED(rv)) {
+    CopyUTF8toUTF16(refPart, ref);

maybe copying the escaped ref part would be better?

r=biesi I think, but at least the UTF-8 origin charset case needs fixing...

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #33)
> (From update of attachment 199851 [details] [diff] [review] [edit])
> +    if (aContent->HasAttr(kNameSpaceID_None, nsHTMLAtoms::id)) {
> 
> don't we have a AttrValueIs function these days?

Dunno but I'm not going to change this copied code.

> +  nsPromiseFlatCString flatInput(aInput);
> 
> more usual would be:
> 
> const nsString& flatInput = PromiseFlatString(aInput);
> 
> However, I think you could avoid all that with .BeginReading() and .get() on
> that iterator together with .Length() - no nulltermination needed

Not worth it IMHO

> +    aOutput.Assign(ustr, dstLen);
> 
> fwiw this does not require nulltermination (if you pass the length like you do)
> 
> This could maybe be optimized using BeginWriting() if you felt like it :)

Ditto :-)

> +    aURL2->Clone(getter_AddRefs(u2));
> 
> why not assign this return value to rv too?

Fixed.

> +  url->GetOriginCharset(charset);
> +  nsAutoString ref;
> +  nsresult rv = ConvertStringFromCharset(charset, refPart, ref);
> 
> Hm... does this deal with an empty string as charset? For URIs, empty origin
> charsets mean UTF-8.

I've changed ConvertStringFromCharset to treat an empty charset as UTF8 (and we
can optimize that by just calling CopyUTF8toUTF16 instead of going through a
component).

> +  if (NS_FAILED(rv)) {
> +    CopyUTF8toUTF16(refPart, ref);
> 
> maybe copying the escaped ref part would be better?

Not sure what you mean but I don't think so.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked into trunk.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> don't we have a AttrValueIs function these days?

This is where the followup optimization bug comes in.

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

>> +  if (NS_FAILED(rv)) {
>> +    CopyUTF8toUTF16(refPart, ref);
>> 
>> maybe copying the escaped ref part would be better?
>Not sure what you mean but I don't think so.

Well, refPart at this point is the unescaped string, which may not necessarily
be valid UTF-8. so, I was trying to say, maybe the original ref string should be
converted tu UTF-16, instead of the unescaped one.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Filed bug 313082 for optimizing MatchElementId a tad.

Comment 39

[Boris Zbarsky [:bzbarsky]]

This seems to have regressed.  I filed bug 387466 on the problem.

Comment 40

[Jonathan Watt [:jwatt]]

We have some XBL tests for SVG in reftests/svg/moz-only now. reftests/svg/moz-only/xbl-grad-ref--grad-in-binding-02.svg specifically covers this bug.