1. load http://higher.com.ua/ in minefield on trunk 2. crash bp-e6d04912-d2c8-44b9-be41-654f32091207 0 XUL factory_HashKey xpcom/components/nsComponentManager.cpp:242 1 XUL PL_DHashTableOperate pldhash.c:615 2 XUL nsComponentManagerImpl::FindFactory xpcom/components/nsComponentManager.cpp:1360 3 XUL nsComponentManagerImpl::GetClassObject xpcom/components/nsComponentManager.cpp:1436 4 XUL nsThebesFontMetrics::GetExternalLeading gfx/src/thebes/nsThebesFontMetrics.cpp:117 5 XUL ComputeLineHeight layout/generic/nsHTMLReflowState.cpp:2078 6 XUL XUL@0x273ae8 7 XUL nsBlockFrame::Reflow layout/generic/nsBlockFrame.cpp:946 8 XUL nsContainerFrame::ReflowChild layout/generic/nsContainerFrame.cpp:774 9 XUL nsCanvasFrame::Reflow layout/generic/nsCanvasFrame.cpp:550 10 XUL nsContainerFrame::ReflowChild layout/generic/nsContainerFrame.cpp:774 11 XUL nsHTMLScrollFrame::ReflowScrolledFrame layout/generic/nsGfxScrollFrame.cpp:545 12 XUL nsHTMLScrollFrame::ReflowContents layout/generic/nsGfxScrollFrame.cpp:639 13 XUL nsHTMLScrollFrame::Reflow layout/generic/nsGfxScrollFrame.cpp:840 14 XUL nsContainerFrame::ReflowChild layout/generic/nsContainerFrame.cpp:774 15 XUL ViewportFrame::Reflow layout/generic/nsViewportFrame.cpp:285 16 XUL PresShell::DoReflow layout/base/nsPresShell.cpp:7363 17 XUL PresShell::ProcessReflowCommands layout/base/nsPresShell.cpp:7493 18 XUL PresShell::FlushPendingNotifications layout/base/nsPresShell.cpp:4930 19 XUL PresShell::ReflowEvent::Run layout/base/nsPresShell.cpp:7173

The stack trace is missing a bunch of frames, but I'd guess this is an issue in the font code, especially given that this site uses downloadable fonts, in font formats that we support (woff and ttf). A good stack trace might help pin it down a little better. This crash actually seems to be one of the more common ones in Mac trunk builds.

Yes, it's font-related. With a debug build, I get a couple of assertions: ###!!! ASSERTION: Requesting a font index that doesn't exist: 'mFonts.Length() > PRUint32(i)', file ../../../dist/include/gfxFont.h, line 1732 ###!!! ASSERTION: invalid array index: 'i < Length()', file ../../../dist/include/nsTArray.h, line 317 followed by the crash.

The problem occurs on a page that is using downloadable fonts. For a page that contains downloadable fonts, after a download occurs text runs are rebuilt. However, the font lists are currently not build the same way they were initially. The constructor for gfxFontGroup has code that catches the case where no font is found for the fonts in the font list and adds a default font. The code in UpdateFontList does not have this. The font list that fails is 'font-family: serif;' when the lang is x-cyrillic, there no longer appears to be a 'Times CY' font under Mac OS X, so the lookup fails and without a default font the font list will be empty. The fix is to put the code that's in the gfxFontGroup constructor into a helper method and call it from both the constructor and UpdateFontList.

Pull the font list init code out from the constructor to a helper method and call that method when rebuilding the list from UpdateFontList.

