1374258 - Crash in mozilla::ipc::Endpoint<T>::Bind from ImageBridgeParent

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-f6406714-2bf5-4b2d-ae54-c795a0170618.
=============================================================
Crashing Thread (24), Name: Compositor
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::ipc::Endpoint<mozilla::PProfilerChild>::Bind(mozilla::PProfilerChild*) 	obj-firefox/dist/include/mozilla/ipc/ProtocolUtils.h:649
1 	xul.dll 	mozilla::layers::ImageBridgeParent::Bind(mozilla::ipc::Endpoint<mozilla::layers::PImageBridgeParent>&&) 	gfx/layers/ipc/ImageBridgeParent.cpp:221
2 	xul.dll 	mozilla::detail::RunnableMethodImpl<mozilla::dom::HTMLMediaElement* const, void ( mozilla::dom::HTMLMediaElement::*)(nsACString const&), 1, 0, nsCString>::Run() 	obj-firefox/dist/include/nsThreadUtils.h:1133
3 	xul.dll 	MessageLoop::RunTask(already_AddRefed<mozilla::Runnable>) 	ipc/chromium/src/base/message_loop.cc:361
4 	xul.dll 	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask&&) 	ipc/chromium/src/base/message_loop.cc:369
5 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc:444
6 	xul.dll 	base::MessagePumpForUI::DoRunLoop() 	ipc/chromium/src/base/message_pump_win.cc:212
7 	xul.dll 	base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate*, base::MessagePumpWin::Dispatcher*) 	ipc/chromium/src/base/message_pump_win.cc:56
8 	xul.dll 	base::MessagePumpWin::Run(base::MessagePump::Delegate*) 	ipc/chromium/src/base/message_pump_win.h:80
9 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
10 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
11 	xul.dll 	base::Thread::ThreadMain() 	ipc/chromium/src/base/thread.cc:180
12 	xul.dll 	`anonymous namespace'::ThreadFunc 	ipc/chromium/src/base/platform_thread_win.cc:28
13 	kernel32.dll 	BaseThreadInitThunk 	
14 	mozglue.dll 	patched_BaseThreadInitThunk 	mozglue/build/WindowsDllBlocklist.cpp:809
15 	ntdll.dll 	__RtlUserThreadStart 	
16 	ntdll.dll 	_RtlUserThreadStart

reports with this signature seem to be increasing in the 55.0b cycle judging from early stability data of the staged rollout there (the signature has been present for a while longer though). reports come from users on windows and generally have MOZ_RELEASE_ASSERT(mMyPid == base::GetCurrentProcId()) that got added in bug 1223240.

Comment 1

[Andrew McCreight [:mccr8]]

I looked at the proto signatures, and almost all of these crashes are in ImageBridgeParent.

Comment 2

[Andrew McCreight [:mccr8]]

None of these crashes have the "process type" field set, which I think means they are in the main process.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

My guess is this is a super rare race here [1]. We call EnsureGPUReady after binding a PID to the pipe. If it returns false, we'll try to bind it in the parent process instead, and the PIDs won't match.

We should probably call EnsureGPUReady at the top and use the return value where we checked it previously (and where we check mGPUChild). This would affect the other pipe-y functions as well.

[1] http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/gfx/ipc/GPUProcessManager.cpp#802

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

This was partially fixed by bug 1314543.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Comment 6

[Mike Taylor [:miketaylr]]

David, wanna land this patch?

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Oh dear, I thought I landed this already. Will do.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a45473c94b8d
Fix race where in-process GPU endpoints could have the wrong PID. (bug 1374258, r=rhunt)

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[[:philipp]]

i don't see any more crashes on nightly after build 20170630030203 - could you request an uplift to beta as you deem fit?

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8879271 [details] [diff] [review]
fix

Approval Request Comment
[Feature/Bug causing the regression]: Compositor Process (firefox 53)
[User impact if declined]: rare crash when the compositor process crashes during launch
[Is this code covered by automated tests?]:
[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]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: This is not an invasive change, we're just checking state earlier to avoid a race.
[String changes made/needed]:

Note: I'm not sure if this patch will apply to beta, since this code was refactored recently. If not let me know and I will make a new patch.

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8879271 [details] [diff] [review]
fix

fix a race condition leading to crash, beta55+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Needs a rebased patch.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Minimal fix for beta.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/57d9f61d45fc

Comment 16

[Andrei Vaida [:avaida]]

(In reply to David Anderson [:dvander] from comment #11)
> [Is this code covered by automated tests?]:
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on David's assessment on manual testing needs.