533251 - Crash [@ factory_HashKey]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bob Clary [:bc] (inactive)]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 2

[Jonathan Kew [:jfkthame]]

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.

Comment 3

[John Daggett (:jtd)]

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.

Comment 4

[John Daggett (:jtd)]

Attachment: patch, v.0.1, always assure at least one font in list of font group fonts

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 5

[Jonathan Kew [:jfkthame]]

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.

Comment 6

[John Daggett (:jtd)]

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.

Comment 7

[Jonathan Kew [:jfkthame]]

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.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 9

[chris hofmann]

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.

Comment 10

[Jonathan Kew [:jfkthame]]

(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.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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 #.

Comment 12

[John Daggett (:jtd)]

Attachment: patch, v.0.2, updated based on review comments

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

Comment 13

[Jonathan Kew [:jfkthame]]

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

Comment 14

[John Daggett (:jtd)]

Attachment: reftest patch

Reduced testcase.

Comment 15

[John Daggett (:jtd)]

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