Paint servers don't work in SVG glyphs

RESOLVED FIXED in mozilla26

Status

()

Core
Graphics: Text
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: eflores, Assigned: eflores)

Tracking

(Blocks: 1 bug)

Trunk
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

16.84 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
112.85 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 671336 [details] [diff] [review]
Fix
Attachment #671336 - Flags: review?(roc)
Comment on attachment 671336 [details] [diff] [review]
Fix

Review of attachment 671336 [details] [diff] [review]:
-----------------------------------------------------------------

What uses glyphs-801467-paintserver.svg?

It's not obvious to me what bug you're fixing here.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> What uses glyphs-801467-paintserver.svg?

It is in the SVG table of svg.woff.

> It's not obvious to me what bug you're fixing here.

Giving the SVG glyph documents a null principal meant that things like fill="url(#gradient)" couldn't be resolved before. This patch fixes that by passing in the principal taken from the font URI (when it has one).
(In reply to Edwin Flores [:eflores] [:edwin] from comment #3)
> It is in the SVG table of svg.woff.

Ah, Splinter did not show me that you were changing svg.woff.

> > It's not obvious to me what bug you're fixing here.
> 
> Giving the SVG glyph documents a null principal meant that things like
> fill="url(#gradient)" couldn't be resolved before. This patch fixes that by
> passing in the principal taken from the font URI (when it has one).

OK. Instead of constructing a null principal when there's no URI, I think it would be better to completely fail when there's no URI, since we don't want some features of SVG fonts to just mysteriously fail to work in some situations while the rest of the font works.

Having said that, I think there's still a conceptual problem here --- and a spec issue. If the font URI is "http://example.com/svg.woff" which contains tables of SVG documents referring to #gradient, then the full URI for those resources is "http://example.com/svg.woff#gradient", which doesn't sound right, especially since #gradient might resolve to different things in different documents. Really each document in the font should have a different URI.

I think the way to go here might be to construct a new URI scheme, say "fonttable:<UUID>", similar to blob: URIs, inheriting from nsSimpleURI so you support refs (see nsBlobURI). Then create a random UUID for each table (see nsUUIDGenerator) and make that the URI for its document. Do not support loading resources (i.e. creating nsIChannels) for such URIs!

Bug 792675 has some refactoring of nsBlobURI that might inspire you. It might make sense to use nsHostObjectURI from those patches.
Comment on attachment 671336 [details] [diff] [review]
Fix

Review of attachment 671336 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing until comments addressed.
Attachment #671336 - Flags: review?(roc)
Created attachment 718612 [details] [diff] [review]
Fix
Attachment #671336 - Attachment is obsolete: true
Attachment #718612 - Flags: review?(roc)
Created attachment 718613 [details] [diff] [review]
Test
Attachment #718613 - Flags: review?(roc)
Comment on attachment 718612 [details] [diff] [review]
Fix

Review of attachment 718612 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +334,5 @@
>  }
>  
>  nsresult
> +gfxSVGGlyphsDocument::ParseDocument(uint8_t *aBuffer, uint32_t aBufLen,
> +                                    const nsCOMPtr<nsIURI>& aFontURI)

I don't understand why we need to pass in aFontURI here.

::: gfx/thebes/gfxSVGGlyphs.h
@@ +92,5 @@
>      nsCOMPtr<nsIPresShell> mPresShell;
>  
>      nsBaseHashtable<nsUint32HashKey, Element*, Element*> mGlyphIdMap;
> +
> +    nsAutoCString mURI;

Call it mSVGGlyphsDocumentURI or something like that to make it clear this is the URI of the SVG glyphs document.
Created attachment 719706 [details] [diff] [review]
Fix
Attachment #718612 - Attachment is obsolete: true
Attachment #718612 - Flags: review?(roc)
Attachment #719706 - Flags: review?(roc)
Blocks: 832483
Comment on attachment 719706 [details] [diff] [review]
Fix

