Closed Bug 1316215 Opened 4 years ago Closed 4 years ago

Convert GMPService to MozPromise

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1316215 - Promisify GMPService GetGMPContentParent and GetServiceChild.
58 bytes, text/x-review-board-request
gerald
: review+
Details
Bug 1316215 - Block GMPContentParent close while a GMPService::GetContentParent is being processed.
58 bytes, text/x-review-board-request
gerald
: review+
Details
Bug 1316215 - Make GMPParent::IsUsed() take into account whether there are pending GetContentParent calls.
58 bytes, text/x-review-board-request
gerald
: review+
Details
Bug 1316215 - Merge SelectGMP and LaunchGMP into one synchronous IPC operation.
58 bytes, text/x-review-board-request
gerald
: review+
Details 
The GMPService should use MozPromises to make control flow and threading simpler. I also need this to help make the work in bug 1315850 easier.
Comment on attachment 8813883 [details]
Bug 1316215 - Promisify GMPService GetGMPContentParent and GetServiceChild.

https://reviewboard.mozilla.org/r/94930/#review95394

r+ as-is, unless my questions/suggestions unveil some needed changes:

::: dom/media/gmp/GMPParent.cpp:1077
(Diff revision 1)
> -  RefPtr<RunCreateContentParentCallbacks> runCallbacks =
> -    new RunCreateContentParentCallbacks(mGMPContentParent);
> +  RefPtr<nsIRunnable> task(NewRunnableMethod(this, &GMPParent::ResolveGetContentParentPromises));
> +  NS_DispatchToCurrentThread(task);

I can see how it made sense before to dispatch a task that would directly call all callbacks.

But MozPromise does all the necessary dispatching itself when it gets resolved. (Though I am not sure this is a guarantee of its API?)
So I think you could just inline ResolveGetContentParentPromises right here.

But if there is any doubt, it's fine to keep this dispatch, it's a small cost for a rare-enough operation.

