1154513 - [EME] GMP crash crashes browser in Nightly

Status: RESOLVED FIXED

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

Description

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

STR:

* Load http://drmtest2.adobe.com/HTML5AAXSPlayer/2/mse-access/ in Nightly
* Press "Play"
* Create bool pref "media.gmp.plugin.crash" and set to true (or toggle to true)
* Browser crashes!

In my case, the pref already exists and was set to true, so I had to toggle it to false and true.

Comment 1

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

Nightly only, so P2.

Comment 2

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

I think this is the crash report I had:

https://crash-stats.mozilla.com/report/index/0897ed68-e6b5-4576-93cf-9eb012150414

Comment 3

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

Attachment: 1154513-no-messaging-if-not-open.patch

When shutting down, only try and send a message if the actor hasn't been destroyed (which would destroy the PGMPContentParent, destroying the MessageChannel with it).

Note: This 'if (mIsOpen)' test was removed by Edwin in bug 1072024, in a patch named "1072024-plug-leak.patch", so it is very possible that this new patch here reintroduces a leak -- To be tested thoroughly!
(Just not sure how this would leak, as the other side of the channel is probably very dead, so not sending a message shouldn't matter?!)

Alternatives would be for PGMPContentParent to notify channel users that the channel is dead; or to keep PGMPContentParent (and hence the channel) alive at least as long as all the channel users.

Comment 4

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

I could reproduce the original issue on both Windows and Mac when using a clearkey test page and crashing the plugin. The proposed patch fixes that as well.

Comment 5

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

I think I killed gtest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c383b917134f
Will investigate...

Comment 6

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

Comment on attachment 8593911 [details] [diff] [review]
1154513-no-messaging-if-not-open.patch

Cancelling r? for now.

Comment 7

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

I can repro the failure. We're spinning the event loop waiting for async shutdown to complete, in GeckoMediaPluginServiceParent::Observe() in the "profile-change-teardown" case, inside the loop:

    // Wait for plugins to do async shutdown...
    while (mWaitingForPluginsAsyncShutdown) {
      NS_ProcessNextEvent(NS_GetCurrentThread(), true);
    }

mWaitingForPluginsAsyncShutdown is never being unset.

We're also supposed to timeout the async shutdown after 3 seconds, and that's not happening...

Comment 8

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

Attachment: 1154513-no-messaging-if-actor-destroyed.patch

After further investigation, I think this patch is probably an acceptable solution.

Debugging shows the following sequence of calls:
0- GMPContentParent::GetGMPDecryptor() -- GMPDecryptorParent is created, mChannel given as raw pointer
 ** kill plugin here **
1- GMPDecryptorParent::ActorDestroy() -- synchronously calls GMPContentParent::DecryptorDestroyed
2- GMPContentParent::DecryptorDestroyed -- here GMPContentParent forgets about GMPDecryptorParent
3- GMPContentParent::ActorDestroy() -- no children left -> kill self
4- GMPContentParent::~GMPContentParent() -- destroys mChannel as well!
5- GMPDecryptorParent::Shutdown() -- calls SendDecryptingComplete(), which crashes because mChannel has been destroyed

We could try and rewire the whole GMP machinery so that GMPDecryptorParent::Shutdown is called *before* GMPContentParent (or just mChannel) is destroyed, but it seems much harder, and in any case it would not really help as messages could not get sent anyway.

The other option of notifying GMPDecryptorParent (that mChannel is dead) is not possible because GMPContentParent forgets about GMPDecryptorParent in step 2 above, some time before the channel destruction.


Try in progress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04a44047b3a7

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5707042f113b

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5707042f113b