871961 - SVG-in-OpenType reftests are not being run

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Bug 719286 added a collection of tests in a new directory layout/reftests/text-svgglyphs. However, it omitted to add this to the "master" reftest manifest in layout/reftests/reftest.list, and so the tests are not actually being run.

This can be confirmed by loading a full reftest log from tbpl and grepping for the name of the test directory "text-svgglyphs", which should show up in the test paths when they're run - but is completely missing.

Unfortunately, enabling the tests reveals that a bunch of them fail (the exact collection of failures depends on the platform):

  https://tbpl.mozilla.org/?tree=Try&rev=42a43b674b30

Moreover, local testing with a debug build indicates that they also leak pretty badly (total reported is about 380K in my OS X build). I've pushed a debug tryserver job to confirm this:

  https://tbpl.mozilla.org/?tree=Try&rev=87c7011a27d0

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - handle on-the-fly changes to the gfx.font_rendering.opentype_svg.enabled preference setting

Part of the problem here is that the pref for enabling SVG-in-OpenType support is not "live-apply", which means it won't work reliably with the pref(...) annotation in the reftest manifest. (It's likely that bug 816483 made things worse here, as we no longer reload the SVG-enabled font for each test that uses it, but keep re-using the same font entry - but then it doesn't notice if the OpenType-SVG pref has changed.) This patch makes the pref "live", which should resolve this problem. It fixes most (but not quite all) of the test failures for me locally; we'll see what tryserver thinks about it.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - add text-svgglyphs subdirectory to the master layout/reftests manifest

And this adds the text-svgglyphs subdir to the manifest, so the tests will be run. Depending on tryserver results, I expect we'll also need some "fails-if" annotations before we can actually land this, though.

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

...wow. Um. Oops.

Comment 4

[Jonathan Kew [:jfkthame]]

So with the two patches above, here's a summary of tryserver failures on these tests, from

  https://tbpl.mozilla.org/?tree=Try&rev=3e7d8a0510ab (opt)
  https://tbpl.mozilla.org/?tree=Try&rev=25c79eab476b (debug)

Linux:
svg-glyph-mask.svg | image comparison (==), max difference: 54, number of differing pixels: 924

OSX 10.6/10.7/10.8:
svg-glyph-mask.svg | image comparison (==), max difference: 188, number of differing pixels: 5872

WinXP:
svg-glyph-html.html | image comparison (==), max difference: 255, number of differing pixels: 10650
svg-glyph-objectgradient.svg | image comparison (==), max difference: 1, number of differing pixels: 6
svg-glyph-objectopacity.svg | image comparison (==), max difference: 1, number of differing pixels: 12
svg-glyph-paintnone.svg | image comparison (==), max difference: 112, number of differing pixels: 32000
svg-glyph-mask.svg | image comparison (==), max difference: 188, number of differing pixels: 5872 

WinXP/debug:
svg-glyph-html.html | image comparison (==), max difference: 255, number of differing pixels: 10650
svg-glyph-objectgradient.svg | image comparison (==), max difference: 1, number of differing pixels: 6
svg-glyph-objectopacity.svg | image comparison (==), max difference: 1, number of differing pixels: 12
svg-glyph-mask.svg | image comparison (==), max difference: 54, number of differing pixels: 924 

Win7:
svg-glyph-html.html | image comparison (==), max difference: 255, number of differing pixels: 10650
svg-glyph-direct.svg | image comparison (==), max difference: 255, number of differing pixels: 6300
svg-glyph-paintnone.svg | image comparison (==), max difference: 112, number of differing pixels: 32000
svg-glyph-mask.svg | image comparison (==), max difference: 188, number of differing pixels: 5872 

Win7/unacc:
svg-glyph-html.html | image comparison (==), max difference: 255, number of differing pixels: 10650
svg-glyph-objectgradient.svg | image comparison (==), max difference: 1, number of differing pixels: 6
svg-glyph-objectopacity.svg | image comparison (==), max difference: 1, number of differing pixels: 12
svg-glyph-paintnone.svg | image comparison (==), max difference: 112, number of differing pixels: 32000
svg-glyph-mask.svg | image comparison (==), max difference: 188, number of differing pixels: 5872 

Win7/debug:
svg-glyph-html.html | image comparison (==), max difference: 255, number of differing pixels: 10650
svg-glyph-mask.svg | image comparison (==), max difference: 54, number of differing pixels: 924 

