861106 - crash in nsDocumentViewer::SetMinFontSize

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Scoobidiver (away)]

It's #116 browser crasher in 20.0 and #179 in 21.0b2.
It first showed up in 20.0a1/20121130 but is discontinuous across builds. The regression range might be:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=597915b66059&tochange=abb39d1df815

Signature 	nsDocumentViewer::SetMinFontSize(int) More Reports Search
UUID	0c2572a9-d6b8-4487-8c06-94ae82130412
Date Processed	2013-04-12 09:11:55
Uptime	4781
Install Age	1.6 hours since version was first installed.
Install Time	2013-04-12 07:33:15
Product	Firefox
Version	20.0.1
Build ID	20130409194949
Release Channel	release
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 42 stepping 7
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x0126, AdapterSubsysID: 04911028, AdapterDriverVersion: 8.15.10.2342
Has dual GPUs. GPU #2: AdapterVendorID2: 0x10de, AdapterDeviceID2: 0x0dcd, AdapterSubsysID2: 04911028, AdapterDriverVersion2: 8.17.12.6721D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
Processor Notes 	sp-processor01.phx1.mozilla.com_7073:2012; exploitability tool failed: 127
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x0126
Total Virtual Memory	4294836224
Available Virtual Memory	3527122944
System Memory Use Percentage	32
Available Page File	13966348288
Available Physical Memory	5720223744

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsDocumentViewer::SetMinFontSize 	layout/base/nsDocumentViewer.cpp:2869
1 	xul.dll 	nsDocShell::SetupNewViewer 	docshell/base/nsDocShell.cpp:8156
2 	xul.dll 	nsDocShell::Embed 	docshell/base/nsDocShell.cpp:6192
3 	xul.dll 	nsDocShell::CreateContentViewer 	docshell/base/nsDocShell.cpp:7923
4 	xul.dll 	nsDSURIContentListener::DoContent 	docshell/base/nsDSURIContentListener.cpp:122
5 	xul.dll 	nsDocumentOpenInfo::TryContentListener 	uriloader/base/nsURILoader.cpp:659
6 	xul.dll 	nsDocumentOpenInfo::DispatchContent 	uriloader/base/nsURILoader.cpp:360
7 	xul.dll 	nsDocumentOpenInfo::OnStartRequest 	uriloader/base/nsURILoader.cpp:252
8 	xul.dll 	mozilla::net::nsHttpChannel::CallOnStartRequest 	netwerk/protocol/http/nsHttpChannel.cpp:960
9 	xul.dll 	mozilla::net::nsHttpChannel::ContinueOnStartRequest2 	netwerk/protocol/http/nsHttpChannel.cpp:4940
10 	xul.dll 	mozilla::net::nsHttpChannel::OnStartRequest 	netwerk/protocol/http/nsHttpChannel.cpp:4913
11 	xul.dll 	nsInputStreamPump::OnStateStart 	netwerk/base/src/nsInputStreamPump.cpp:417
12 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:368
13 	xul.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:82
14 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
15 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:82
16 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:208
17 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:182
18 	xul.dll 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:163
19 	xul.dll 	nsAppShell::Run 	widget/windows/nsAppShell.cpp:154
20 	xul.dll 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3823
21 	xul.dll 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3890
22 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4093
23 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp:195
24 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:105
25 	firefox.exe 	__tmainCRTStartup 	crtexe.c:552
26 	kernel32.dll 	BaseThreadInitThunk 	
27 	ntdll.dll 	__RtlUserThreadStart 	
28 	ntdll.dll 	_RtlUserThreadStart 	

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsDocumentViewer%3A%3ASetMinFontSize%28int%29

Comment 1

[Scott Johnson (:jwir3)]

It looks like this is caused by a SetMinFontSize call while the document is still loading. Perhaps a race condition? If this is true, then I think the line that's causing this is this one:

>   mDocument->EnumerateExternalResources(SetExtResourceMinFontSize,
>                                        NS_INT32_TO_PTR(aMinFontSize));

So, we should probably check to make sure mDocument actually exists before doing this, as it seems like other functions specifically check this before accessing mDocument. Otherwise, we should probably wait until mDocument is initialized, then schedule SetMinFontSize() to be re-called.

