721315 - avoid potential dangling mFamily pointers in font entries

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: patch, clear the faces' mFamily pointers when the family is deleted

See bug 668813 comment 47 - cached fonts might live longer than their parent families, so we need to clear the mFamily pointers in the gfxFontEntry objects when the family is deleted.

The attached patch clears the pointers when the family goes away; we should also verify that whenever font entries want to look at their mFamily, they check for null.

Comment 1

[John Daggett (:jtd)]

Comment on attachment 591716 [details] [diff] [review]
patch, clear the faces' mFamily pointers when the family is deleted

Looks good.

Comment 2

[Jonathan Kew [:jfkthame]]

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

Comment 3

[Jonathan Kew [:jfkthame]]

Backed out by https://hg.mozilla.org/integration/mozilla-inbound/rev/f35f0858a5b4 because of reftest crash:

TEST-UNEXPECTED-FAIL | http://localhost:4444/1327742103719/198/font-face/download-3-ref.html | Exited with code 1 during test run
PROCESS-CRASH | http://localhost:4444/1327742103719/198/font-face/download-3-ref.html | application crashed (minidump found)
Thread 0 (crashed)

I suspect this may mean we actually have a bug in the gfxUserFontSet code...

Comment 4

[John Daggett (:jtd)]

Do you have a link to the logfile for that test?  Does it occur on only a subset of platforms (e.g. OSX or Linux only)?

Comment 5

[Jonathan Kew [:jfkthame]]

It affects all platforms; here's a Linux debug log:

https://tbpl.mozilla.org/php/getParsedLog.php?id=8901053&tree=Mozilla-Inbound

The crash (in the Linux case) looks like:

PROCESS-CRASH | http://localhost:4444/1327741862626/198/font-face/download-3-ref.html | application crashed (minidump found)
Crash dump filename: /tmp/tmp_JdG-_/minidumps/01dad2ac-1e3d-3394-1438d4d4-112aa96a.dmp
Operating system: Linux
                  0.0.0 Linux 2.6.31.5-127.fc12.i686.PAE #1 SMP Sat Nov 7 21:25:57 EST 2009 i686
CPU: x86
     GenuineIntel family 6 model 23 stepping 10
     2 CPUs

Crash reason:  SIGSEGV
Crash address: 0x10

Thread 0 (crashed)
 0  libxul.so!nsAString_internal::Assign [nsTSubstring.cpp : 372 + 0x0]
    eip = 0x01f31fa6   esp = 0xbfb7f8c0   ebp = 0xbfb7f8d8   ebx = 0x029ed9e8
    esi = 0x0a1baf3c   edi = 0x0000000c   eax = 0x0a1baf3c   ecx = 0x029ed9e8
    edx = 0x0000000c   efl = 0x00010283
    Found by: given as instruction pointer in context
 1  libxul.so!gfxFontEntry::gfxFontEntry [gfxFont.h : 226 + 0xd]
    eip = 0x01f85295   esp = 0xbfb7f8e0   ebp = 0xbfb7f908   ebx = 0x029ed9e8
    esi = 0x0a1baf30   edi = 0x00000000
    Found by: call frame info
 2  libxul.so!gfxFcFontEntry::gfxFcFontEntry [gfxPangoFonts.cpp : 232 + 0xa]
    eip = 0x01f8f820   esp = 0xbfb7f910   ebp = 0xbfb7f928   ebx = 0x029ed9e8
    esi = 0x0a1baf30   edi = 0x0994aee8
    Found by: call frame info
 3  libxul.so!gfxUserFcFontEntry::gfxUserFcFontEntry [gfxPangoFonts.cpp : 439 + 0xc]
    eip = 0x01f8fe3b   esp = 0xbfb7f930   ebp = 0xbfb7f958   ebx = 0x029ed9e8
    esi = 0x0a1baf30   edi = 0x0994aee8
    Found by: call frame info
 4  libxul.so!gfxDownloadedFcFontEntry::gfxDownloadedFcFontEntry [gfxPangoFonts.cpp : 557 + 0x8]
    eip = 0x01f8fe9d   esp = 0xbfb7f960   ebp = 0xbfb7f998   ebx = 0x029ed9e8
    esi = 0x0a1baf30   edi = 0x0b810b88
    Found by: call frame info
 5  libxul.so!gfxPangoFontGroup::NewFontEntry [gfxPangoFonts.cpp : 2359 + 0xf]
    eip = 0x01f91d34   esp = 0xbfb7f9a0   ebp = 0xbfb7f9e8   ebx = 0x029ed9e8
    esi = 0x094e8170   edi = 0x000005d8
    Found by: call frame info
 6  libxul.so!gfxUserFontSet::OnLoadComplete [gfxUserFontSet.cpp : 505 + 0xa]
    eip = 0x01f864b3   esp = 0xbfb7f9f0   ebp = 0xbfb7fbe8   ebx = 0x029ed9e8
    esi = 0x094e8170   edi = 0x000005d8
    Found by: call frame info

Ah - I suspect the problem may occur when we rebuild the user font set due to a stylesheet modification, and migrate existing proxy entries to new families; we're probably not maintaining the family pointer properly at that time. I'll look into it, unless you care to...

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: followup, fix nsUserFontSet::UpdateRules to detach faces from families that are being replaced

The problem arose because when we rebuild the user font set (due to a stylesheet update), we discard the existing family records and create new ones, migrating the existing font entries where possible (so that we don't re-download fonts that were already loaded).

However, it turns out that the old family record may not get deleted until _after_ the font entry is added to the new one (because there may be an in-progress loader that's holding a reference to it); in this case, when the old family dies, it will clear the mFamily pointer in the font entry, wiping out the new mFamily that was set when we re-parented the entry.

The simplest solution, I think, is to ensure that we "detach" the font entries from their family records before rebuilding the user font set, so that even if the old families take some time to completely die off, they won't do any harm.

With this fix, the original patch to avoid dangling mFamily pointers passes tests on tryserver successfully.

Comment 7

[John Daggett (:jtd)]

Comment on attachment 592667 [details] [diff] [review]
followup, fix nsUserFontSet::UpdateRules to detach faces from families that are being replaced

>     // tell our font entries that their family is disowning them
>     // as it is shortly going to be deleted

Simpler comment please, "clear family pointer for all entries"?

Comment 8

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4508f505db3 (nsUserFontSet::UpdateRules followup)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e56264ba8ae (the original dangling-ptr patch)

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b4508f505db3
https://hg.mozilla.org/mozilla-central/rev/5e56264ba8ae