778671 - crash in anonymous namespace::TelemetryImpl::RecordChromeHang

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Scoobidiver (away)]

It first appeared in 16.0a1/20120701. The regression range might be (low volume):
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f08d285b63b0&tochange=d9d61d199b11

Signature 	`anonymous namespace''::TelemetryImpl::RecordChromeHang(unsigned int, nsTArray<unsigned int, nsTArrayDefaultAllocator> const&, SharedLibraryInfo&) More Reports Search
UUID	c7eb2fbc-0014-4d4a-a25a-2be672120730
Date Processed	2012-07-30 07:16:27
Uptime	71
Last Crash	5.3 days before submission
Install Age	6.0 hours since version was first installed.
Install Time	2012-07-30 01:13:43
Product	Firefox
Version	17.0a1
Build ID	20120729030516
Release Channel	nightly
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	0x48
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x0116, AdapterSubsysID: 15821043, AdapterDriverVersion: 8.15.10.2696
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x0116
Total Virtual Memory	4294836224
Available Virtual Memory	3869265920
System Memory Use Percentage	24
Available Page File	10979766272
Available Physical Memory	4792958976

Frame 	Module 	Signature 	Source
0 	xul.dll 	`anonymous namespace'::TelemetryImpl::RecordChromeHang 	toolkit/components/telemetry/Telemetry.cpp:1300
1 	xul.dll 	mozilla::HangMonitor::ThreadMain 	xpcom/threads/HangMonitor.cpp:242
2 	nspr4.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:395
3 	nspr4.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:90
4 	msvcr100.dll 	_callthreadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:314
5 	msvcr100.dll 	_threadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:292
6 	kernel32.dll 	BaseThreadInitThunk 	
7 	ntdll.dll 	__RtlUserThreadStart 	
8 	ntdll.dll 	_RtlUserThreadStart

More reports at:
https://crash-stats.mozilla.com/report/list?signature=%60anonymous+namespace%27%27%3A%3ATelemetryImpl%3A%3ARecordChromeHang%28unsigned+int%2C+nsTArray%3Cunsigned+int%2C+nsTArrayDefaultAllocator%3E+const%26%2C+SharedLibraryInfo%26%29

Comment 1

[Vladan Djeric (:vladan)]

Attachment: Do bounds check on every loop iteration

Comment 2

[Vladan Djeric (:vladan)]

Attachment: Check bounds on every loop iteration + make sure Telemetry is initialized

Fixes 2 sources of crashes

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 656563 [details] [diff] [review]
Check bounds on every loop iteration + make sure Telemetry is initialized

Review of attachment 656563 [details] [diff] [review]:
-----------------------------------------------------------------

OK, but please add an explanation to the bug on how we get here with a NULL sTelemetry.

Comment 4

[Vladan Djeric (:vladan)]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #3)
> OK, but please add an explanation to the bug on how we get here with a NULL
> sTelemetry.

I think it might be because we initialize HangMonitor before Telemetry in nsXPComInit.cpp. It would also explain why we only have crashes from sTelemetry being NULL in RecordChromeHang but never in RecordSlowStatement. I don't know how we ended up with multi-hour uptimes in the hang reports but it might be because of a hang on startup. 

I will also change the order of Hang Monitor and Telemetry initializations.

Comment 5

[Vladan Djeric (:vladan)]

I think we hit sTelemetry = NULL after hours of uptime is because Telemetry is destructed before the HangMonitor. The new checks should fix this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9396aa559ea

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Vladan Djeric (:vladan) from comment #5)
> I think we hit sTelemetry = NULL after hours of uptime is because Telemetry
> is destructed before the HangMonitor. The new checks should fix this.


Should we change the order they are destructed in?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c9396aa559ea