Closed Bug 462593 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Layout, defect, P2, critical)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: roc)

References

()

Details

(Keywords: crash, testcase, verified1.9.1)

Attachments

(5 files)

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
Summary: Crash [@ gfxMixedFontFamily::RemoveFontEntry] when closing tab in this → Crash [@ gfxMixedFontFamily::RemoveFontEntry] when closing tab in this case
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 460037
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.
Status: RESOLVED → REOPENED
Depends on: 460037
Resolution: DUPLICATE → ---
Attachment #352059 - Attachment mime type: text/plain → text/plain; charset=shift-jis
Attached file testcase
I see this crash occurring a lot with this zipped up testcase.
Summary: Crash [@ gfxMixedFontFamily::RemoveFontEntry] when closing tab in this case → Crash [out of memory] when rapidly opening multiple frames with downloadable fonts
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
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?
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)
Attachment #353027 - Attachment mime type: application/zip → application/java-archive
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.
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
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.
For the real patch, I need nsPtrHashKey. So I'm adding it to nsHashKeys.
Assignee: nobody → roc
Attachment #355737 - Flags: review?(benjamin)
Attached patch part 2: real fixSplinter Review
-- 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.
Attachment #355739 - Flags: review?(jdaggett)
Whiteboard: [needs review]
Attachment #355737 - Flags: review?(benjamin) → review+
Pushed part 1 as a9f6f29f6fe5
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
Attachment #355739 - Flags: review?(jdaggett) → review+
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.
I think we need to call nsUserFontSet::Destroy from nsPresContext::SetShell(nsnull). That should fix it.
Whiteboard: [needs review] → [needs landing]
Pushed b2c58375f633 with that change.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Whiteboard: [needs 191 landing]
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
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
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)
Status: RESOLVED → VERIFIED
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
You need to log in before you can comment on or make changes to this bug.