460037 - Crash [@ nsFontFaceLoader::OnStreamComplete] when window is removed and page using font-face

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: zipped up testcase

See zipped up testcase, to reproduce:
- Unzip zipped up testcase
- Open the file named 'nsFontFaceLoaderOnDownloadComplete.htm'

It usually crashes after a short while, if it doesn't crash quickly enough, try opening another instance of it in a new tab.

I guess using data urls for @font-face is not working, right? Because if that was working, I could have wrapped the testcase as a simple .htm file to the bug.

http://crash-stats.mozilla.com/report/index/c02bce82-9abc-11dd-a219-0013211cbf8a?p=1
0  	xul.dll  	nsFontFaceLoader::OnDownloadComplete  	 layout/style/nsFontFaceLoader.cpp:214
1 	xul.dll 	nsDownloader::OnStopRequest 	netwerk/base/src/nsDownloader.cpp:163
2 	xul.dll 	nsBaseChannel::OnStopRequest 	netwerk/base/src/nsBaseChannel.cpp:662
3 	xul.dll 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:576
4 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:401
5 	xul.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:111
6 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
7 	xul.dll 	NS_ProcessPendingEvents_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:180
8 	xul.dll 	xul.dll@0x2ac0db 	
9 	xul.dll 	nsAppShell::EventWindowProc 	widget/src/windows/nsAppShell.cpp:75
10 	user32.dll 	InternalCallWinProc 	
11 	user32.dll 	UserCallWinProcCheckWow 	
12 	user32.dll 	DispatchMessageWorker 	
13 	user32.dll 	DispatchMessageW 	
14 	ole32.dll 	ole32.dll@0xef9e3 	
15 	ole32.dll 	ole32.dll@0xf0483 	
16 	xul.dll 	nsDragService::StartInvokingDragSession 	widget/src/windows/nsDragService.cpp:315
17 	xul.dll 	nsDragService::InvokeDragSession 	widget/src/windows/nsDragService.cpp:263
18 	xul.dll 	nsBaseDragService::InvokeDragSessionWithImage 	widget/src/xpwidgets/nsBaseDragService.cpp:275
19 	xul.dll 	nsEventStateManager::DoDefaultDragStart 	content/events/src/nsEventStateManager.cpp:2320
20 	xul.dll 	nsEventStateManager::GenerateDragGesture 	content/events/src/nsEventStateManager.cpp:2075
21 	xul.dll 	xul.dll@0x3100ec 	
22 	xul.dll 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:5915
23 	xul.dll 	PresShell::HandlePositionedEvent 	layout/base/nsPresShell.cpp:5813
24 	xul.dll 	xul.dll@0x3114a8 	
25 	xul.dll 	nsViewManager::HandleEvent 	view/src/nsViewManager.cpp:1389
26 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:1348
27 	xul.dll 	HandleEvent 	view/src/nsView.cpp:167
28 	xul.dll 	nsWindow::DispatchEvent 	widget/src/windows/nsWindow.cpp:955
29 	xul.dll 	nsWindow::DispatchWindowEvent 	widget/src/windows/nsWindow.cpp:975
30 	xul.dll 	nsWindow::DispatchMouseEvent 	widget/src/windows/nsWindow.cpp:6212
31 	xul.dll 	ChildWindow::DispatchMouseEvent 	widget/src/windows/nsWindow.cpp:6359
32 	xul.dll 	xul.dll@0x32c575 	
33 	xul.dll 	nsWindow::WindowProc 	widget/src/windows/nsWindow.cpp:1165
34 	xul.dll 	xul.dll@0x27c0d1 	
35 	xul.dll 	nsAnonymousContentList::Release

Comment 1

[John Daggett (:jtd)]

Data url's work but you need to disable same-site origin checking for now.  This probably should be considered a bug.  As part of the work on implementing support for access control headers, I'm going to look over the load policy code again.

Comment 2

[Martijn Wargers (dead)]

