Closed Bug 1352924 Opened 7 years ago Closed 7 years ago

Fix GMP async shutdown

Categories

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

Product:
Core
Component:
Audio/Video: GMP
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(3 files)

Bug 1352924 - Keep list of GMPServiceParents in GeckoMediaPluginServiceParent.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1352924 - Block creation of new GMPs once parent process begins shutdown.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1352924 - Disconnect GMP service in child from parent once all GMPs are shutdown.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details 
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 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 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 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+
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.
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: