1163456 - Assertion failure: mGMP, at media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp:326 when GMP crashes

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P1)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

While working on bug 1161814, got this issue on Mac non-e10s:
1) Visit http://mozilla.github.io/webrtc-landing/pc_test.html on a device that has some kind of camera enabled
2) Make sure "Require H.264 video" is checked, and then click "Start"
3) You should see two feeds - the one on the right is live, the one on the left is being encoded via H.264.
4) Open about:config, and set (or add, if it's your first time) media.gmp.plugin.crash to true to crash the GMP process.

It sometimes asserts:
Assertion failure: mGMP, at media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp:326
Which is in WebrtcGmpVideoEncoder::Encode_g:
  MOZ_ASSERT(mGMP);

Gut-feel: Terminated() cleared mGMP between Encode() and Encode_g, so we probably shouldn't assert, but just test for null pointer.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1163456-mGMP-check.patch

Quick&naive patch: Check for null pointers in dispatched methods, instead of asserting.
I'd first like to know if that even makes sense before testing&reviewing further.

ni:rjesup as you seem to be the expert!? Please forward to someone else if more appropriate.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

This bug is a dupe of older bug 1161589.
However before closing it as dupe, I'd like to know if the proposed patch here makes sense, in which case we could move it there, or just apply it and close both bugs.

Comment 3

[Maire Reavy [:mreavy]]

(In reply to Gerald Squelart [:gerald] from comment #2)
> This bug is a dupe of older bug 1161589.
> However before closing it as dupe, I'd like to know if the proposed patch
> here makes sense, in which case we could move it there, or just apply it and
> close both bugs.

cpearce, jesup -- Can you look at the patch here and advise on the best way to proceed?  This is pretty important (P1) since crashing the plugin is bringing down the browser.

Comment 4

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

Just checking for a null ptr here implies strongly that mGMP and/or mHost are being accessed (and changed) from multiple threads independently (i.e. not strict ownership); this would almost certainly be a TSAN violation, and also there's no guarantee in that case that the CPU may not have fetched the pointer into a register, and then if gets cleared (or worse, deallocated) by another thread, the pointer could get used leading to a UAF (though in this case, likely it would just Dispatch() to GMPThread, which would null-deref using mGMP, so no UAF in practice, except maybe in ~WebrtcGmpVideoEncoder/Decoder (see below)).

The answer is to move all (or all but we-know-the-gmpthread-won't-touch-it) accesses to mGMP to the GMPThread.  Most are already; largely it's *removing* MOZ_ASSERT(mGMP) and if (!mGMP) { return failure; } code from non-gmp-thread calls (like Encode()), and making the gmp side generally test if mGMP is null when starting a function called via a runnable (like Encode_g()), as in this patch.  ~WebrtcGmpVideoEncoder/Decoder will need to not pass the value of mGMP in the SyncRunnable, but instead pass 'this' so the other thread can fetch it directly.

This may help with the other bug 1161589 as well.

cpearce: sound good?
gerald: can you update the patch?

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8603907 [details] [diff] [review]
1163456-mGMP-check.patch

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

r- (to make sure this patch doesn't get used) on behalf of :jesup, see comment 4 for details.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Randell Jesup [:jesup] from comment #4)

Sorry, this is above my level of incompetence. You should assign someone who knows WebrtcGmpVideoEncoder well to look into this issue.

Comment 7

[Maire Reavy [:mreavy]]

I talked with Randell.  He is taking this;  he has a WIP patch already, but it's deadlocking.

Comment 8

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

The patch in question is utterly bitrotted now - but the main issue seems to have been resolved by intervening updates, most notably bug 1182289.