::: dom/media/gmp/GMPParent.cpp:1099
(Diff revision 1)
>        if (!EnsureProcessLoaded() || !PGMPContent::Open(this)) {
> -        return false;
> +        MOZ_ASSERT(mGetContentParentPromises.Length() == 1);
> +        mGetContentParentPromises[0]->Reject(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__);
> +        return;

Should we empty mGetContentParentPromises? (To quickly release the promise holder we don't need anymore.)

I see it was not done before either.
Are you assuming that the rejection will get the GMPParent destroyed soon enough, along with its mGetContentParentPromises?

::: dom/media/gmp/GMPServiceChild.cpp:251
(Diff revision 1)
> +      UniquePtr<GetNodeIdCallback> callback(rawCallback);
> -    nsCString outId;
> +      nsCString outId;
> -    if (!aGMPServiceChild->SendGetGMPNodeId(mOrigin, mTopLevelOrigin,
> -                                            mGMPName,
> -                                            mInPrivateBrowsing, &outId)) {
> -      mCallback->Done(NS_ERROR_FAILURE, EmptyCString());
> +      if (!child->SendGetGMPNodeId(origin, topLevelOrigin,
> +                                   gmpName,
> +                                   pb, &outId)) {
> +        callback->Done(NS_ERROR_DOM_MEDIA_FATAL_ERR, EmptyCString());

Is this error-code change really related to this promisification patch?
(more below)
Attachment #8813883 - Flags: review?(gsquelart) → review+
Comment on attachment 8813884 [details]
Bug 1316215 - Block GMPContentParent close while a GMPService::GetContentParent is being processed.

https://reviewboard.mozilla.org/r/94932/#review95400

r+ with typos fixed:

::: dom/media/gmp/GMPContentParent.h:1
(Diff revision 1)
>  /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

In commit description:
- 'MozPromimse' -> 'MozPromise'
- 'closes it's' -> 'closes its'
Attachment #8813884 - Flags: review?(gsquelart) → review+
Comment on attachment 8813885 [details]
Bug 1316215 - Make GMPParent::IsUsed() take into account whether there are pending GetContentParent calls.

https://reviewboard.mozilla.org/r/94934/#review95402
Attachment #8813885 - Flags: review?(gsquelart) → review+
Comment on attachment 8813886 [details]
Bug 1316215 - Merge SelectGMP and LaunchGMP into one synchronous IPC operation.

https://reviewboard.mozilla.org/r/94936/#review95406

r+ with commit description fixed:

::: dom/media/gmp/GMPServiceChild.cpp:1
(Diff revision 1)
>  /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

At the end of the commit description: 'in Bug 1267918 to and', spurious 'to' or missing blurb?
Attachment #8813886 - Flags: review?(gsquelart) → review+
(In reply to Gerald Squelart [:gerald] from comment #8)
> Comment on attachment 8813883 [details]
> Bug 1316215 - Promisify GMPService GetGMPContentParent and GetServiceChild.
> 
> https://reviewboard.mozilla.org/r/94930/#review95394
> 
> r+ as-is, unless my questions/suggestions unveil some needed changes:
> 
> ::: dom/media/gmp/GMPParent.cpp:1077
> (Diff revision 1)
> > -  RefPtr<RunCreateContentParentCallbacks> runCallbacks =
> > -    new RunCreateContentParentCallbacks(mGMPContentParent);
> > +  RefPtr<nsIRunnable> task(NewRunnableMethod(this, &GMPParent::ResolveGetContentParentPromises));
> > +  NS_DispatchToCurrentThread(task);
> 
> I can see how it made sense before to dispatch a task that would directly
> call all callbacks.
> 
> But MozPromise does all the necessary dispatching itself when it gets
> resolved. (Though I am not sure this is a guarantee of its API?)
> So I think you could just inline ResolveGetContentParentPromises right here.
> 
> But if there is any doubt, it's fine to keep this dispatch, it's a small
> cost for a rare-enough operation.

I had just made it async as it was already, but yes, the promise resolve should make this async, so it's probably safe to just call it directly.


 
> ::: dom/media/gmp/GMPParent.cpp:1099
> (Diff revision 1)
> >        if (!EnsureProcessLoaded() || !PGMPContent::Open(this)) {
> > -        return false;
> > +        MOZ_ASSERT(mGetContentParentPromises.Length() == 1);
> > +        mGetContentParentPromises[0]->Reject(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__);
> > +        return;
> 
> Should we empty mGetContentParentPromises? (To quickly release the promise
> holder we don't need anymore.)
> 
> I see it was not done before either.
> Are you assuming that the rejection will get the GMPParent destroyed soon
> enough, along with its mGetContentParentPromises?

I had been assuming that the rejection would get the GMPParent destroyed. But it would make the code more robust if we rejected all the promises and free'd their holders, just to be sure. It shouldn't be possible for other promises to be added while we're loading a GMP, but it doesn't hurt to be careful.

> 
> ::: dom/media/gmp/GMPServiceChild.cpp:251
> (Diff revision 1)
> > +      UniquePtr<GetNodeIdCallback> callback(rawCallback);
> > -    nsCString outId;
> > +      nsCString outId;
> > -    if (!aGMPServiceChild->SendGetGMPNodeId(mOrigin, mTopLevelOrigin,
> > -                                            mGMPName,
> > -                                            mInPrivateBrowsing, &outId)) {
> > -      mCallback->Done(NS_ERROR_FAILURE, EmptyCString());
> > +      if (!child->SendGetGMPNodeId(origin, topLevelOrigin,
> > +                                   gmpName,
> > +                                   pb, &outId)) {
> > +        callback->Done(NS_ERROR_DOM_MEDIA_FATAL_ERR, EmptyCString());
> 
> Is this error-code change really related to this promisification patch?
> (more below)

hmmm... yeah, not strictly, I will revert them.
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb29571e8491
Promisify GMPService GetGMPContentParent and GetServiceChild. r=gerald
https://hg.mozilla.org/integration/autoland/rev/4be3169b9d02
Block GMPContentParent close while a GMPService::GetContentParent is being processed. r=gerald
https://hg.mozilla.org/integration/autoland/rev/efff1ff587e3
Make GMPParent::IsUsed() take into account whether there are pending GetContentParent calls. r=gerald
https://hg.mozilla.org/integration/autoland/rev/0390e2080381
Merge SelectGMP and LaunchGMP into one synchronous IPC operation. r=gerald
You need to log in before you can comment on or make changes to this bug.