Win8:
svg-glyph-html.html | image comparison (==), max difference: 255, number of differing pixels: 10650
svg-glyph-paintnone.svg | image comparison (==), max difference: 102, number of differing pixels: 25600
svg-glyph-mask.svg | image comparison (==), max difference: 188, number of differing pixels: 5872 

Win8/unacc:
svg-glyph-html.html | image comparison (==), max difference: 255, number of differing pixels: 10650
svg-glyph-objectgradient.svg | image comparison (==), max difference: 1, number of differing pixels: 6
svg-glyph-objectopacity.svg | image comparison (==), max difference: 1, number of differing pixels: 12
svg-glyph-paintnone.svg | image comparison (==), max difference: 103, number of differing pixels: 25600
svg-glyph-mask.svg | image comparison (==), max difference: 188, number of differing pixels: 5872 

Win8/debug:
svg-glyph-html.html | image comparison (==), max difference: 255, number of differing pixels: 10650
svg-glyph-mask.svg | image comparison (==), max difference: 188, number of differing pixels: 5872 

Android:
svg-glyph-basic.svg | image comparison (==), max difference: 255, number of differing pixels: 22900
svg-glyph-positioning.svg | image comparison (==), max difference: 255, number of differing pixels: 22900
svg-glyph-objectgradient.svg | image comparison (==), max difference: 4, number of differing pixels: 3
svg-glyph-objectopacity.svg | image comparison (==), max difference: 5, number of differing pixels: 6
svg-glyph-mask.svg | image comparison (==), max difference: 198, number of differing pixels: 5872

B2G:
svg-glyph-basic.svg | image comparison (==), max difference: 255, number of differing pixels: 22900
svg-glyph-positioning.svg | image comparison (==), max difference: 255, number of differing pixels: 22900
svg-glyph-objectgradient.svg | image comparison (==), max difference: 1, number of differing pixels: 6
svg-glyph-objectopacity.svg | image comparison (==), max difference: 1, number of differing pixels: 12
svg-glyph-mask.svg | image comparison (==), max difference: 188, number of differing pixels: 5872

Comment 5

[Jonathan Kew [:jfkthame]]

A few observations:

(1) First the simplest case: svg-glyph-mask.svg fails everywhere (although looking in reftest-analyzer, the nature of the failure varies). Let's just mark it as failing and file a bug on it.

(2) When svg-glyph-objectgradient.svg and svg-glyph-objectopacity.svg fail, it's always a few individual pixels on a color/gradient boundary that differ by a small amount. I propose to just annotate these as fuzzy and move on.

(3) The svg-glyph-html.html failure on Windows looks like it may be related to line-height not behaving as expected. Let's annotate it as fails-if(winWidget) and file a bug to investigate further.

(4) In svg-glyph-basic.svg and svg-glyph-positioning.svg, both Android and B2G seem to get the third glyph badly wrong. Annotate as failing, and file.

(5) That leaves svg-glyph-paintnone.svg, which seems to fail on some Windows runs and not others, but it's not clear to me what's causing the difference (a combination of Windows version, accel or not, opt or debug). Annotate as random on windows and file a bug.

Comment 6

[Jonathan Kew [:jfkthame]]

Finally, there's the question of leaks. (See Ubuntu64 in https://tbpl.mozilla.org/?tree=Try&rev=25c79eab476b.) On that job, only the Ubuntu64 run reported a leak. I get a similar leak locally on OS X if I run ./mach reftest --filter text-svgglyphs, but that doesn't show up on the tryserver job.

I suspect that there's a timing-related issue here: if the run ends soon enough after running these testcases, we get a large leak reported; but if the run continues long enough, it goes away. So Ubuntu64 is getting "unlucky" on tryserver, but the leak could potentially appear on any platform.

To confirm this, I pushed a tryserver job that runs -only- the text-svgglyphs reftests, by stripping the other subdirs out of the master reftest.list manifest. Sure enough, in this case all platforms reported leaks of several hundred KB:
  https://tbpl.mozilla.org/?tree=Try&rev=d5c5af1a60b6

And in contrast, I also tried moving text-svgglyphs to the beginning of the main reftest.list manifest, so that there are a lot more testcases run -after- these. In this case, no leaks are reported:
  https://tbpl.mozilla.org/?tree=Try&rev=d4e5bbb7052c

Comment 7

