Crash in mozilla::MediaManager::PostTask

RESOLVED FIXED in Firefox 56

Status

()

defect
P2
critical
Rank:
13
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: philipp, Assigned: mchiang)

Tracking

({crash, regression})

56 Branch
mozilla57
All
Windows
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 wontfix, firefox56 fixed, firefox57 fixed)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

2 years ago
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
Looks like OnDeviceChange is calling EnumerateRawDevices in shutdown.

Munro, could you take a look?
Flags: needinfo?(mchiang)
Assignee: nobody → mchiang
Flags: needinfo?(mchiang)
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.
Rank: 13
Priority: -- → P1
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2

Comment 5

2 years ago
mozreview-review
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?
Attachment #8907918 - Flags: review?(apehrson) → review+
I have filed Bug 1399770 to follow this.
See Also: → 1399770

Comment 7

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ec3c4234c98a
skip devicechange event handler in shutdown stage. r=pehrsons
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ec3c4234c98a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Reporter

Comment 9

2 years ago
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?
Flags: needinfo?(mchiang)
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
Flags: needinfo?(mchiang)
Attachment #8907918 - Flags: approval-mozilla-beta?
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
Attachment #8907918 - Flags: approval-mozilla-release?
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.
Attachment #8907918 - Flags: approval-mozilla-release?
Attachment #8907918 - Flags: approval-mozilla-release+
Attachment #8907918 - Flags: approval-mozilla-beta?
Attachment #8907918 - Flags: approval-mozilla-beta-
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.