Fix GMP async shutdown

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
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.
Attachment #8853848 - Flags: review?(gsquelart) → review+

Comment 6

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

https://reviewboard.mozilla.org/r/125874/#review128372
Attachment #8853849 - Flags: review?(gsquelart) → review+

Comment 7

2 years ago
mozreview-review
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
Attachment #8853850 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 years ago
Pushed by cpearce@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2a84366d13af
https://hg.mozilla.org/mozilla-central/rev/11caedf0d4d3
https://hg.mozilla.org/mozilla-central/rev/52ae8008d066
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.