[Jonathan Kew [:jfkthame]]

So we could work around the "leak" issue for now by putting the svgglyphs tests early in the full reftest job, and argue that since the "leaked" objects are apparently getting cleaned up eventually (GC gets around to it?), they don't really matter. But the reported leak - which will turn the reftest job orange - would be liable to resurface for anyone who runs selected subsets of the full reftest suite, or if we start "chunking" reftests into smaller groups for parallelization.

Alternatively, we'll need to figure out what's actually causing the leak report, and ensure things are cleaned up promptly when the testcases finish instead of leaving them lying around for some indeterminate time. (Maybe this is related to the deferred "expiration" of cached font objects?)

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - annotate failing svgglyphs reftests

This annotates the svgglyphs tests that fail on various platforms, as per comment #5 above. (Also svg-glyph-direct.svg, which fails intermittently on Windows, like svg-glyph-paintnone.svg -- I missed that one in comment #5, but it shows on the tryserver runs.) I've filed bugs for the various tests that are failing, and set them as blocking 719286 so we can keep track. We'll need to fold this together with the preceding patch in order to actually land.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - move the svgglyphs reftests early in the full reftest job, to avoid leak reports at end of run

And finally a patch to run the svgglyphs tests earlier during reftest, to avoid the leak-at-shutdown issue, as discussed above. This is just a wallpaper patch, but should be sufficient to let us start running the tests; we should investigate the "temporary leak" condition properly in a followup, and figure out whether it's benign or something that really needs to be fixed. Again, this will need to be folded with the preceding patches, to avoid turning tbpl orange.

Comment 10

[Jonathan Kew [:jfkthame]]

Try run with these patches, to confirm the reftests actually end up green:
https://tbpl.mozilla.org/?tree=Try&rev=c699afe02c56

Comment 11

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

