1224442 - Armor GMP Parent against late Shmem messages from the child

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, defect)

Description

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

After we call Shutdown() in the parent, and send EncodingComplete to the child, we can receive a RecvParentShmemForPool() (I know) or AnswerNeedShmem() (I believe).  Other RecvXxxx() calls all have checks that block post-shutdown access.

This is easiest to see on Android with OpenH264 1.5 or 1.5.1

Comment 1

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

Attachment: null-check GMP Parent Shmem messages from the Child to handle messages after shutdown

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8686994 [details] [diff] [review]
null-check GMP Parent Shmem messages from the Child to handle messages after shutdown

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

r+ with the same for GMPVideoDecoderParent, to be safe.

Comment 3

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

Attachment: null-check GMP Parent Shmem messages from the Child to handle messages after shutdown

Modified to match Decoder, which already had fixed this, plus cleanup

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8687016 [details] [diff] [review]
null-check GMP Parent Shmem messages from the Child to handle messages after shutdown

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

::: dom/media/gmp/GMPVideoEncoderParent.cpp
@@ +248,5 @@
>  {
>    LOGD(("%s::%s: %p (%d)", __CLASS__, __FUNCTION__, this, (int) aWhy));
>    mIsOpen = false;
>    mActorDestroyed = true;
> +

Adding whitespace here.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09dc1434c390

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09dc1434c390

Comment 7

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

Comment on attachment 8687016 [details] [diff] [review]
null-check GMP Parent Shmem messages from the Child to handle messages after shutdown

Approval Request Comment
[Feature/regressing bug #]: N/A
This bug was fixed for Decoders by Edwin back in January, as part of a larger changeset without any real explanation, but didn't realize Encoders had the same problem.

[User impact if declined]: Bad timing in a GMP Encoder plugin can crash the parent.  The crash is a null deref.  This appears to get hit with low frequency at random in the field; an update to openh264 we're trying to release was causing it to get hit 100% of the time on Android. 

[Describe test coverage new/current, TreeHerder]:  We can't easily test this in automation, though it's possible with a lot of work I think.

[Risks and why]: Low risk; it would be good to be able to push updated plugins without worrying about them crashing the browser

[String/UUID change made/needed]: none

Comment 8

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

We could verify this on Nightly before uplifting - Especially if our next beta build is next Monday. 

Randell, can you give some steps to reproduce the crash?

Ioana, am I right to needinfo you for verifications like this?

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8687016 [details] [diff] [review]
null-check GMP Parent Shmem messages from the Child to handle messages after shutdown

This change has been on Nightly for almost 2 weeks and fixes a potential crash on Fennec, seems like a good idea to uplift to Aurora.

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

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

Ritu - testing it requires a specific build of OpenH264 which has since been fixed, and can't be installed by hand generally; it has to be served by our update servers.  It was rarely possible to make happen on Desktop (it's a timing issue), but very very hard.  Other than cleaning up some duplicate functions, this really just adds a null-check before calling a method (and log if it's null and logging is on).  This was tested by hand on android with an openh264 build that was permacrashing on this.

Given the risk and nature of the fix, I think we should simply take it based on the risk and manual testing.

Comment 12

[Hank Peng]

The crashes reported from Windows and Mac OS X:
https://crash-stats.mozilla.com/report/index/3c006a80-1d19-4c84-a285-d45672151126
https://crash-stats.mozilla.com/report/index/8d4abc22-c47b-47b6-855d-d716a2151126

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Randell. Forwarding this to Liz

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/c8917edc6050

Comment 15

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8687016 [details] [diff] [review]
null-check GMP Parent Shmem messages from the Child to handle messages after shutdown

Crash fix, has had manual testing, let's uplift it to beta.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eda3d07d686c

Comment 17

[Paul Silaghi, QA [:pauly]]

based on comment 11