Closed Bug 721315 Opened 8 years ago Closed 8 years ago
avoid potential dangling m
Family pointers in font entries
1.14 KB, patch
|Details | Diff | Splinter Review|
2.90 KB, patch
|Details | Diff | Splinter Review|
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.
Attachment #591716 - Flags: review?(jdaggett)
Comment on attachment 591716 [details] [diff] [review] patch, clear the faces' mFamily pointers when the family is deleted Looks good.
Attachment #591716 - Flags: review?(jdaggett) → review+
Target Milestone: --- → mozilla12
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...
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)?
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 188.8.131.52-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...
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.
Attachment #592667 - Flags: review?(jdaggett)
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"?
Attachment #592667 - Flags: review?(jdaggett) → review+
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)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: mozilla12 → mozilla13
You need to log in before you can comment on or make changes to this bug.