Review of attachment 719706 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +371,5 @@
> +    document->SetBaseURI(uri);
> +    document->SetPrincipal(principal);
> +
> +    nsHostObjectProtocolHandler::AddDataEntry(NS_LITERAL_CSTRING(FONTTABLEURI_SCHEME),
> +                                              domDoc, principal, mSVGGlyphsDocumentURI);

I don't think we need to add or remove this from the table at all, since nothing should be looking up this URI in the table.
Created attachment 719782 [details] [diff] [review]
Fix
Attachment #719706 - Attachment is obsolete: true
Attachment #719706 - Flags: review?(roc)
Attachment #719782 - Flags: review?(roc)
Comment on attachment 719782 [details] [diff] [review]
Fix

Review of attachment 719782 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +27,5 @@
>                                            nsISupports* aObject,
>                                            nsIPrincipal* aPrincipal,
>                                            nsACString& aUri)
>  {
> +  if (aUri.IsEmpty()) {

Why this if statement?
Created attachment 751574 [details] [diff] [review]
Fix v2

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Comment on attachment 719782 [details] [diff] [review]
> Fix
> 
> Review of attachment 719782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsHostObjectProtocolHandler.cpp
> @@ +27,5 @@
> >                                            nsISupports* aObject,
> >                                            nsIPrincipal* aPrincipal,
> >                                            nsACString& aUri)
> >  {
> > +  if (aUri.IsEmpty()) {
> 
> Why this if statement?

Leftover badness. Taken out.
Attachment #719782 - Attachment is obsolete: true
Attachment #719782 - Flags: review?(roc)
Attachment #751574 - Flags: review?(roc)
Comment on attachment 751574 [details] [diff] [review]
Fix v2

Review of attachment 751574 [details] [diff] [review]:
-----------------------------------------------------------------

Needs a test!

::: gfx/thebes/gfxSVGGlyphs.h
@@ +89,5 @@
>      nsCOMPtr<nsIPresShell> mPresShell;
>  
>      nsBaseHashtable<nsUint32HashKey, Element*, Element*> mGlyphIdMap;
> +
> +    nsAutoCString mSVGGlyphsDocumentURI;

nsCString. nsAutoCString isn't long enough to hold it.
Attachment #751574 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Comment on attachment 751574 [details] [diff] [review]
> Fix v2
> 
> Review of attachment 751574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Needs a test!

attachment 718613 [details] [diff] [review] ?
Comment on attachment 718613 [details] [diff] [review]
Test

Review of attachment 718613 [details] [diff] [review]:
-----------------------------------------------------------------

Right!
Attachment #718613 - Flags: review?(roc) → review+
Duplicate of this bug: 877796
Blocks: 878786
Comment on attachment 751574 [details] [diff] [review]
Fix v2

Review of attachment 751574 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +310,5 @@
> +#if DEBUG
> +  bool schemeIs;
> +  nsresult rv = uri->SchemeIs("fonttable", &schemeIs);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ASSERTION(schemeIs, "Non-fonttable spec in nsFontTableProtocolHander");

Do we want an assertion here, or should this be just a warning? It's perfectly possible to hit this, e.g. with a font that has an SVG glyph that includes something like <image xlink:href="foo.jpg"/>.

@@ +315,5 @@
> +#endif
> +
> +  uri.forget(aResult);
> +
> +  return NS_OK;

If the scheme is -not- fonttable (see above), should this function actually return an error rather than NS_OK?
Blocks: 871961
(In reply to Jonathan Kew (:jfkthame) from comment #18)
> ::: content/base/src/nsHostObjectProtocolHandler.cpp
> @@ +310,5 @@
> > +#if DEBUG
> > +  bool schemeIs;
> > +  nsresult rv = uri->SchemeIs("fonttable", &schemeIs);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  NS_ASSERTION(schemeIs, "Non-fonttable spec in nsFontTableProtocolHander");
> 
> Do we want an assertion here, or should this be just a warning? It's
> perfectly possible to hit this, e.g. with a font that has an SVG glyph that
> includes something like <image xlink:href="foo.jpg"/>.

That's a relative URL so the absolute URI would still have the fonttable scheme. How would we hit this code with a non-fonttable scheme?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> (In reply to Jonathan Kew (:jfkthame) from comment #18)
> > ::: content/base/src/nsHostObjectProtocolHandler.cpp
> > @@ +310,5 @@
> > > +#if DEBUG
> > > +  bool schemeIs;
> > > +  nsresult rv = uri->SchemeIs("fonttable", &schemeIs);
> > > +  NS_ENSURE_SUCCESS(rv, rv);
> > > +  NS_ASSERTION(schemeIs, "Non-fonttable spec in nsFontTableProtocolHander");
> > 
> > Do we want an assertion here, or should this be just a warning? It's
> > perfectly possible to hit this, e.g. with a font that has an SVG glyph that
> > includes something like <image xlink:href="foo.jpg"/>.
> 
> That's a relative URL so the absolute URI would still have the fonttable
> scheme. How would we hit this code with a non-fonttable scheme?

We hit this because the spec passed in to nsFontTableProtocolHandler::NewURI is simply the (relative) URL from the xlink:href attribute; it hasn't been resolved to an absolute URI. Is nsFontTableProtocolHandler::NewURI here supposed to resolve it relative to the given baseURI? That seems plausible (and the baseURI has the expected fonttable: scheme), but the code doesn't currently do it.

I started hitting this as soon as I happened to have an SVG glyph with an image link like that in it. I realize this is broken anyhow, inasmuch as the link isn't going to work as "intended" within an SVG glyph document - it won't be able to resolve the relative URL to the original file. But it still shouldn't be hitting an assertion, IMO; it should just silently fail to find the resource, perhaps issuing an NS_WARNING along the way.
(In reply to Jonathan Kew (:jfkthame) from comment #20)
> We hit this because the spec passed in to nsFontTableProtocolHandler::NewURI
> is simply the (relative) URL from the xlink:href attribute; it hasn't been
> resolved to an absolute URI. Is nsFontTableProtocolHandler::NewURI here
> supposed to resolve it relative to the given baseURI? That seems plausible
> (and the baseURI has the expected fonttable: scheme), but the code doesn't
> currently do it.

Relative URIs that are just a #ref do get resolved relative to aBaseURI:
  if (aSpec.Length() && aSpec.CharAt(0) == '#') {
    nsAutoCString absSpec;
    nsresult rv = aBaseURI->GetSpecIgnoringRef(absSpec);
    absSpec.Append(aSpec);

I guess you're saying other relative URIs like "foobar/baz#abc" cause the assertion to fail? That makes sense. I guess we should just add an "else" clause so that any other aSpecs just return a failure code.
Right, bare #ref is handled, but anything else just gets passed as-is to uri->SetSpec(), which can't do anything meaningful with a relative spec such as "foo.png", and we hit the assertion.
Created attachment 760353 [details] [diff] [review]
followup - relative URIs other than #ref should return failure, but not assert

Something like this should handle the situation.
Attachment #760353 - Flags: review?(roc)
Assignee: edwin → jfkthame
Attachment #760353 - Flags: review?(roc) → review+
These patches really should be reviewed by a DOM or Necko peer.
Created attachment 760354 [details] [diff] [review]
Give SVG glyph documents a legitimate principal and URI so that references to paint servers are able to be resolved  * *

Unbitrotted Edwin's original patch here to apply to current m-c trunk, and folded in the followup; carrying forward r=roc but requesting r?bz in addition, as per comment 24.
Attachment #760354 - Flags: review?(bzbarsky)
Comment on attachment 760354 [details] [diff] [review]
Give SVG glyph documents a legitimate principal and URI so that references to paint servers are able to be resolved  * *

>+#define FONTTABLEURI_SCHEME "fonttable"

"moz-fonttable", perhaps.  Especially if this is not web-exposed.

>+nsFontTableProtocolHandler::NewURI(const nsACString& aSpec,
>+                                    const char *aCharset,
>+                                    nsIURI *aBaseURI,
>+                                    nsIURI **aResult)

Fix the indent?

>+  if (aSpec.Length() && aSpec.CharAt(0) == '#') {

In this case, I think it would be better to CloneIgnoringRef aBaseURI and then SetRef() on it.  Especially if the non-ref part of these URIs can be long.

>+    document->SetBaseURI(uri);

Why do you need this bit?  Or the SetPrincipal call, for that matter?  Also, why are we using a null documentURI here?

>-    channel->SetOwner(principal);

Why can this go away? If we take it out, then the StartDocumentLoad will clobber the document's principal, no?
Attachment #760354 - Flags: review?(bzbarsky) → review-
Edwin, will you work on addressing Boris's comments here (as they relate to the original patch in attachment 751574 [details] [diff] [review]), or would you like me to try and look into this further?
Flags: needinfo?(edwin)
Yep, I'll sort it.
Flags: needinfo?(edwin)
Created attachment 763295 [details] [diff] [review]
Give SVG glyph documents a legitimate principal and URI so that references to paint servers are able to be resolved

Addressed comments in comment 26
Attachment #760354 - Attachment is obsolete: true
Attachment #763295 - Flags: review?(bzbarsky)
Attachment #763295 - Attachment is obsolete: true
Attachment #763295 - Flags: review?(bzbarsky)
Created attachment 763309 [details] [diff] [review]
Give SVG glyph documents a legitimate principal and URI so that references to paint servers are able to be resolved

This time addressed comments in comment 26
Attachment #763309 - Flags: review?(bzbarsky)
Comment on attachment 763309 [details] [diff] [review]
Give SVG glyph documents a legitimate principal and URI so that references to paint servers are able to be resolved

r=me, but I'd like to see some tests here
Attachment #763309 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #31)
> Comment on attachment 763309 [details] [diff] [review]
> Give SVG glyph documents a legitimate principal and URI so that references
> to paint servers are able to be resolved
> 
> r=me, but I'd like to see some tests here

Edwin created a test already (attachment 718613 [details] [diff] [review]); the patch will need updating before anything lands due to changes in the SVG-in-OT spec we're implementing, but aside from that it looks like it will still test the functionality. Are there other kinds of test you think we should have here?

I'll rebase the patch and fix up the existing test shortly.
Flags: needinfo?(bzbarsky)
Nope, that test seems to cover the basic testing I was thinking of.
Flags: needinfo?(bzbarsky)
Created attachment 795848 [details] [diff] [review]
Give SVG glyph documents a legitimate principal and URI so that references to paint servers are able to be resolved.

Rebased Edwin's patch to current tip, ready for landing; carrying forward r=roc,bz.
Attachment #751574 - Attachment is obsolete: true
Attachment #763309 - Attachment is obsolete: true
Created attachment 795849 [details] [diff] [review]
Test that url(#id) references work in OpenType SVG glyph documents.

Updated testcase for the latest version of the svg-in-ot spec (post-bug-906521); carrying forward r=roc.
Attachment #795849 - Flags: review+
Attachment #795848 - Flags: review+
Attachment #718613 - Attachment is obsolete: true
Attachment #760353 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a96be5082b0 (patch)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a32b2b75f33c (reftest)

(Reset the assignee to Edwin, as he really wrote the patch and test here - I've just been rebasing/updating to get them landed.)
Assignee: jfkthame → edwin
Target Milestone: --- → mozilla26
https://hg.mozilla.org/mozilla-central/rev/6a96be5082b0
https://hg.mozilla.org/mozilla-central/rev/a32b2b75f33c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.