(In reply to comment #1)
> Data url's work but you need to disable same-site origin checking for now. 

Ah, gotcha! Just found the gfx.downloadable_fonts.enforce_same_site_origin pref. Thanks.

Comment 3

[John Daggett (:jtd)]

Note: testcase requires VeraSeBd.ttf in the same directory as the testcase files.  Download here:

http://ftp.gnome.org/pub/GNOME/sources/ttf-bitstream-vera/1.10/

With the latest nightly, this works for me.  I let five tabs run the testcase for 30 minutes with no crash.  I think the fix for bug 458861 may have fixed this, since with this change font downloader objects are added to the load group so they get cleaned up when the window is closed before the font download completes.

Martijn, could you test this again with the latest nightly to see if the crash still occurs or not?

Comment 4

[Martijn Wargers (dead)]

Yes, worksforme, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081029 Minefield/3.1b2pre

Comment 5

[Martijn Wargers (dead)]

Never mind, I still hit this crash while testing. I'll try to come up with a new testcase.

Comment 6

[Martijn Wargers (dead)]

Sorry, it seems to be indeed worksforme, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081030 Minefield/3.1b2pre

I forgot to include the VeraSeBd.ttf font, which I forgot to include in the zipped up testcase, that's why it was worksforme, earlier.

Comment 7

[Martijn Wargers (dead)]

Attachment: zipped up testcase2

I can still see the bug with this testcase, which is using a font that is large in size. You can download the font here (8MB): http://martijn.martijn.googlepages.com/CODE2000.TTF

The stacktrace is slightly different, it seems:
http://crash-stats.mozilla.com/report/index/8f5a0017-a790-11dd-9f45-001a4bd43ed6?p=1
0  	xul.dll  	nsFontFaceLoader::OnStreamComplete  	 layout/style/nsFontFaceLoader.cpp:152
1 	xul.dll 	nsStreamLoader::OnStopRequest 	netwerk/base/src/nsStreamLoader.cpp:108
2 	xul.dll 	nsBaseChannel::OnStopRequest 	netwerk/base/src/nsBaseChannel.cpp:677
3 	xul.dll 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:576
4 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:401
5 	xul.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:111
6 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
7 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
8 	nspr4.dll 	PR_GetEnv 	
9 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:87
10 	firefox.exe 	firefox.exe@0x2197 	
11 	kernel32.dll 	BaseProcessStart

Comment 8

[(mostly gone) XtC4UaLL [:xtc4uall]]

just found on crash-stats.mozilla.com bp-85a43619-a8af-11dd-91b1-001cc45a2ce4

0 	XUL 	nsFontFaceLoader::OnStreamComplete(nsIStreamLoader*, nsISupports*, unsigned int, unsigned int, unsigned char const*) 	layout/style/nsFontFaceLoader.cpp:152
1 	XUL 	nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) 	netwerk/base/src/nsStreamLoader.cpp:108
2 	XUL 	nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) 	netwerk/base/src/nsBaseChannel.cpp:677
3 	XUL 	nsInputStreamPump::OnStateStop() 	netwerk/base/src/nsInputStreamPump.cpp:576
4 	XUL 	nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) 	netwerk/base/src/nsInputStreamPump.cpp:401
5 	XUL 	nsInputStreamReadyEvent::Run() 	xpcom/io/nsStreamUtils.cpp:111
6 	XUL 	nsThread::ProcessNextEvent(int, int*) 	xpcom/threads/nsThread.cpp:510
7 	XUL 	NS_ProcessPendingEvents_P(nsIThread*, unsigned int) 	nsThreadUtils.cpp:180
8 	XUL 	nsBaseAppShell::NativeEventCallback() 	widget/src/xpwidgets/nsBaseAppShell.cpp:121
9 	XUL 	nsAppShell::ProcessGeckoEvents(void*) 	widget/src/cocoa/nsAppShell.mm:373
10 	CoreFoundation 	CoreFoundation@0x72614 	
11 	CoreFoundation 	CoreFoundation@0x72cf7 	
12 	HIToolbox 	HIToolbox@0x2f47f 	
13 	HIToolbox 	HIToolbox@0x2f1d1 	
14 	HIToolbox 	HIToolbox@0x2f10c 	
15 	AppKit 	AppKit@0x403ec 	
16 	AppKit 	AppKit@0x3fc9f 	
17 	AppKit 	AppKit@0x38cda 	
18 	XUL 	nsAppShell::Run() 	widget/src/cocoa/nsAppShell.mm:692
19 	XUL 	nsAppStartup::Run() 	toolkit/components/startup/src/nsAppStartup.cpp:182
20 	XUL 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3263
21 	firefox-bin 	main 	browser/app/nsBrowserApp.cpp:156
22 	firefox-bin 	_start 	
23 	firefox-bin 	start

similiar to the one of comment #c7, but on MacOSX platform with Build ID 20081031020241
=> ALL or new bug?

Comment 9

[philippe (part-time)]

