1186875 - [gtk3] TEST-UNEXPECTED-FAIL | valgrind-test | Invalid read of size 4 at gfxFcPlatformFontList::FindGenericFamily / gfxFcPlatformFontList::FindFamily / gfxFontGroup::FindPlatformFont / gfxFontGroup::FindGenericFonts

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Mike Hommey [:glandium]]

TEST-UNEXPECTED-FAIL | valgrind-test | Invalid read of size 4 at gfxFcPlatformFontList::FindGenericFamily / gfxFcPlatformFontList::FindFamily / gfxFontGroup::FindPlatformFont / gfxFontGroup::FindGenericFonts

==17578== Invalid read of size 4
==17578==    at 0xDBEB435: gfxFcPlatformFontList::FindGenericFamily(nsAString_internal const&, nsIAtom*) (gfxFcPlatformFontList.cpp:1414)
==17578==    by 0xDBEB59A: gfxFcPlatformFontList::FindFamily(nsAString_internal const&, nsIAtom*, bool) (gfxFcPlatformFontList.cpp:1246)
==17578==    by 0xDC188A4: gfxFontGroup::FindPlatformFont(nsAString_internal const&, bool, void*) (gfxTextRun.cpp:1726)
==17578==    by 0xDC1CE18: gfxFontGroup::FindGenericFonts(mozilla::FontFamilyType, nsIAtom*, void*) (gfxTextRun.cpp:1564)
==17578==    by 0xDC1CE93: gfxFontGroup::EnumerateFontList(nsIAtom*, void*) (gfxTextRun.cpp:1674)
==17578==    by 0xDBF5548: CreateFontGroup (gfxPlatformGtk.cpp:246)
==17578==    by 0xDBF5548: gfxPlatformGtk::CreateFontGroup(mozilla::FontFamilyList const&, gfxFontStyle const*, gfxUserFontSet*) (gfxPlatformGtk.cpp:241)
==17578==    by 0xDB4EC81: nsFontMetrics::Init(nsFont const&, nsIAtom*, bool, gfxFont::Orientation, nsDeviceContext*, gfxUserFontSet*, gfxTextPerfMetrics*) (nsFontMetrics.cpp:154)
==17578==    by 0xDB43A06: nsFontCache::GetMetricsFor(nsFont const&, nsIAtom*, bool, gfxFont::Orientation, gfxUserFontSet*, gfxTextPerfMetrics*, nsFontMetrics*&) (nsDeviceContext.cpp:164)
==17578==    by 0xE8403B2: nsLayoutUtils::GetFontMetricsForStyleContext(nsStyleContext*, nsFontMetrics**, float) (nsLayoutUtils.cpp:3957)
==17578==    by 0xE8A7113: ComputeLineHeight (nsHTMLReflowState.cpp:2635)
==17578==    by 0xE8A7113: nsHTMLReflowState::CalcLineHeight(nsIContent*, nsStyleContext*, int, float) (nsHTMLReflowState.cpp:2659)
==17578==    by 0xE865867: nsBlockReflowState::nsBlockReflowState(nsHTMLReflowState const&, nsPresContext*, nsBlockFrame*, bool, bool, bool, int) (nsBlockReflowState.cpp:146)
==17578==    by 0xE87112E: nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsBlockFrame.cpp:1096)
==17578==    by 0xE8861CC: nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, unsigned int&, nsOverflowContinuationTracker*) (nsContainerFrame.cpp:998)
==17578==    by 0xE86AD0D: nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsCanvasFrame.cpp:677)
==17578==    by 0xE8861CC: nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, unsigned int&, nsOverflowContinuationTracker*) (nsContainerFrame.cpp:998)
==17578==    by 0xE89DDA2: nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) (nsGfxScrollFrame.cpp:523)
==17578==    by 0xE89E3F4: nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) (nsGfxScrollFrame.cpp:634)
==17578==    by 0xE8A9ED8: nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsGfxScrollFrame.cpp:868)
==17578==    by 0xE8860ED: nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) (nsContainerFrame.cpp:1040)
==17578==    by 0xE8D2D60: ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsViewportFrame.cpp:218)
==17578==    by 0xE83E0D8: PresShell::DoReflow(nsIFrame*, bool) (nsPresShell.cpp:9040)
==17578==    by 0xE8524C3: PresShell::ProcessReflowCommands(bool) (nsPresShell.cpp:9212)
==17578==    by 0xE852125: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (nsPresShell.cpp:4124)
==17578==    by 0xE7CC67B: nsRefreshDriver::Tick(long, mozilla::TimeStamp) (nsRefreshDriver.cpp:1738)
==17578==    by 0xE7CD1B6: mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) (nsRefreshDriver.cpp:187)
==17578==    by 0xE7CD2C7: RunRefreshDrivers (nsRefreshDriver.cpp:438)
==17578==    by 0xE7CD2C7: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) (nsRefreshDriver.cpp:372)
==17578==    by 0xE7C9DFD: apply<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp)> (nsThreadUtils.h:634)
==17578==    by 0xE7C9DFD: nsRunnableMethodImpl<void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), true, mozilla::TimeStamp>::Run() (nsThreadUtils.h:828)
==17578==    by 0xD61AFFC: ProcessNextEvent (nsThread.cpp:867)
==17578==    by 0xD61AFFC: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:752)
==17578==    by 0xD634BDC: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:277)
==17578==    by 0xD61E640: Shutdown (nsThread.cpp:665)
==17578==    by 0xD61E640: nsThread::Shutdown() (nsThread.cpp:624)
==17578==    by 0xD61619C: apply<nsIThread, nsresult (nsIThread::*)()> (nsThreadUtils.h:621)
==17578==    by 0xD61619C: nsRunnableMethodImpl<nsresult (nsIThread::*)(), true>::Run() (nsThreadUtils.h:828)
==17578==    by 0xD61AFFC: ProcessNextEvent (nsThread.cpp:867)
==17578==    by 0xD61AFFC: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:752)
==17578==    by 0xD634BDC: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:277)
==17578==    by 0xD61E640: Shutdown (nsThread.cpp:665)
==17578==    by 0xD61E640: nsThread::Shutdown() (nsThread.cpp:624)
==17578==    by 0xD61619C: apply<nsIThread, nsresult (nsIThread::*)()> (nsThreadUtils.h:621)
==17578==    by 0xD61619C: nsRunnableMethodImpl<nsresult (nsIThread::*)(), true>::Run() (nsThreadUtils.h:828)
==17578==    by 0xD61AFFC: ProcessNextEvent (nsThread.cpp:867)
==17578==    by 0xD61AFFC: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:752)
==17578==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==17578==