(In reply to Jonathan Kew (:jfkthame) from comment #9)
> And finally a patch to run the svgglyphs tests earlier during reftest, to
> avoid the leak-at-shutdown issue, as discussed above. This is just a
> wallpaper patch, but should be sufficient to let us start running the tests;
> we should investigate the "temporary leak" condition properly in a followup,
> and figure out whether it's benign or something that really needs to be
> fixed. Again, this will need to be folded with the preceding patches, to
> avoid turning tbpl orange.

This sounds a bit risky. I think we should actually fix the leak.

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> (In reply to Jonathan Kew (:jfkthame) from comment #9)
> > And finally a patch to run the svgglyphs tests earlier during reftest, to
> > avoid the leak-at-shutdown issue, as discussed above. This is just a
> > wallpaper patch, but should be sufficient to let us start running the tests;
> > we should investigate the "temporary leak" condition properly in a followup,
> > and figure out whether it's benign or something that really needs to be
> > fixed. Again, this will need to be folded with the preceding patches, to
> > avoid turning tbpl orange.
> 
> This sounds a bit risky. I think we should actually fix the leak.

Well, yes, I agree. But I'm not sure I'll have time to figure that out right away; hence the suggested wallpaper.

If we don't do that, we can't actually enable the tests at all for the time being, until the "leak" is resolved. As things stand, we could thoroughly break the feature and our testing would miss it entirely.

Comment 13

[Jonathan Kew [:jfkthame]]

FWIW, I've confirmed that when I run a single testcase, e.g.

  ./mach reftest --filter svg-glyph-html

the specific SVGDocument that was created by gfxSVGGlyphsDocument::ParseDocument and stored in its mDocument field is the one reported as being "leaked" (along with loads of other objects, which I expect are ultimately hanging off it).

At the end of the test run, the gfxSVGGlyphs object gets deleted, and in turn the gfxSVGGlyphsDocument that it had created is also deleted; but the SVGDocument is not, although gfxSVGGlyphsDocument clearly releases -its- reference (nsCOMPtr) to it.

Note that this SVGDocument is not "truly" leaked, in that if the browser continues to run for some time, it eventually goes away (presumably after various caches expire and garbage-collection runs, or something). That's why no leak gets reported if the svgglyphs tests run well before the end of the entire reftest job.

At this point, it's not obvious to me how to ensure the SVGDocument etc will be fully released before the shutdown leak-check, given that it seems to be happening asynchronously some time after the testcases using it have finished.

Comment 14

[Jonathan Kew [:jfkthame]]

Pushed part 1 (making the pref "live") to inbound, mainly because the patch in bug 847344 was written on top of this. The tests have -not- yet been enabled, due to the "leak" issue, so this is not resolved.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba23d27b9cec

[leave open when merging to m-c]

Comment 15

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

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> If we don't do that, we can't actually enable the tests at all for the time
> being, until the "leak" is resolved. As things stand, we could thoroughly
> break the feature and our testing would miss it entirely.

True, but I think that just means we should prioritize fixing the leak :-),

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba23d27b9cec

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> (In reply to Jonathan Kew (:jfkthame) from comment #12)
> > If we don't do that, we can't actually enable the tests at all for the time
> > being, until the "leak" is resolved. As things stand, we could thoroughly
> > break the feature and our testing would miss it entirely.
> 
> True, but I think that just means we should prioritize fixing the leak :-),

I don't have any good ideas at this point for how to tackle that; see comment 13.

From the observed behavior, I suspect that if we cleared all caches (fonts, bfcache, etc) and forced GC to run after these testcases are finished, the "leak" would probably disappear - but (a) I'm not sure how to go about that, and (b) it sounds like a really ugly sledgehammer approach!

Alternatively, we could view this as a problem in shutdown behavior - we're shutting down the browser (and checking for leaked objects) without having correctly flushed everything first. But the only thing this affects is leak-reporting in debug builds; in a release build, we shouldn't care about explicitly releasing everything anyhow, we just want the whole process to go away. And we know (see comment 6) that we are not really "leaking" these objects in a permanent sense, as letting the browser run a bunch more tests before shutting down makes them disappear.

Unless you can suggest a clear way forward, I doubt I'll be "fixing" this anytime soon. :(

Comment 18

[Jonathan Kew [:jfkthame]]

FWIW, here's the call stack for deleting the SVGDocument that was created to parse SVG glyphs, in the case where we run the browser long enough that it goes away and no leak is reported.

AFAICS, this implies that it's being deleted by cycle-collection, as it looks like ContentUnbinder is something the cycle collector uses.

For "normal" SVGDocument instances (i.e. when we've simply loaded a .svg file in its own right), the call stack when it is deleted is exactly the same; those are also cleaned up via ContentUnbinder, etc.

mozilla::dom::SVGDocument::~SVGDocument()+0x00000169 [XUL +0x019841B9]
mozilla::dom::SVGDocument::~SVGDocument()+0x00000015 [XUL +0x01984045]
mozilla::dom::SVGDocument::~SVGDocument()+0x00000019 [XUL +0x01984019]
nsNodeUtils::LastRelease(nsINode*)+0x0000040E [XUL +0x00C92FEE]
nsDocument::Release()+0x00000231 [XUL +0x00BD9471]
mozilla::dom::XMLDocument::Release()+0x0000001F [XUL +0x011BA02F]
mozilla::dom::SVGDocument::Release()+0x0000001F [XUL +0x0198492F]
nsNodeInfoManager::RemoveNodeInfo(nsNodeInfo*)+0x000000B8 [XUL +0x00C8F818]
nsNodeInfo::~nsNodeInfo()+0x00000034 [XUL +0x00C8CAE4]
nsNodeInfo::~nsNodeInfo()+0x00000015 [XUL +0x00C8CAA5]
nsNodeInfo::~nsNodeInfo()+0x00000019 [XUL +0x00C8CA79]
nsNodeInfo::LastRelease()+0x00000041 [XUL +0x00C8D8A1]
nsNodeInfo::Release()+0x000001F6 [XUL +0x00C8D836]
nsCOMPtr<nsINodeInfo>::~nsCOMPtr()+0x0000005B [XUL +0x006A137B]
nsCOMPtr<nsINodeInfo>::~nsCOMPtr()+0x00000015 [XUL +0x006A1315]
nsINode::~nsINode()+0x000000B2 [XUL +0x00C6D5A2]
nsIContent::~nsIContent()+0x00000015 [XUL +0x00C64C15]
mozilla::dom::FragmentOrElement::~FragmentOrElement()+0x000000C9 [XUL +0x00D10B29]
mozilla::dom::Element::~Element()+0x00000015 [XUL +0x00C57A65]
nsStyledElementNotElementCSSInlineStyle::~nsStyledElementNotElementCSSInlineStyle()+0x00000015 [XUL +0x00C88115]
nsSVGElement::~nsSVGElement()+0x00000081 [XUL +0x019CE1A1]
mozilla::dom::SVGTransformableElement::~SVGTransformableElement()+0x0000006F [XUL +0x019EA57F]
mozilla::dom::SVGGraphicsElement::~SVGGraphicsElement()+0x00000031 [XUL +0x019EA491]
mozilla::dom::SVGSVGElement::~SVGSVGElement()+0x00000092 [XUL +0x01A57782]
mozilla::dom::SVGSVGElement::~SVGSVGElement()+0x00000015 [XUL +0x01A57295]
mozilla::dom::SVGSVGElement::~SVGSVGElement()+0x00000019 [XUL +0x01A572B9]
nsNodeUtils::LastRelease(nsINode*)+0x0000040E [XUL +0x00C92FEE]
mozilla::dom::FragmentOrElement::Release()+0x000001F9 [XUL +0x00D144C9]
nsSVGElement::Release()+0x0000001F [XUL +0x019C247F]
mozilla::dom::SVGGraphicsElement::Release()+0x0000001F [XUL +0x01A257EF]
mozilla::dom::SVGSVGElement::Release()+0x0000001F [XUL +0x01A5329F]
nsCOMPtr<nsIContent>::~nsCOMPtr()+0x0000005B [XUL +0x0011B0FB]
nsCOMPtr<nsIContent>::~nsCOMPtr()+0x00000015 [XUL +0x0011B095]
nsTArrayElementTraits<nsCOMPtr<nsIContent> >::Destruct(nsCOMPtr<nsIContent>*)+0x00000015 [XUL +0x0011E1B5]
nsTArray_Impl<nsCOMPtr<nsIContent>, nsTArrayInfallibleAllocator>::DestructRange(unsigned int, unsigned int)+0x00000052 [XUL +0x0011E152]
nsTArray_Impl<nsCOMPtr<nsIContent>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned int, unsigned int)+0x00000165 [XUL +0x0011E0D5]
nsTArray_Impl<nsCOMPtr<nsIContent>, nsTArrayInfallibleAllocator>::Clear()+0x0000002F [XUL +0x0011DEAF]
ContentUnbinder::Run()+0x000000C5 [XUL +0x00D19755]
nsThread::ProcessNextEvent(bool, bool*)+0x00000644 [XUL +0x02BB5874]
NS_ProcessPendingEvents(nsIThread*, unsigned int)+0x000000A2 [XUL +0x02B17EA2]
nsBaseAppShell::NativeEventCallback()+0x000000BF [XUL +0x0217EB7F]
nsAppShell::ProcessGeckoEvents(void*)+0x000001AC [XUL +0x0210AB7C]
__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__+0x00000011 [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x000124F1]
__CFRunLoopDoSources0+0x000000FD [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x00011D5D]
__CFRunLoopRun+0x00000389 [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x00038B49]
CFRunLoopRunSpecific+0x000000E6 [/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x00038486]
RunCurrentEventLoopInMode+0x00000115 [/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x000022BF]
ReceiveNextEventCommon+0x00000163 [/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x0000956D]
BlockUntilNextEventMatchingListInMode+0x0000003E [/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x000093FA]
_DPSNextEvent+0x00000293 [/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x00008779]
-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]+0x00000087 [/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x0000807D]
-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]+0x00000077 [XUL +0x021093C7]
-[NSApplication run]+0x000001D6 [/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x000049B9]
nsAppShell::Run()+0x0000008C [XUL +0x0210B5FC]
nsAppStartup::Run()+0x00000097 [XUL +0x01D5FBF7]
XREMain::XRE_mainRun()+0x00001588 [XUL +0x00013B18]
XREMain::XRE_main(int, char**, nsXREAppData const*)+0x0000029E [XUL +0x000142BE]
XRE_main+0x0000003F [XUL +0x0001470F]
_ZL7do_mainiPPcP7nsIFile+0x00000662 [firefox-bin +0x000023F2]
main+0x0000013C [firefox-bin +0x0000193C]