(In reply to comment #8)
> ... bp-85a43619-a8af-11dd-91b1-001cc45a2ce4

I'm pretty sure that one is mines, with Martijn's test case in comment 7.

> => ALL or new bug?
yeah, ALL

Comment 10

[John Daggett (:jtd)]

Much simpler testcase:

Steps:

1. Open http://opentype.info/demo/webfontdemo.html
2. Wait for the page to finish loading
3. Select Print
4. Click on the Preview button

Result: blammo!  Welcome to Crash Reporter

The problem is here:

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsFontFaceLoader.h#94

The nsFontFaceLoaderContext class keeps a ptr to the nsPresContext but when the nsPresContext goes way, this will be a bum pointer.  Switching to nsCOMPtr<nsPresContext> fixes the crashing problem but introduces lots of leaks for some reason.

Comment 11

[John Daggett (:jtd)]

Attachment: patch, v.0.1, restructure loading of downloadable fonts

As suggested by roc in comment 87 of bug 441473, create a StartLoad virtual method of gfxUserFontSet, and create a subclass of this within layout code that initiates the font download process using nsFontFaceLoader objects.  This eliminates the need for LoaderContext gluk and makes the code simpler.  The subclass, nsUserFontSet, maintains a weak pointer to the pres context.  Since the user font set is owned by the pres context, this will always be live while the user font set is live.

During the font download process, nsFontFaceLoader objects maintain a weak pointer to the presShell using nsWeakPtr.  If the pres shell is torn down before the font download completes, the mShell member will be notified and cleaned out, eliminating the chance of a crash.

Needs some more testing.

Comment 12

[John Daggett (:jtd)]

Attachment: patch, v.0.2, update to latest

The diff makes this look more complicated than it really is, the StartLoad method in the nsUserFontSet subclass replaces the CreateHandler static method of nsFontFaceLoader, the code is identical except for the first couple lines.  CheckLoadAllowed is the same but is now public to allow the StartLoad method to call it.  I didn't switch all the instances of gfxUserFontSet to nsUserFontSet, I don't think that's necessary.

Comment 13

[John Daggett (:jtd)]

The attached changes fix both the crasher here and the crasher in bug 460743.

Comment 16

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

+    // whether an error occurred or not, notify the user font set of the completion
+  fontUpdate = ctx->GetUserFontSet()->OnLoadComplete(mFontEntry, aLoader,

+  // Update layout for the presence of the new font.  Since this is
+  // asynchronous, reflows will coalesce.
+    ctx->UserFontSetUpdated();

Your comment indents are weird.

Basically fine, but we don't actually need to use real weak refs here. You could just hold a regular nsCOMPtr<nsIPresShell> and have nsPresContext::SetShell update that reference via a method in mUserFontSet. I would prefer to do that; Someone might be holding a reference to the nsIPresShell so it stays alive long after it's been Destroy()ed.

Comment 17

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

That approach also means the font loader can do something when the presshell is going away, if we ever need to.

Comment 18

[John Daggett (:jtd)]

Attachment: patch, v.0.3, use nsCOMPtr<nsIPresShell> instead

Instead of a nsWeakPtr, use a nsCOMPtr<nsIPresShell> and check IsDestroying before calling the OnLoadComplete method.  

This fixes the original bug but doesn't handle the problem in bug 462593, that appears to be a pathological case where new page loads are somehow swamping out cleanup of old pages.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I assume that the only thing that owns the loader is necko, so there's no danger of cycles here?

Comment 20

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

I believe so.

Comment 21

[John Daggett (:jtd)]

(In reply to comment #19)
> I assume that the only thing that owns the loader is necko, so there's no
> danger of cycles here?

On Linux we maintain a reference to the original font loader as a way of keeping the font data in memory, since FT doesn't copy font data on load (Windows/Mac do).  To avoid cycles we can null out the presShell ptr at the end of OnDownloadComplete, that should be sufficient.

Comment 22

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

(In reply to comment #21)
> On Linux we maintain a reference to the original font loader as a way of
> keeping the font data in memory, since FT doesn't copy font data on load
> (Windows/Mac do).  To avoid cycles we can null out the presShell ptr at the end
> of OnDownloadComplete, that should be sufficient.

If that's necessary to avoid cycles, please make sure it also happens with various forms of network error (e.g., malformed URI, DNS failure, can't connect to host) if the cycle is still possible with those types of errors.

Comment 23

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

...and load interrupted due to user pressing "Stop".

Comment 24

[John Daggett (:jtd)]

(In reply to comment #22) 
> If that's necessary to avoid cycles, please make sure it also happens with
> various forms of network error (e.g., malformed URI, DNS failure, can't connect
> to host) if the cycle is still possible with those types of errors.

All of these events cause a stop event which will end up calling the OnDownloadComplete method.  A status parameter is passed in to reflect the nature of the error.  The font system will only take a reference from within gfxUserFontSet::OnLoadComplete which is called within OnDownloadComplete.  On Linux the reference is only hung onto if the download was successful, so network errors won't produce cycles.  So as long as the presShell reference is set to null after a call to OnLoadComplete we should be fine.

Note: the OnLoadComplete method of the user font set needs to get called in failure cases because we need to move to the load of the next url in the src chain.

Comment 25

[Karl Tomlinson (:karlt)]

(In reply to comment #21)
> (In reply to comment #19)
> > I assume that the only thing that owns the loader is necko, so there's no
> > danger of cycles here?
> 
> On Linux we maintain a reference to the original font loader as a way of
> keeping the font data in memory, since FT doesn't copy font data on load
> (Windows/Mac do).

The Linux code maintains a reference only to the nsIStreamLoader, which will release its reference to the nsFontFaceLoader (nsIStreamLoaderObserver) during nsStreamLoader::OnStopRequest...

> To avoid cycles we can null out the presShell ptr at the end
> of OnDownloadComplete, that should be sufficient.

... so I doubt the nsIStreamLoader reference would necessitate this.

Comment 26

[John Daggett (:jtd)]

Ah, ok, good then.  Won't hurt to null out the pointer anyways.

Comment 27

[John Daggett (:jtd)]

Landed on trunk
http://hg.mozilla.org/mozilla-central/rev/4c81e3e5b6de

Comment 28

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081214 Minefield/3.2a1pre

Comment 29

[John Daggett (:jtd)]

checked in on release branch
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e01c1f4e4c37

Comment 30

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre.