SVG-in-OpenType reftests are not being run

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Assignee

Description

6 years ago
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
Assignee

Comment 1

6 years ago
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.
Attachment #749399 - Flags: review?(roc)
Assignee

Updated

6 years ago
Assignee: nobody → jfkthame
Assignee

Comment 2

6 years ago
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.
Attachment #749401 - Flags: review?(roc)
Assignee

Comment 4

6 years ago
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
Assignee

Comment 5

6 years ago
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.
Assignee

Comment 6

6 years ago
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
Assignee

Comment 7

6 years ago
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?)
Assignee

Comment 8

6 years ago
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.
Attachment #749903 - Flags: review?(roc)
Assignee

Comment 9

6 years ago
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.
Attachment #749906 - Flags: review?(roc)
Assignee

Comment 10

6 years ago
Try run with these patches, to confirm the reftests actually end up green:
https://tbpl.mozilla.org/?tree=Try&rev=c699afe02c56
(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.
Assignee

Comment 12

6 years ago
(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.
Assignee

Comment 13

6 years ago
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.
Assignee

Comment 14

6 years ago
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]
Whiteboard: [leave open]
(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 :-),
Assignee

Comment 17

6 years ago
(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. :(
Assignee

Comment 18

6 years ago
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]
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.
Assignee

Comment 20

6 years ago
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.
Attachment #752536 - Flags: review?(roc)
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 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.
Attachment #752536 - Flags: review?(roc) → review+
Assignee

Updated

6 years ago
Attachment #749906 - Attachment is obsolete: true
Attachment #749906 - Flags: review?(roc)
Assignee

Comment 23

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

Updated

6 years ago
Attachment #752536 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #752552 - Flags: review+
Assignee

Comment 24

6 years ago
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
Assignee

Updated

6 years ago
Attachment #752552 - Attachment is obsolete: true
Assignee

Comment 25

6 years ago
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.
Attachment #752625 - Flags: review?(roc)

Updated

6 years ago
Depends on: 875878
Assignee

Comment 28

6 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Updated

6 years ago
Depends on: 801467, 878786
Can this be relanded now that the dependencies are fixed?
Flags: needinfo?(jfkthame)
Assignee

Comment 31

6 years ago
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.
Flags: needinfo?(jfkthame)
Assignee

Comment 32

6 years ago
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.
Assignee

Comment 33

6 years ago
Part 4, updated for re-landing; carrying forward r=roc.
Assignee

Updated

6 years ago
Attachment #752625 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/bd6256823ad6
https://hg.mozilla.org/mozilla-central/rev/2b8f9312e064
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla24 → mozilla26

Updated

4 years ago
Blocks: 1135329

Updated

4 years ago
No longer blocks: 1135329
Depends on: 1135329
You need to log in before you can comment on or make changes to this bug.