462593 - Crash [out of memory] when rapidly opening multiple frames with downloadable fonts

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

Might be related to bug 460037.

Steps to reproduce:
Visit url, let it open for at least 30 seconds or so, then close the tab of that page.

Result: crash

http://crash-stats.mozilla.com/report/index/1e7a3da1-a799-11dd-a97d-001a4bd43e5c?p=1
0  	xul.dll  	gfxMixedFontFamily::RemoveFontEntry  	 obj-firefox/dist/include/thebes/gfxUserFontSet.h:113
1 	xul.dll 	gfxUserFontSet::LoadNext 	gfx/thebes/src/gfxUserFontSet.cpp:371
2 	xul.dll 	gfxUserFontSet::OnLoadComplete 	gfx/thebes/src/gfxUserFontSet.cpp:270
3 	xul.dll 	nsFontFaceLoader::OnStreamComplete 	layout/style/nsFontFaceLoader.cpp:142

Comment 2

[John Daggett (:jtd)]

Reopening.  With 10 tabs open, the original crash in user font set code doesn't occur but an out-of-memory condition occurs. Need to confirm the underlying cause of this.

Comment 3

[John Daggett (:jtd)]

Attachment: stack crawl, crash after running testcase in 10 tabs for a few minutes

Comment 4

[Martijn Wargers (dead)]

Attachment: testcase

I see this crash occurring a lot with this zipped up testcase.

Comment 5

[John Daggett (:jtd)]

Stack crawl snippet:

msvcr80d.dll!_onexit_nolock
msvcr80d.dll!_CxxThrowException
msvcr80d.dll!operator new
gklayout.dll!nsIFrame::GetOverflowAreaProperty
gklayout.dll!nsIFrame::FinishAndStoreOverflow
gklayout.dll!nsIFrame::FinishAndStoreOverflow
gklayout.dll!nsBlockFrame::Reflow
gklayout.dll!nsContainerFrame::ReflowChild
gklayout.dll!CanvasFrame::Reflow
gklayout.dll!nsContainerFrame::ReflowChild
gklayout.dll!nsHTMLScrollFrame::ReflowScrolledFrame
gklayout.dll!nsHTMLScrollFrame::ReflowContents
gklayout.dll!nsHTMLScrollFrame::Reflow

Comment 6

[Boris Zbarsky [:bzbarsky]]

That first stack has some weird recursion going on....

What happens if you quit the browser before hitting OOM?  Are there leaks in this case?

Comment 7

[timeless]

that's probably not recursion. mw, when you see nsThread::Shutdown in stacks, can you attach the stacks for the other threads too? (also for bonus points, can you check the nsThread objects and try to pair nsThreads to system threads)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The URL for the zipped-up testcase is jar:https://bugzilla.mozilla.org/attachment.cgi?id=353027!/crash1.htm

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

With Martin's stack, I get this

#6  0x0309b509 in gfxQuartzFontCache::MakePlatformFont (this=0x48b98d0, aProxyEntry=0xebae3c0, aFontData=0x7200008 "", aLength=58736) at /Users/roc/mozilla-trunk/xpcom/string/src/nsReadableUtils.cpp:812
1322	                NS_ConvertUTF16toUTF8(proxyEntry->mFamily->Name()).get());

(gdb) p proxyEntry->mFamily[0]
warning: can't find linker symbol for virtual table for `gfxMixedFontFamily' value
$9 = {
  <gfxFontFamily> = {
    _vptr$gfxFontFamily = 0xda264e0, 
    mRefCnt = {
      mValue = 23726592
    }, 
    mName = {
      <nsAString_internal> = {
        mData = 0xeb63470, 
        mLength = 246821328, 
        mFlags = 6512941
      }, <No data fields>}
  },

proxyEntry looks OK but proxyEntry->mFamily is obviously garbage.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Interestingly this is after we failed to validate the font:

WARNING: invalid font (bad checksum): file /Users/roc/mozilla-trunk/gfx/thebes/src/gfxFontUtils.cpp, line 768
WARNING: downloaded font error, invalid font data for (Bitstream Vera Serif Bold): file /Users/roc/mozilla-trunk/gfx/thebes/src/gfxQuartzFontCache.mm, line 1323
WARNING: invalid font (bad checksum): file /Users/roc/mozilla-trunk/gfx/thebes/src/gfxFontUtils.cpp, line 768

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, here's what happens.

We see the first @font-face rule and kick off a load of "Bitstream Vera Serif Bold" with a certain gfxProxyEntry and gfxMixedFontFamily.
Then something happens (seeing the second @font-face rule?) and we do FlushUserFontSet.
We reprocess the first @font-face rule and kick off another load of "Bitstream Vera Serif Bold" with different gfxProxyEntry and gfxMixedFontFamily objects.
We get an nsFontFaceLoader::OnStreamComplete for the first load. The font's invalid, so all srcs for that face failed, so all faces for that family object failed, so gfxUserFontSet::LoadNext does "RemoveFamily(family->Name());". Unfortunately this deletes the second gfxMixedFontFamily object since we're in a new gfxUserFontSet now! So the second gfxProxyEntry has a dangling pointer.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 1: nsPtrHashKey

For the real patch, I need nsPtrHashKey. So I'm adding it to nsHashKeys.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 2: real fix

-- Keep a list of active nsFontFaceLoaders in the nsUserFontSet
-- Let each nsFontFaceLoader have a strong reference to its nsUserFontSet
-- When the nsUserFontSet gets detached from the presentation, it cancels all its nsFontFaceLoaders, which cancels the download and unhooks the reference from the loader to the fontset

This fixes the bug here by ensuring that a loader can never affect a font set that's not the active font set for the presentation.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed part 1 as a9f6f29f6fe5

Comment 15

[John Daggett (:jtd)]

Attachment: patch, part2a, updated patch and added null shell check

The part2 patch needed to be adjusted because of some code I checked in a couple days ago.  It also failed with the original testcase because in some cases ctx->PresShell might be null.  I added and null check and it no longer fails with the original testcase but I still see an assertion about the null presShell ptr.

Original testcase is no longer available at the download site so I uploaded it here:

http://people.mozilla.org/~jdaggett/bugs/462593iframefontloader.zip

Comment 16

[John Daggett (:jtd)]

Comment on attachment 355739 [details] [diff] [review]
part 2: real fix

Looks good but not sure whether the assertion with the original testcase is valid or not.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we need to call nsUserFontSet::Destroy from nsPresContext::SetShell(nsnull). That should fix it.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed b2c58375f633 with that change.

Comment 19

[Daniel Holbert [:dholbert]]

Patches pushed to 1.9.1 branch on behalf of roc.

Part 1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5954c9f70316
Part 2: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/36d04c3ebb98

Comment 20

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090407 Minefield/3.6a1pre (.NET CLR 3.5.30729)

Comment 21

[Aakash Desai [:aakashd]]

verified FIXED on Shiretoko: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre ID:20090422042031