1041525 - Possible deadlock in Crashreporter::OOPInit() from GMP plugin initialization

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Main thread stack: https://pastebin.mozilla.org/5586355
GMP thread stack: https://pastebin.mozilla.org/5586330

> <jesup> gfritzsche: looks like it's a deadlock: OOPInit SyncRunnable's to MainThread, and MainThread is SyncRunnableing to GMPThread
> <jesup> cpearce had a patch to proxy SyncLaunch to MainThread, but bsmedberg didn't want to do that, and updated things to keep SyncLaunch on GMPThread. Forget the bug number

Comment 1

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

Attachment: Disable GMP OOPInit() during SyncLaunch() until crashreporting fully hooked up

tested on Linux m-c opt --enable-crashreporter build that failed before in a possible SyncRunnable-to-mainthread deadlock

Comment 2

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

Attachment: Disable GMP OOPInit() during SyncLaunch() until crashreporting fully hooked up

remove unrelated change

Comment 3

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/40954106dc7a

Comment 4

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

Attachment: Disable GMP OOPInit() during SyncLaunch() until crashreporting fully hooked up

Comment 5

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

Attachment: Disable GMP OOPInit() during SyncLaunch() until crashreporting fully hooked up

Comment 6

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

Incorrect patch landed; backout and landed correct one:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81215b610d8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa68d4a514cd

Comment 7

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

Comment on attachment 8459601 [details] [diff] [review]
Disable GMP OOPInit() during SyncLaunch() until crashreporting fully hooked up

NOTE: request for Aurora/33, not 32, under the assumption this patch doesn't get included in the uplift (which seems likely)

Approval Request Comment
[Feature/regressing bug #]: GMP code as a whole (exact bug not relevant)

[User impact if declined]: OpenH264 will not work (crash/lockup) in opt builds with CrashReporter turned on

[Describe test coverage new/current, TBPL]: Dummy OpenH264 plugin and test in-tree; only runnable by hand until we get some help from the build peers this week.  (Bug 1037125 and follow-ons)  Note that this only fails in crashreporter-enabled builds!

[Risks and why]: minimal risk; specific disabling to the GMP plugin, where there are a few small missing pieces in getting GMP crash reporting up. (see bug 1041226, which may be related)

[String/UUID change made/needed]: none

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #8)
> https://hg.mozilla.org/mozilla-central/rev/fa68d4a514cd

This didn't make the cut for 33, for the record.

Comment 10

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

Attachment: some api proposals

Some unfinished WIP for API changes

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Deadlock are a pain. Tracking it.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8459601 [details] [diff] [review]
Disable GMP OOPInit() during SyncLaunch() until crashreporting fully hooked up

https://hg.mozilla.org/releases/mozilla-aurora/rev/e263af5237ff

Comment 13

[Georg Fritzsche [:gfritzsche]]

I can't reproduce this and the stacks in pastebin are gone already.
I'm not sure if we have any useful next steps for now.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Maybe jesup still has the stacks somewhere?

Comment 15

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

IIRC, new profile; download, go to http://mozilla.github.com/webrtc-landing/pc_test.html, click h264, start.

Needed to be an opt build with --enable-crashreporter.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Thanks, i tried with that today.
I can try some variations tomorrow and speculative reading of the code.

If that doesn't work i propose we re-enable this later tomorrow together with bug 1041226 fixed. Then we either see it's gone or should get crash-/hangreports.

Comment 17

[Georg Fritzsche [:gfritzsche]]

(This is given the time-frame here we're working with for re-enabling)

Comment 18

[Georg Fritzsche [:gfritzsche]]

adalucinet got a hang with an Aurora build from before workaround landed and produced a crash report with proper stacks:
bp-51371aef-307c-4c20-b72a-829752140723

Comment 19

[Georg Fritzsche [:gfritzsche]]

Reliably reproduced this on OS X & m-c rev 9383ccd71289 with --disable-debug, --enable-optimize, --enable-crashreporter.

The problem this ran into is two SyncRunnables blocking each other:
* main thread: http://hg.mozilla.org/mozilla-central/annotate/06ac51c2b8a8/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp#l131
* gmp thread: http://hg.mozilla.org/mozilla-central/annotate/06ac51c2b8a8/toolkit/crashreporter/nsExceptionHandler.cpp#l2557

Suggested simple and - per spot-checking - working fix: for the main-thread dispatch, use mGMPThread->Dispatch(..., NS_DISPATCH_SYNC) so it keeps spinning the event loop.

Comment 20

[Georg Fritzsche [:gfritzsche]]

Attachment: Dispatch using NS_DISPATCH_SYNC

Comment 21

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8459998 [details] [diff] [review]
some api proposals

This wasn't meant for this bug.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8461627 [details] [diff] [review]
Dispatch using NS_DISPATCH_SYNC

I can explain the related bug 1042348 now and local testing looks good.

Comment 23

[Georg Fritzsche [:gfritzsche]]

Attachment: Dispatch using NS_DISPATCH_SYNC

Forgot putting the re-enabling part up as well.

Comment 24

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

Comment on attachment 8461643 [details] [diff] [review]
Dispatch using NS_DISPATCH_SYNC

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

r+ to land just the GeckoChildProcessHost.cpp part (I landed fixes for the Dispatch on another bug already)

Comment 25

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ee16696f61e

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2ee16696f61e

Comment 27

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

Comment on attachment 8461643 [details] [diff] [review]
Dispatch using NS_DISPATCH_SYNC

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: No GMP crashreporter in Aurora/33

[Describe test coverage new/current, TBPL]: Marked test for GMP plugins for uplift; this gets called on every plug invocation

[Risks and why]: Failures if bug 1041232 isn't landed on Aurora first

[String/UUID change made/needed]: None

Comment 28

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

NOTE: it appears that we need to land bug 1041232 and this bug on Aurora to stop Aurora (especially Mac) crashes

Comment 29

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/a107c16ebe66

Comment 31

[u279076]

Nils, could you please verify this is fixed? It's probably sufficient to verify via testing that bug 1044249 no longer reproduces.

Comment 32

[Nils Ohlmeier [:drno]]

As we did not have exact procedure to reproduce the crashes I placed lots of manual H264 calls on pc_tes.html with FF 33 and 34 on Mac and Windows and I was not able to reproduce any crash any more.

Comment 33

[u279076]

Thanks for your help, Nils.