878786 - assertion "Could not get loadGroup" when <image xlink:href="data:...."> is used in an SVG glyph

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

When I use a font with SVG-in-OpenType glyphs (bug 719286) where the SVG document uses an embedded <image> element with a data: URI (e.g. within <mask>), I'm hitting this assertion repeatedly; it looks like it fires once for each <image> that is used in the glyph being loaded:

###!!! ASSERTION: Could not get loadgroup; onload may fire too early: 'loadGroup', file content/base/src/nsContentUtils.cpp, line 2739
nsContentUtils::LoadImage(nsIURI*, nsIDocument*, nsIPrincipal*, nsIURI*, imgINotificationObserver*, int, imgRequestProxy**)+0x00000175 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00BCA365]
nsImageLoadingContent::LoadImage(nsIURI*, bool, bool, nsIDocument*, unsigned int)+0x0000050F [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00CCCDCF]
nsImageLoadingContent::LoadImage(nsAString_internal const&, bool, bool)+0x00000252 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00CCD922]
mozilla::dom::SVGImageElement::LoadSVGImage(bool, bool)+0x0000017C [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01A0843C]
mozilla::dom::SVGImageElement::AfterSetAttr(int, nsIAtom*, nsAttrValue const*, bool)+0x000000A2 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01A08512]
mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool)+0x00000460 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00B55180]
mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool)+0x0000039A [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00B54CDA]
nsXMLContentSink::AddAttributes(unsigned short const**, nsIContent*)+0x0000012C [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x011D041C]
nsXMLContentSink::HandleStartElement(unsigned short const*, unsigned short const**, unsigned int, int, unsigned int, bool)+0x00000613 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x011CD7E3]
nsXMLContentSink::HandleStartElement(unsigned short const*, unsigned short const**, unsigned int, int, unsigned int)+0x0000004A [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x011CD1CA]
non-virtual thunk to nsXMLContentSink::HandleStartElement(unsigned short const*, unsigned short const**, unsigned int, int, unsigned int)+0x0000004D [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x011CDC2D]
nsExpatDriver::HandleStartElement(unsigned short const*, unsigned short const**)+0x0000012F [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x005E1E4F]
_ZL25Driver_HandleStartElementPvPKtPS1_+0x00000069 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x005E5E09]
doContent+0x00000A91 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x035B4621]
contentProcessor+0x00000064 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x035B19F4]
doProlog+0x00000B0D [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x035AEAFD]
prologProcessor+0x000000A3 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x035ADFE3]
prologInitProcessor+0x00000063 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x035B9CE3]
MOZ_XML_Parse+0x00000271 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x035AB341]
nsExpatDriver::ParseBuffer(unsigned short const*, unsigned int, bool, unsigned int*)+0x0000024C [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x005E4C2C]
nsExpatDriver::ConsumeToken(nsScanner&, bool&)+0x0000043C [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x005E51EC]
non-virtual thunk to nsExpatDriver::ConsumeToken(nsScanner&, bool&)+0x00000042 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x005E57D2]
nsParser::Tokenize(bool)+0x000001BA [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x005F9D4A]
nsParser::ResumeParse(bool, bool, bool)+0x00000208 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x005F97E8]
nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long long, unsigned int)+0x000003B2 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x005FA662]
non-virtual thunk to nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long long, unsigned int)+0x0000004F [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x005FA9BF]
gfxSVGGlyphsDocument::ParseDocument(unsigned char const*, unsigned int)+0x00000978 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DF5D78]
gfxSVGGlyphsDocument::gfxSVGGlyphsDocument(unsigned char const*, unsigned int, hb_blob_t*)+0x00000088 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DF52E8]
gfxSVGGlyphsDocument::gfxSVGGlyphsDocument(unsigned char const*, unsigned int, hb_blob_t*)+0x0000002B [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DF43EB]
gfxSVGGlyphs::FindOrCreateGlyphsDocument(unsigned int)+0x00000130 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DF4370]
gfxSVGGlyphs::GetGlyphElement(unsigned int)+0x00000050 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DF51A0]
gfxSVGGlyphs::HasSVGGlyph(unsigned int)+0x0000001B [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DF523B]
gfxFontEntry::HasSVGGlyph(unsigned int)+0x00000062 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DACFE2]
gfxFont::RenderSVGGlyph(gfxContext*, gfxPoint, gfxFont::DrawMode, unsigned int, gfxTextObjectPaint*)+0x00000045 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DB4B85]
gfxFont::RenderSVGGlyph(gfxContext*, gfxPoint, gfxFont::DrawMode, unsigned int, gfxTextObjectPaint*, gfxTextRunDrawCallbacks*, bool&)+0x000000A4 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DB4A94]
gfxFont::Draw(gfxTextRun*, unsigned int, unsigned int, gfxContext*, gfxFont::DrawMode, gfxPoint*, gfxFont::Spacing*, gfxTextObjectPaint*, gfxTextRunDrawCallbacks*)+0x000005A9 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DB2DA9]
gfxTextRun::DrawGlyphs(gfxFont*, gfxContext*, gfxFont::DrawMode, gfxPoint*, gfxTextObjectPaint*, unsigned int, unsigned int, gfxTextRun::PropertyProvider*, unsigned int, unsigned int, gfxTextRunDrawCallbacks*)+0x000001B8 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DBD738]
gfxTextRun::Draw(gfxContext*, gfxPoint, gfxFont::DrawMode, unsigned int, unsigned int, gfxTextRun::PropertyProvider*, double*, gfxTextObjectPaint*, gfxTextRunDrawCallbacks*)+0x00000465 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02DBDFE5]
_ZL11DrawTextRunP10gfxTextRunP10gfxContextRK8gfxPointjjP16PropertyProviderjPdP18gfxTextObjectPaintPN11nsTextFrame17DrawPathCallbacksE+0x0000019D [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008FEA8D]
nsTextFrame::DrawTextRun(gfxContext*, gfxPoint const&, unsigned int, unsigned int, PropertyProvider&, unsigned int, double&, bool, gfxTextObjectPaint*, nsTextFrame::DrawPathCallbacks*)+0x0000009D [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008FE6ED]
nsTextFrame::DrawText(gfxContext*, gfxRect const&, gfxPoint const&, gfxPoint const&, unsigned int, unsigned int, PropertyProvider&, nsTextPaintStyle const&, unsigned int, nsCharClipDisplayItem::ClipEdges const&, double&, bool, unsigned int const*, gfxText+0x00000297 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008FBF47]
nsTextFrame::PaintText(nsRenderingContext*, nsPoint, nsRect const&, nsCharClipDisplayItem const&, gfxTextObjectPaint*, nsTextFrame::DrawPathCallbacks*)+0x000007A4 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008F9854]
nsDisplayText::Paint(nsDisplayListBuilder*, nsRenderingContext*)+0x00000171 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x008F9081]
mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*)+0x00000A76 [./obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x006C04B6]
[...etc]