Comment 19

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

We should do a full round of cycle collection before shutting down. In debug builds at least.

Maybe the problem is that gfxFontCache::Shutdown is called too late. Can we move it to nsLayoutStatics::Shutdown? Or if that's problematic, we could add gfxFontCache::DisconnectSVGDocuments method that gets called early in nsLayoutStatics::Shutdown. This seems like the right thing to do anyway, having documents still alive after nsLayoutStatics::Shutdown is bad no matter what.

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - on quit-application notification, release any SVG-glyphs documents held by fonts

nsLayoutStatics::Shutdown is too late - it's called from within cycle-collector shutdown. But it seems to work OK if I disconnect any SVGDocuments from the fonts on receiving the quit-application notification, which happens safely before we get into the CC endgame.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not sure quit-application is the right thing here; I think it's a notification specific to certain XUL applications rather than a general XPCOM thing -- though I might be wrong.

I'd be more inclined to suggest using one of the notifications in https://wiki.mozilla.org/XPCOM_Shutdown , though it's worth checking that the notification in question exists since the document there might not be fully accurate.  (If "xpcom-shutdown" works, it seems like the default choice that would make sense.)

Comment 22

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

Comment on attachment 752536 [details] [diff] [review]
pt 4 - on quit-application notification, release any SVG-glyphs documents held by fonts

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

