Closed Bug 1224442 Opened 4 years ago Closed 4 years ago

Armor GMP Parent against late Shmem messages from the child

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- wontfix
firefox43 --- fixed
firefox44 --- fixed
firefox45 --- fixed
firefox-esr38 --- wontfix
b2g-v2.5 --- fixed

People

(Reporter: jesup, Assigned: jesup)

Details

(Keywords: crash, csectype-nullptr)

Attachments

(1 file, 1 obsolete file)

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
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
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.
Attachment #8686994 - Flags: review?(cpearce) → review+
Modified to match Decoder, which already had fixed this, plus cleanup
Attachment #8687016 - Flags: review?(cpearce)
Attachment #8686994 - Attachment is obsolete: true
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.
Attachment #8687016 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/09dc1434c390
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Summary: Armor GMP Parent against last Shmem messages from the child → Armor GMP Parent against late Shmem messages from the child
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
Attachment #8687016 - Flags: approval-mozilla-beta?
Attachment #8687016 - Flags: approval-mozilla-aurora?
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?
Flags: qe-verify+
Flags: needinfo?(rjesup)
Flags: needinfo?(ioana.chiorean)
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.
Attachment #8687016 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(rjesup) → needinfo?(rkothari)
Thanks Randell. Forwarding this to Liz
Flags: needinfo?(rkothari) → needinfo?(lhenry)
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.
Flags: needinfo?(lhenry)
Attachment #8687016 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
based on comment 11
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(ioana.chiorean)
You need to log in before you can comment on or make changes to this bug.