Among other things, a reftest that I'm planning to add to bug 875629 hits this assertion repeatedly.

I guess the fact that we pass NULL for the aLoadGroup parameter to StartDocumentLoad at http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxSVGGlyphs.cpp#354 may be the root of the problem here.

It seems that simply creating a new loadGroup here and passing it to StartDocumentLoad is sufficient to stop the assertions; is that a legitimate thing to do?

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: pass a valid loadGroup to StartDocumentLoad when parsing an SVG glyph

Something like this?

Comment 2

[Boris Zbarsky [:bzbarsky]]

That seems like it silences the assertion without fixing the underlying problem, no?

Is there a concept of load events or readiness for the document ParseDocument is supposed to create?  If so, then we should be using loadgroups properly to manage that state.  If not, we should do something to relax the assertion accordingly.

Comment 3

[Jonathan Kew [:jfkthame]]

I have no idea, really ... I was hoping someone who actually knows about these things would have a look.

Comment 4

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

(In reply to Boris Zbarsky (:bz) from comment #2)
> Is there a concept of load events or readiness for the document
> ParseDocument is supposed to create?  If so, then we should be using
> loadgroups properly to manage that state.  If not, we should do something to
> relax the assertion accordingly.

We already have the complete document, so that's not an issue, and we don't need to fire load events since script is disabled. However I think animation is supposed to start when the load event fires, and that could be delayed momentarily while we load data: and blob: URIs. So I think it makes sense to have a loadgroup. I have no idea what the correct way is to set one up and use it though.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm.  The load event only fires via a document viewer, iirc, and even then probably only if you have a window...  So I am guessing not in this case?

Comment 6

[Boris Zbarsky [:bzbarsky]]

And all the bits that track the load event are docshell-related, for that matter.

If there is no parent document whose onload should be blocked on this stuff (like there is for SVG resource documents, say), we should probably just relax the assertion...

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: don't require a loadGroup if there is no parent document

Can we simply weaken the assertion to ignore the lack of a loadgroup if the loadingDocument has no parent? This avoids the assertion for <image> within SVG glyphs; but are there other cases where it could be a problem?

Comment 8

[Jonathan Kew [:jfkthame]]

Hmm, that should probably be checking GetParentDocument(), not GetParent()...?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 757869 [details] [diff] [review]
don't require a loadGroup if there is no parent document

No, this will fail to catch bogus image loads in, say, a toplevel HTML document in b2g or other e10s setup...

Ideally, we would make the assertion as narrowly targeted at <image> in SVG glyphs as possible.

Oh, and actually...  is the lifetime of that <image> load for SVG glyphs tied to any particular document?  Or is this load shared across documents and thus shouldn't be affected by things like stop buttons, windows being closed, etc?

Comment 10

[Jonathan Kew [:jfkthame]]

It's not clear to me how we can currently tell that the image is being loaded by an SVG glyph (short of adding a special flag to such SVG documents, or something like that). Fortunately, however, it appears that the patch awaiting landing in bug 801467 is helpfully going to resolve this, by creating a new URI scheme "fonttable:" that will be used by such documents.

Hence, we should be able to relax the assertion in a targeted way by using the new IsFontTableURI() function introduced in that bug.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: relax assertion requiring a loadGroup when the loading document is an SVG-glyph document loaded from a font table

This is intended to go on top of the (reviewed but unlanded) patch in bug 801467.

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Boris Zbarsky (:bz) from comment #9)
> Oh, and actually...  is the lifetime of that <image> load for SVG glyphs
> tied to any particular document?  Or is this load shared across documents
> and thus shouldn't be affected by things like stop buttons, windows being
> closed, etc?

The same font resource might be used by multiple documents, so I don't think stop buttons, closed windows, etc., should affect it.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 758173 [details] [diff] [review]
relax assertion requiring a loadGroup when the loading document is an SVG-glyph document loaded from a font table

r=me

Comment 14

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ab17d2d97bf

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6ab17d2d97bf