r+ with that!

::: gfx/thebes/gfxFont.cpp
@@ +1292,5 @@
>          if (fontCache) {
>              fontCache->FlushShapedWordCaches();
>          }
>      }
> +    if (!nsCRT::strcmp(aTopic, "quit-application")) {

Let's make this xpcom-shutdown please, assuming that works.

Comment 23

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - on xpcom-shutdown notification, release any SVG-glyphs documents held by fonts.

Yes, xpcom-shutdown seems to work fine; updated patch accordingly, carrying forward r=roc.

Comment 24

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - on xpcom-shutdown notification, release any SVG-glyphs documents held by fonts.

Updated the patch to disconnect SVGDocuments from font entries in the UserFontCache instead of the gfxFontCache. This version also works for me locally on OS X; pushed to tryserver to check whether it's happy there. https://tbpl.mozilla.org/?tree=Try&rev=83886d37a04d

Comment 25

[Jonathan Kew [:jfkthame]]

Comment on attachment 752625 [details] [diff] [review]
pt 4 - on xpcom-shutdown notification, release any SVG-glyphs documents held by fonts.

Finally, a patch that passes on try. :) Re-requesting review for the updated version; it's basically the same as before, except this time it's the gfxUserFontSet::UserFontCache observer that listens for xpcom-shutdown and disconnects any SVG documents from the fonts it knows about.

Comment 26

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8f3694dd6df
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b7bfd871a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/b77d5e403c3d

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d8f3694dd6df
https://hg.mozilla.org/mozilla-central/rev/e8b7bfd871a9
https://hg.mozilla.org/mozilla-central/rev/b77d5e403c3d

Comment 28

[Jonathan Kew [:jfkthame]]

Backed out part 4, because it appears to be causing crashes at shutdown for some people; see bug 875878.

Without part 4, we'd be liable to get debug-reftest orange (at least on Linux) according to my tryserver runs; accordingly, I also backed out part 2 to remove the reftests from the manifest. (Sigh.)

Backout (of both parts 2 & 4):
https://hg.mozilla.org/integration/mozilla-inbound/rev/173700b1c3e9

Comment 29

[Scoobidiver (away)]

Backout on m-c:
http://hg.mozilla.org/mozilla-central/rev/173700b1c3e9

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Can this be relanded now that the dependencies are fixed?

Comment 31

[Jonathan Kew [:jfkthame]]

We could try, perhaps, but I suspect bug 875878 would reappear. AFAIK, we haven't properly understood what was happening there yet, so whatever was causing it probably hasn't been fixed.

Comment 32

[Jonathan Kew [:jfkthame]]

I believe the crash in bug 875878 was due to gfxFontEntry::mSVGGlyphs being left uninitialized; as a result, at shutdown, gfxFontEntry::DisconnectSVG() could find itself trying to delete some random pointer value.

In bug 906643, mSVGGlyphs was changed from a raw pointer to an nsAutoPtr, so there's no longer any risk of it being left uninitialized. So we can re-land the backed-out patches (pt 2 and 4) here, after rebasing to current tip, with the expectation that the crash should no longer occur.

Comment 33

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - on xpcom-shutdown notification, release any SVG-glyphs documents held by fonts.

Part 4, updated for re-landing; carrying forward r=roc.

Comment 34

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6256823ad6 (pt.2)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b8f9312e064 (pt.4)

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd6256823ad6
https://hg.mozilla.org/mozilla-central/rev/2b8f9312e064