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

VERIFIED FIXED

Status

()

Core
Layout
P2
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: jtd)

Tracking

({crash, testcase, verified1.9.1})

Trunk
crash, testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

10 years ago
Created attachment 343233 [details]
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
(Assignee)

Comment 1

10 years ago
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.
(Assignee)

Updated

10 years ago
Assignee: nobody → jdaggett
(Reporter)

Comment 2

10 years ago
(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.
(Assignee)

Comment 3

10 years ago
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?
(Reporter)

Comment 4

10 years ago
Yes, worksforme, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081029 Minefield/3.1b2pre
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 5

10 years ago
Never mind, I still hit this crash while testing. I'll try to come up with a new testcase.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Reporter)

Comment 6

10 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 7

10 years ago
Created attachment 345799 [details]
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
(Reporter)

Updated

10 years ago
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: Crash [@ nsFontFaceLoader::OnDownloadComplete] when window is removed and page using font-face → Crash [@ nsFontFaceLoader::OnStreamComplete] when window is removed and page using font-face
(Reporter)

Updated

10 years ago
Attachment #343233 - Attachment is obsolete: true
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?
(Reporter)

Updated

10 years ago
Flags: blocking1.9.1?

Comment 9

10 years ago
(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
OS: Windows XP → All
Hardware: PC → All
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
(Assignee)

Comment 10

10 years ago
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.

Updated

10 years ago
Blocks: 441473
(Assignee)

Comment 11

10 years ago
Created attachment 351862 [details] [diff] [review]
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.
(Assignee)

Comment 12

10 years ago
Created attachment 352036 [details] [diff] [review]
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.
Attachment #351862 - Attachment is obsolete: true
Attachment #352036 - Flags: superreview?(roc)
Attachment #352036 - Flags: review?(roc)
(Assignee)

Comment 13

10 years ago
The attached changes fix both the crasher here and the crasher in bug 460743.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 460743
(Assignee)

Updated

10 years ago
Duplicate of this bug: 462593
(Assignee)

Updated

10 years ago
Blocks: 468568
(Assignee)

Updated

10 years ago
Blocks: 462593
+    // 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.
That approach also means the font loader can do something when the presshell is going away, if we ever need to.
(Assignee)

Comment 18

10 years ago
Created attachment 352279 [details] [diff] [review]
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.
Attachment #352036 - Attachment is obsolete: true
Attachment #352279 - Flags: superreview?(roc)
Attachment #352279 - Flags: review?(roc)
Attachment #352036 - Flags: superreview?(roc)
Attachment #352036 - Flags: review?(roc)
Attachment #352279 - Flags: superreview?(roc)
Attachment #352279 - Flags: superreview+
Attachment #352279 - Flags: review?(roc)
Attachment #352279 - Flags: review+
I assume that the only thing that owns the loader is necko, so there's no danger of cycles here?
(Assignee)

Comment 21

10 years ago
(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.
(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.
...and load interrupted due to user pressing "Stop".
(Assignee)

Comment 24

10 years ago
(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.
(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.
(Assignee)

Comment 26

10 years ago
Ah, ok, good then.  Won't hurt to null out the pointer anyways.
(Assignee)

Comment 27

10 years ago
Landed on trunk
http://hg.mozilla.org/mozilla-central/rev/4c81e3e5b6de
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: baking1.9.1
(Reporter)

Comment 28

10 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081214 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
(Assignee)

Comment 29

10 years ago
checked in on release branch
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e01c1f4e4c37
Keywords: fixed1.9.1
Whiteboard: baking1.9.1
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.
Keywords: fixed1.9.1 → verified1.9.1
Crash Signature: [@ nsFontFaceLoader::OnStreamComplete]
You need to log in before you can comment on or make changes to this bug.