Comment on attachment 426796 [details] [diff] [review] patch, v.0.1, always assure at least one font in list of font group fonts >+ // Initialize the list of fonts >+ void InitFonts(); Personally, I'd prefer a more descriptive name such as "BuildFontList" or even "FindFontsForPlatform" here, if you'd be OK with that. >+void >+gfxFontGroup::InitFonts() >+{ > // "#if" to be removed once all platforms are moved to gfxPlatformFontList interface > // and subclasses of gfxFontGroup eliminated > #if defined(XP_MACOSX) || defined(XP_WIN) > ForEachFont(FindPlatformFont, this); I think that moving ForEachFont here inside the #if will (in principle, at least) break UpdateFontList on platforms that are not XP_MACOSX or XP_WIN, and do not have their own override of UpdateFontList in their gfxFontGroup subclass. It looks like this applies to OS/2, as gfxOS2FontGroup does not override; it would also apply to gfxFT2FontGroup if the FT2 classes are used on a non-XP_WIN platform; I'm not sure if this actually happens. gfxPangoFontGroup reimplements UpdateFontList, so it should be OK. I'd suggest at least leaving the ForEachFont call outside the platform #if, to maintain existing behavior for such platforms. In theory I guess OS/2 is still slightly broken in that case, as it doesn't include code to add a default font if the list is empty after UpdateFontList (as it does in the gfxOS2FontGroup constructor), but at least we wouldn't be regressing it further. > if (!mStyle.systemFont) { > for (PRUint32 i = 0; i < mFonts.Length(); ++i) { ...etc. While we're here, I suggest also moving this outside the #if, as it is platform-independent code and I don't think it's dependent on the use of a gfxPlatformFontList. I don't know if we actually have any "bad underline" blacklists on other platforms, but in principle we could.

No problem with the name change but looking over the code in gfxFT2FontGroup, it looks like the original reworking wasn't complete, fonts would be added to the list within the gfxFontGroup constructor and again in the constructor for gfxFT2FontGroup. I think fixing that needs more than just moving code around within the #if section. Since this is a common crasher, I think it would be better to land a fix for the crash and fix up the FT2 and OS/2 code in a follow-on bug.

Ok, we can deal with those better in a followup. I'd guess (without checking carefully) that the FT2 situation is merely inefficient rather than positively broken, if it's adding fonts redundantly. In that case, though, how about making the change in gfxFontGroup::UpdateFontList() also be dependent on #if defined(XP_WIN) || defined(XP_MACOSX), and leave the existing ForEachFont call in place on the other platforms for now. This would be a temporary hack, but that way we won't actually regress them here; they'll stay in their existing (slightly-broken) state until we do that followup.

Same crash happens on our brand new http://opentochoice.org website for the Bulgarian version. Minefield on OS X crashes immediately while loading. No other platforms and branches are affected.

are we still looking for regression-window wanted? I see low volume crash reports of this going pretty far back http://crash-stats.mozilla.com/report/index/9dbbae28-9e81-4469-ae4c-19ad42090914 Version 3.5.2 Build ID 20090729211433 and probably even earlier.

(In reply to comment #9) > are we still looking for regression-window wanted? I don't think so - we know what's causing this, we just need to get a fix tidied up and landed. This is a regression from bug 502906. > I see low volume crash reports of this going pretty far back > http://crash-stats.mozilla.com/report/index/9dbbae28-9e81-4469-ae4c-19ad42090914 > > Version 3.5.2 > Build ID 20090729211433 > > and probably even earlier. That's not the same; although at the lowest level, it's also a hashtable-related crash, it's coming from an unrelated place. This particular issue is not present on 1.9.1 or 1.9.2 branches.

(In reply to comment #10) > I don't think so - we know what's causing this, we just need to get a fix > tidied up and landed. This is a regression from bug 502906. Please insert the causing bug into the blocking field if you know which patch has been caused a regression. That way everyone who is CC'ed on the causing crash will be notified. Thanks Jonathan for the bug #.

Like this? With 'xxx' replaced by the follow-on bug number, I'll log that this morning.

Comment on attachment 428516 [details] [diff] [review] patch, v.0.2, updated based on review comments Yes, let's do this for now. One nit - there are spaces on the blank line here: >+ >+ // bug xxx - need to clean up FT2, OS/2 platform code to use BuildFontList

Reduced testcase.

Pushed to trunk with reftest http://hg.mozilla.org/mozilla-central/rev/ddfecbc93934