1352924 - Fix GMP async shutdown

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, enhancement, P3)

Description

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

As outlined here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1313258#c28

GMP async shutdown doesn't work properly. We added code in bug 1279612 to make the GMP service block async shutdown until the IPC connection to the GMPServiceChild in the content processes are shutdown, but the IPC connections don't shutdown until the GMP service in the child process is destroyed during a later phase during shutdown.

So I think what we should do is when the parent process begins to shutdown we should send a message to the GMPServices in the content processes telling them to neuter themselves and shutdown their IPC connection when all their connections to their GMPs have closed. The GeckoMediaPluginServiceChild objects would then still exist in the content process, but most of their methods would fail, and their IPC connection would be closed and not be able to be re-established, and thus un-block the parent process so it can shut down cleanly.

The existing GMPService shutdown blocker would then work as expected; the GMPService in the parent process could shut down when all the IPC connections to the child process are shut down.

Comment 1

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

Attachment: Bug 1352924 - Keep list of GMPServiceParents in GeckoMediaPluginServiceParent.

This will allow us to broadcast a notification to the GMPServices running in
the content processes when they need to shutdown.

Review commit: https://reviewboard.mozilla.org/r/125872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125872/

Comment 2

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

Attachment: Bug 1352924 - Block creation of new GMPs once parent process begins shutdown.

This ensures that when we've started shutdown we don't try to start up new
GMPs. Doing so would create more connections from the content process to the
main process, and the main process can't shutdown until all such connections
are shut down.

Review commit: https://reviewboard.mozilla.org/r/125874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125874/

Comment 3

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

Attachment: Bug 1352924 - Disconnect GMP service in child from parent once all GMPs are shutdown.

This ensures that the IPC connection from the content process to the main
process is shut down as soon as possible. Once all the IPC connections are
closed, the main process removes its async shutdown blocker, and Firefox
can shutdown.

Review commit: https://reviewboard.mozilla.org/r/125876/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125876/

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad0e51e32e4a5e7d69a44ee6f423d842a032acde&selectedJob=88220828

Comment 5

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

Comment on attachment 8853848 [details]
Bug 1352924 - Keep list of GMPServiceParents in GeckoMediaPluginServiceParent.

https://reviewboard.mozilla.org/r/125872/#review128360

r+ provided you fixed the race, or can prove it won't happen.

::: dom/media/gmp/GMPServiceParent.h:211
(Diff revision 1)
>    bool mLoadPluginsFromDiskComplete;
>  
>    // Hashes nodeId to the hashtable of storage for that nodeId.
>    nsRefPtrHashtable<nsCStringHashKey, GMPStorage> mTempGMPStorage;
>  
> -  // Tracks how many users are running (on the GMP thread). Only when this count
> +  // Tracks how many IPC connections to GMPServices running content processes

"running *in* content processes"?

::: dom/media/gmp/GMPServiceParent.cpp:1706
(Diff revision 1)
> +  nsCOMPtr<nsIRunnable> task = NewRunnableMethod<GMPServiceParent*>(
> +    "GeckoMediaPluginServiceParent::ServiceUserCreated",
> +    mService.get(),
> +    &GeckoMediaPluginServiceParent::ServiceUserCreated,
> +    this);
> +  mService->MainThread()->Dispatch(task.forget());

Potential race, I think, if this constructor is not called on the main thread:

GMPServiceParent is ref-counted, we are in the constructor, so count=0.
NewRunnable<GMPServiceParent*> will automatically store it into a RefPtr, doing an AddRef, now count=1.
In the worst case, the task could run to completion before the constructor ends or the pointer is stored into its first RefPtr/already_AddRefed; This would bring the count down to 0, and destroy the object!

If you want to bypass the RefPtr storage, do `NewRunnableMethod<StorePtrPassByPtr<GMPServiceParent>>(...)`.
But then, there's the risk that GMPServiceParent could be destroyed before/when the task runs!

