792372 - crash in mozilla::ipc::AsyncChannel::CloseWithError

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Scoobidiver (away)]

It's #71 top browser crasher in 17.0a2 and #116 in 18.0a1.
It was a low volume crash but has happened more often since 17.0a1/20120826. The regression range might be:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f077de66e52d&tochange=b3cce81fef1a

According to comments, it seems related to Flash games in Facebook.

Signature 	mozilla::ipc::AsyncChannel::CloseWithError() More Reports Search
UUID	dca891fe-da47-4117-8a2d-133ce2120917
Date Processed	2012-09-17 19:56:55
Uptime	566
Last Crash	1.8 weeks before submission
Install Age	9.4 minutes since version was first installed.
Install Time	2012-09-17 19:47:23
Product	Firefox
Version	18.0a1
Build ID	20120917030530
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 15 model 4 stepping 9
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x01d1, AdapterSubsysID: 341f1458, AdapterDriverVersion: 8.15.11.8593
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers- 
EMCheckCompatibility	True
Adapter Vendor ID	0x10de
Adapter Device ID	0x01d1
Total Virtual Memory	2147352576
Available Virtual Memory	1676615680
System Memory Use Percentage	38
Available Page File	3300843520
Available Physical Memory	1323638784

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::ipc::AsyncChannel::CloseWithError 	ipc/glue/AsyncChannel.cpp:799
1 	xul.dll 	mozilla::plugins::PluginModuleParent::OnCrash 	dom/plugins/ipc/PluginModuleParent.cpp:1434
2 	xul.dll 	CrashReporter::ReportInjectedCrash::Run 	toolkit/crashreporter/nsExceptionHandler.cpp:2311
3 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:624
4 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:82
5 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:201
6 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:175
7 	xul.dll 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:163
8 	xul.dll 	nsAppShell::Run 	widget/windows/nsAppShell.cpp:232
9 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:296
10 	xul.dll 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3834
11 	xul.dll 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3911
12 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3987
13 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:100
14 	firefox.exe 	__tmainCRTStartup 	crtexe.c:552
15 	kernel32.dll 	BaseThreadInitThunk 	
16 	ntdll.dll 	__RtlUserThreadStart 	
17 	ntdll.dll 	_RtlUserThreadStart

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Aipc%3A%3AAsyncChannel%3A%3ACloseWithError%28%29

Comment 1

[Benjamin Smedberg]

This is a race, probably made more common by:

2bcda3ce2bc9	Chris Jones — Bug 784647: Ensure that Tasks and XPCOM events are dispatched with the same priority. r=bent

A null check should suffice.

Comment 2

[Benjamin Smedberg]

Attachment: Nul-check, rev. 1

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 662581 [details] [diff] [review]
Nul-check, rev. 1

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +1431,5 @@
>  void
>  PluginModuleParent::OnCrash(DWORD processID)
>  {
> +    ::mozilla::ipc::AsyncChannel* channel = GetIPCChannel();
> +    if (channel) {

Maybe i'm missing something, but PPluginModuleParent::GetIPCChannel() returns (non-overloaded) &mChannel, which can't be null.
This might instead be a race with the channel being Clear()ed already?

Comment 4

[Benjamin Smedberg]

Attachment: Shutdown check, rev. 2

Quite right! I believe this will work because ActorDestroy sets mShutdown correctly.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 662996 [details] [diff] [review]
Shutdown check, rev. 2

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +1431,5 @@
>  void
>  PluginModuleParent::OnCrash(DWORD processID)
>  {
> +    if (!mShutdown) {
> +        GetIPCChannel->CloseWithError();

Missing the "()" after "GetIPCChannel".
It might still make sense to check or assert on the channel not being null as the interface doesn't guarantee it (to avoid accidentally breaking it).

Comment 6

[Benjamin Smedberg]

This one! I forgot about it but I think it's important enough to get up the channels.

Comment 7

[Benjamin Smedberg]

https://hg.mozilla.org/mozilla-central/rev/970b0467c3ef

Comment 8

[Alex Keybl [:akeybl]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> This one! I forgot about it but I think it's important enough to get up the
> channels.

This isn't a top crasher, so no need to track. That being said, we would accept a 17 uplift nomination given the low risk nature of this patch.

Comment 9

[Benjamin Smedberg]

Comment on attachment 662996 [details] [diff] [review]
Shutdown check, rev. 2

Bug caused by (feature/regressing bug #): bug 769048
User impact if declined: Firefox sometimes crashes when Flash crashes
Testing completed (on m-c, etc.): landed without incident on m-c. The crash signature appears to have already been abset on m-c so I'm not sure if some other patch also made this go away on trunk.

Risk to taking this patch (and alternatives if risky): It's a null check, and basically risk-free.

String or UUID changes made by this patch: none

Comment 10

[Scoobidiver (away)]

It's already in Aurora because it landed in m-c before the last merge.

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 662996 [details] [diff] [review]
Shutdown check, rev. 2

Still early in the cycle, null check. Approved for Beta.

Comment 12

[Benjamin Smedberg]

https://hg.mozilla.org/releases/mozilla-beta/rev/c72cdc4f3d95

Comment 13

[u279076]

Please verify this is fixed by checking Socorro.

Comment 14

[Ioana (away)]

https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=mozilla%3A%3Aipc%3A%3AAsyncChannel%3A%3ACloseWithError%28%29

No crashes with this signature are listed in Socorro on any builds with the fix.