crash in mozilla::ipc::AsyncChannel::CloseWithError

VERIFIED FIXED in Firefox 17

Status

()

Core
Plug-ins
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Scoobidiver (away), Assigned: Benjamin Smedberg)

Tracking

({crash, regression})

17 Branch
mozilla18
x86
Windows 7
crash, regression
Points:
---

Firefox Tracking Flags

(firefox17- verified, firefox18 verified)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

6 years ago
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.
Assignee: nobody → benjamin
(Assignee)

Comment 2

6 years ago
Created attachment 662581 [details] [diff] [review]
Nul-check, rev. 1
Attachment #662581 - Flags: review?(georg.fritzsche)
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?
(Assignee)

Comment 4

6 years ago
Created attachment 662996 [details] [diff] [review]
Shutdown check, rev. 2

Quite right! I believe this will work because ActorDestroy sets mShutdown correctly.
Attachment #662581 - Attachment is obsolete: true
Attachment #662581 - Flags: review?(georg.fritzsche)
Attachment #662996 - Flags: review?(georg.fritzsche)
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).
Attachment #662996 - Flags: review?(georg.fritzsche) → review+
(Assignee)

Comment 6

6 years ago
This one! I forgot about it but I think it's important enough to get up the channels.
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/970b0467c3ef
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox18: --- → fixed
tracking-firefox18: ? → ---
Resolution: --- → FIXED
Version: 17 Branch → 18 Branch

Comment 8

6 years ago
(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.
tracking-firefox17: ? → -
(Assignee)

Comment 9

6 years ago
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
Attachment #662996 - Flags: approval-mozilla-beta?
Attachment #662996 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 10

6 years ago
It's already in Aurora because it landed in m-c before the last merge.
status-firefox17: --- → affected
Target Milestone: --- → mozilla18
Version: 18 Branch → 17 Branch
(Assignee)

Updated

6 years ago
Attachment #662996 - Flags: approval-mozilla-aurora?
Comment on attachment 662996 [details] [diff] [review]
Shutdown check, rev. 2

Still early in the cycle, null check. Approved for Beta.
Attachment #662996 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/c72cdc4f3d95
status-firefox17: affected → fixed
Please verify this is fixed by checking Socorro.
Keywords: verifyme

Updated

6 years ago
status-firefox17: fixed → verified
status-firefox18: fixed → verified
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: ioana.budnar
You need to log in before you can comment on or make changes to this bug.