Another solution would be to dispatch an intermediate task to the current queue (so the construction will finish, and the object will be correctly stored) and from there dispatch the task calling ServiceUserCreated on the main thread.

::: dom/media/gmp/GMPServiceParent.cpp:1716
(Diff revision 1)
> +  mService->MainThread()->Dispatch(task.forget());
> +}
> +
>  GMPServiceParent::~GMPServiceParent()
>  {
> -  nsCOMPtr<nsIRunnable> task = NewRunnableMethod(
> +  nsCOMPtr<nsIRunnable> task = NewRunnableMethod<GMPServiceParent*>(

You'll definitely want to do `NewRunnableMethod<StorePtrPassByPtr<GMPServiceParent>>(...)` here, so you don't touch the ref count.

Comment 6

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

Comment on attachment 8853849 [details]
Bug 1352924 - Block creation of new GMPs once parent process begins shutdown.

https://reviewboard.mozilla.org/r/125874/#review128372

Comment 7

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

Comment on attachment 8853850 [details]
Bug 1352924 - Disconnect GMP service in child from parent once all GMPs are shutdown.

https://reviewboard.mozilla.org/r/125876/#review128374

Comment 8

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

Comment on attachment 8853848 [details]
Bug 1352924 - Keep list of GMPServiceParents in GeckoMediaPluginServiceParent.

https://reviewboard.mozilla.org/r/125872/#review128360

> Potential race, I think, if this constructor is not called on the main thread:
> 
> GMPServiceParent is ref-counted, we are in the constructor, so count=0.
> NewRunnable<GMPServiceParent*> will automatically store it into a RefPtr, doing an AddRef, now count=1.
> In the worst case, the task could run to completion before the constructor ends or the pointer is stored into its first RefPtr/already_AddRefed; This would bring the count down to 0, and destroy the object!
> 
> If you want to bypass the RefPtr storage, do `NewRunnableMethod<StorePtrPassByPtr<GMPServiceParent>>(...)`.
> But then, there's the risk that GMPServiceParent could be destroyed before/when the task runs!
> 
> Another solution would be to dispatch an intermediate task to the current queue (so the construction will finish, and the object will be correctly stored) and from there dispatch the task calling ServiceUserCreated on the main thread.

Thanks for the review. This was a thoughful comment.

While GeckoMediaPluginServiceParent is refcounted, it turns out that GMPServiceParent (the IPC actor) is in fact not refcounted; it's allocated and stored in an nsAutoPtr here:
https://dxr.mozilla.org/mozilla-central/rev/38894655c89e68bcd8f45d31a0d3005f2c2b53db/dom/media/gmp/GMPServiceParent.cpp#1896

And I also discovered while verifying this, that we actually run the constructor on the main thread, as this is created from a PContent IPC call, and that runs on the main thread. However the destructor runs on a non-main thread in a runnable dispatched in  GMPServiceParent::ActorDestroy() to either the GMP thread or the IPC MessageLoop thread, but I'm not 100% sure.

Comment 9

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

Comment on attachment 8853848 [details]
Bug 1352924 - Keep list of GMPServiceParents in GeckoMediaPluginServiceParent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125872/diff/1-2/

Comment 10

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

Comment on attachment 8853849 [details]
Bug 1352924 - Block creation of new GMPs once parent process begins shutdown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125874/diff/1-2/

Comment 11

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

Comment on attachment 8853850 [details]
Bug 1352924 - Disconnect GMP service in child from parent once all GMPs are shutdown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125876/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a84366d13af
Keep list of GMPServiceParents in GeckoMediaPluginServiceParent. r=gerald
https://hg.mozilla.org/integration/autoland/rev/11caedf0d4d3
Block creation of new GMPs once parent process begins shutdown. r=gerald
https://hg.mozilla.org/integration/autoland/rev/52ae8008d066
Disconnect GMP service in child from parent once all GMPs are shutdown. r=gerald

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a84366d13af
https://hg.mozilla.org/mozilla-central/rev/11caedf0d4d3
https://hg.mozilla.org/mozilla-central/rev/52ae8008d066