This is a crash I actually got locally when I had no X fonts installed. That happened to me when I tried to reproduce some build or test environment and I missed some packages to be installed. This is obviously not normal conditions, but they seem to be happening on valgrind tests... which, in fact, is kind of dubious.

So I'm left to wonder, is it worth fixing, or should we just change the valgrind build environments to have fonts installed? BTW, I'm surprised PGO builds don't fail the same way, I thought valgrind builds used the same environment as normal builds...

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: check if FcFontSort returns non-null

This particular invalid read is apparently because FcFontSort is returning null, which it is allowed to do. In other places in the code, we check that it's non-null, but apparently not in this one case. The fix is, at least, trivially to just add the check.

Comment 2

[Jonathan Kew [:jfkthame]]

If there are no fonts installed at all, I'm surprised you weren't hitting the NS_RUNTIMEABORT at http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxTextRun.cpp#1904.

Allowing gfxFcPlatformFontList::FindGenericFamily to return NULL sounds like it could easily trip us up later, as we'd normally expect to get something usable from it. ni'ing jdaggett to make sure he sees this... is it appropriate to return NULL here, or is there some other way we should handle the problem?

Comment 3

[Lee Salzman [:lsalzman]]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> If there are no fonts installed at all, I'm surprised you weren't hitting
> the NS_RUNTIMEABORT at
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxTextRun.cpp#1904.
> 
> Allowing gfxFcPlatformFontList::FindGenericFamily to return NULL sounds like
> it could easily trip us up later, as we'd normally expect to get something
> usable from it. ni'ing jdaggett to make sure he sees this... is it
> appropriate to return NULL here, or is there some other way we should handle
> the problem?

That abort I think would otherwise get hit, but only if FindGenericFamily doesn't crash doing the null-dereference here, since FindGenericFamily is actually getting called inside other functions at line 1866 there.

As far as I can see, FindGenericFamily can also legitimately return null even if FcFontSort does not, and any code that calls it looks insulated against the case that it returns null as far as I can see by visual inspection. Worth trying what happens with that null-deref patched, though.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> If there are no fonts installed at all, I'm surprised you weren't hitting
> the NS_RUNTIMEABORT at
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxTextRun.cpp#1904.

So in fact, solving this particular null deref *does* end up on that abort, which is actually better because it actually tells what's wrong:
###!!! ABORT: unable to find a usable font (serif): file /builds/slave/try-l64-valgrind-0000000000000/src/gfx/thebes/gfxTextRun.cpp, line 1904

So, independently of the missing fonts issues, this is worth fixing. Now, I need to figure out why fonts are missing for those, but not for e.g. PGO builds.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #4)
> So, independently of the missing fonts issues, this is worth fixing. Now, I
> need to figure out why fonts are missing for those, but not for e.g. PGO
> builds.

And in fact, it seems PGO builds have problems too, so I filed bug 1187245.

Comment 6

[John Daggett (:jtd)]

Comment on attachment 8637900 [details] [diff] [review]
check if FcFontSort returns non-null

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

r+ but just return nullptr since genericFamily will always be null at that point. Returning null from FindGenericFamily should be fine. But if you're really trying to run on systems without fonts in the fontlist, bad things are going to happen but at least that'll be an abort rather than a hard crash.

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: check if FcFontSort returns non-null

Just return null as requested by John Daggett.

Comment 8

[Lee Salzman [:lsalzman]]

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4adb6805a24

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67ee7f75059e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67ee7f75059e