1399395 - Crash in mozilla::MediaManager::PostTask

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-74786dee-0a1e-4abe-9714-9047a0170912.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::MediaManager::PostTask(already_AddRefed<mozilla::Runnable>) 	dom/media/MediaManager.cpp:2001
1 	xul.dll 	mozilla::MediaManager::EnumerateRawDevices(unsigned __int64, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool) 	dom/media/MediaManager.cpp:1789
2 	xul.dll 	<lambda_8f9699fc120d46dcab1291f5e26e5bf6>::operator() 	dom/media/MediaManager.cpp:2077
3 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1446
4 	xul.dll 	nsThread::Shutdown() 	xpcom/threads/nsThread.cpp:1045
5 	xul.dll 	mozilla::ProcessHangMonitor::~ProcessHangMonitor() 	dom/ipc/ProcessHangMonitor.cpp:1132
6 	xul.dll 	mozilla::ProcessHangMonitor::`scalar deleting destructor'(unsigned int) 	
7 	xul.dll 	mozilla::ProcessHangMonitor::Release() 	dom/ipc/ProcessHangMonitor.cpp:1146
8 	xul.dll 	RefPtr<mozilla::ProcessHangMonitor>::~RefPtr<mozilla::ProcessHangMonitor>() 	obj-firefox/dist/include/mozilla/RefPtr.h:78
9 	xul.dll 	`anonymous namespace'::HangMonitorParent::~HangMonitorParent 	dom/ipc/ProcessHangMonitor.cpp:606
10 	xul.dll 	`anonymous namespace'::HangMonitorParent::`scalar deleting destructor'(unsigned int) 	
11 	xul.dll 	mozilla::ProcessHangMonitor::RemoveProcess(mozilla::PProcessHangMonitorParent*) 	dom/ipc/ProcessHangMonitor.cpp:1295
12 	xul.dll 	mozilla::dom::ContentParent::ActorDestroy(mozilla::ipc::IProtocol::ActorDestroyReason) 	dom/ipc/ContentParent.cpp:1701
13 	xul.dll 	mozilla::dom::PContentParent::OnChannelError() 	obj-firefox/ipc/ipdl/PContentParent.cpp:8437
14 	xul.dll 	mozilla::dom::ContentParent::OnChannelError() 	dom/ipc/ContentParent.cpp:1543
15 	xul.dll 	mozilla::ipc::MessageChannel::NotifyMaybeChannelError() 	ipc/glue/MessageChannel.cpp:2555
16 	xul.dll 	mozilla::ipc::MessageChannel::OnNotifyMaybeChannelError() 	ipc/glue/MessageChannel.cpp:2586
17 	xul.dll 	mozilla::detail::RunnableMethodImpl<mozilla::ipc::MessageChannel* const, void ( mozilla::ipc::MessageChannel::*)(void), 0, 1>::Run() 	xpcom/threads/nsThreadUtils.h:1172
18 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1446
19 	xul.dll 	XPTC__InvokebyIndex 	xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_64.asm:97
20 	xul.dll 	JSScript::maybeSweepTypes(js::AutoClearTypeInferenceStateOnOOM*) 	js/src/vm/TypeInference.cpp:4431

this crash signature is regressing in volume during the 56.0b cycle:
https://crash-stats.mozilla.com/signature/?product=Firefox&release_channel=beta&signature=mozilla%3A%3AMediaManager%3A%3APostTask&date=%3E%3D2017-06-13T09%3A41%3A27.000Z#graphs

Comment 1

[Andreas Pehrson [:pehrsons]]

Looks like OnDeviceChange is calling EnumerateRawDevices in shutdown.

Munro, could you take a look?

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Simple fix would be to leak the task and not crash (but complain).  More complex (perhaps not very) would be to return an error and let the caller clean up correctly.  

It's interesting that it spiked in intensity.

P1/13 for the simple patch, which is also upliftable.

Comment 3

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 4

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1399395 - skip devicechange event handler in shutdown stage.

Review commit: https://reviewboard.mozilla.org/r/179596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179596/

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8907918 [details]
Bug 1399395 - skip devicechange event handler in shutdown stage.

https://reviewboard.mozilla.org/r/179596/#review184850

This seems all right to fix the spike. Could you also file a bug to fix PostTask per comment 2?

Comment 6

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

I have filed Bug 1399770 to follow this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec3c4234c98a
skip devicechange event handler in shutdown stage. r=pehrsons

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 9

[[:philipp]]

this seems to be a fairly simple fix. could you request an uplift to 56 (which is mozilla-release by now) if you deem fit to do so?

Comment 10

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8907918 [details]
Bug 1399395 - skip devicechange event handler in shutdown stage.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1364038
[User impact if declined]: content process may crash in some circumstance
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: small change which fixes the crash
[String changes made/needed]: no

Comment 11

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8907918 [details]
Bug 1399395 - skip devicechange event handler in shutdown stage.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1364038
[User impact if declined]: content process may crash in some circumstance
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: small change which fixes the crash
[String changes made/needed]: no

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8907918 [details]
Bug 1399395 - skip devicechange event handler in shutdown stage.

Fix for a crash that got significantly worse in 56. 
Let's land this on m-r for the 56 RC build next Monday.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0bfaf562bf40 (FIREFOX_56b13_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/e5b95529a8f8

Comment 14

[Andrei Vaida [:avaida]]

(In reply to Munro Mengjue Chiang [:mchiang] from comment #11)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Munro Mengjue Chiang's assessment on manual testing needs and the fact that this fix has automated coverage.