Comment 2

[Scott Johnson (:jwir3)]

It looks like pogo.com is sticking out from the crash reports as being an area where FF has troubles...

Martijn, is there any way to construct a testcase for this? I can create a patch, but it would be nice to have some way of verifying that the patch fixes the crash.

Comment 3

[Martijn Wargers (dead)]

Could bug 493879 be related to this?

Comment 4

[Scott Johnson (:jwir3)]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #3)
> Could bug 493879 be related to this?

It does look related, but I can't seem to get a crash out of that bug. It does hang, but it actually looks like it's still doing something (i.e. the throbber is still going strong 5 or so minutes in). Plus, the debug output is still spewing out stuff of the form:

JavaScript error: file:///home/sjohnson/Desktop/crash1.xul, line 1: syntax error
WARNING: NS_ENSURE_SUCCESS(result, result) failed with result 0x80004005: file /home/sjohnson/Source/mozilla-central/mozilla/content/events/src/nsEventListenerManager.cpp, line 866
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && subjPrincipal) failed: file /home/sjohnson/Source/mozilla-central/mozilla/docshell/base/nsDocShell.cpp, line 8284
WARNING: An event was posted to a thread that will never run it (rejected): file /home/sjohnson/Source/mozilla-central/mozilla/xpcom/threads/nsThread.cpp, line 368
[Thread 0x7fffcf1ff700 (LWP 3252) exited]
###!!! ASSERTION: forward references have already been resolved: 'Error', file /home/sjohnson/Source/mozilla-central/mozilla/content/xul/document/src/XULDocument.cpp, line 1159
[New Thread 0x7fffcf1ff700 (LWP 3418)]
WARNING: Subdocument container has non-subdocument frame: file /home/sjohnson/Source/mozilla-central/mozilla/layout/base/nsDocumentViewer.cpp, line 2378
++DOMWINDOW == 35 (0xb059a0) [serial = 2922] [outer = 0x28d6860]

Over-and-over again...

Comment 5

[Scott Johnson (:jwir3)]

Also, by 'hang', I meant the throbber is still going, as I mentioned, and the debug output is still happening, but the UI is still responsive. So, it's not really a 'hang' per se... It's more like a slowdown in the UI responsiveness.

Comment 6

[Martijn Wargers (dead)]

Yeah, I guess that bug might be worksforme.

However, there is bug 731049 with the same stacktrace, it seems. And apparently, the reporter can reproduce it.

I'll try if I can get a reproducable testcase.

Comment 7

[Scott Johnson (:jwir3)]

Attachment: b861106

blassey, I'm flagging you for review, since it seems as though you wrote this function... please let me know if there is someone better to review it.

What this patch does it set a member variable for "pending" changes that require a document to proceed - SetTextZoom, SetFullZoom, and SetMinFontSize. Then, when mDocument changes (is assigned) in the document viewer, it "commits" these changes, essentially calling the set* function again.

I'm working on getting a crashtest ready for this bug, as well, but I didn't want to delay the review based on that.

Comment 8

[Scott Johnson (:jwir3)]

Comment on attachment 743688 [details] [diff] [review]
b861106

Just so that things are documented in the right places, I thought I would comment and state that I discussed this with Brad via email. Basically, there are two possible solutions to this problem (IMHO):

A) Check for mDocument != nullptr, and return an error on failure, or
B) Cache the value if mDocument is null, and when mDocument becomes non-null at some
point later, retry the Set* function.

Brad pointed out that I'm effectively implementing both solutions in this patch. This is true, and the reason for this is that if we cache the value, then, at a later time, commit these pending changes, there's a possibility that mDocument could have been set to nullptr. Thus, we would want to retain the cache (i.e. not reset the value of mPending* in the Commit() function). I communicate this failure via the NS_ERROR_* value.

Brad also stated that he thought just returning the error was fine, and forego the caching of the values. This is a simpler solution, and I'm testing a patch that does exactly this ATM.

Comment 9

[Scott Johnson (:jwir3)]

Attachment: b861106 (v2)

New version that just bails on null mDocument.

Comment 10

[Scott Johnson (:jwir3)]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08d7cf75697b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08d7cf75697b