Closed Bug 801467 Opened 7 years ago Closed 6 years ago

Paint servers don't work in SVG glyphs

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: eflores, Assigned: eflores)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 10 obsolete files)

16.84 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
112.85 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch Fix (obsolete) — Splinter Review
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)
Attached patch Fix (obsolete) — Splinter Review
Attachment #671336 - Attachment is obsolete: true
Attachment #718612 - Flags: review?(roc)
Attached patch Test (obsolete) — Splinter Review
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.
Attached patch Fix (obsolete) — Splinter Review
Attachment #718612 - Attachment is obsolete: true
Attachment #718612 - Flags: review?(roc)
Attachment #719706 - Flags: review?(roc)
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.
Attached patch Fix (obsolete) — Splinter Review
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?
Attached patch Fix v2 (obsolete) — Splinter Review
(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.
Something like this should handle the situation.
Attachment #760353 - Flags: review?(roc)
Assignee: edwin → jfkthame
These patches really should be reviewed by a DOM or Necko peer.
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)
Attachment #763295 - Attachment is obsolete: true
Attachment #763295 - 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)
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
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
You need to log in before you can comment on or make changes to this bug.