1321871 - Replace use of opens and bridges in GMP protocols with endpoints

Status: RESOLVED FIXED

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

Comment 1

[Andrew McCreight [:mccr8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73b56235914d2bb0719c5e5b8c07d3b188442b3

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1321871, part 1 - Replace use of opens and bridges in GMP protocols with endpoints.

This removes the open of PGMPContent from PGMP, the bridge of
PGMPService and PGMP from PGMPContent, and the spawn of PGMP from
PGMPService. I did these changes all at once because the way the
bridges works it was hard to split it up.

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

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Bug 1321871, part 2 - Remove PContent opens of PGMPService.

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

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 8824261 [details]
Bug 1321871, part 1 - Replace use of opens and bridges in GMP protocols with endpoints.

https://reviewboard.mozilla.org/r/102788/#review107520

I think after this patch the comment at http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/ipc/PContent.ipdl#27 can be removed. Could you check if you can change the order of those includes and remove the comment if so?
Marking r+ but I'd really like an answer about the sync/async difference between Bridge and InitGMPContentChild.

::: dom/media/gmp/GMPParent.cpp:1066
(Diff revision 1)
>      holder->Resolve(blocker, __func__);
>    }
>  }
>  
> -PGMPContentParent*
> -GMPParent::AllocPGMPContentParent(Transport* aTransport, ProcessId aOtherPid)
> +bool
> +GMPParent::OpenContent()

I'd name this OpenPGMPContent.

::: dom/media/gmp/GMPServiceParent.cpp:2013
(Diff revision 1)
> +    return IPC_OK();
> +  }
> +
> +  *aOutEndpoint = Move(parent);
> +
> +  if (!gmp->InitGMPContentChild(child)) {

Can't we call SendInitGMPContentChild directly here?

::: dom/media/gmp/PGMP.ipdl:39
(Diff revision 1)
>    async CrashPluginNow();
>    intr StartPlugin(nsString adapter);
>    async SetNodeId(nsCString nodeId);
>    async PreloadLibs(nsCString libs);
>    async CloseActive();
> +  async InitGMPContentChild(Endpoint<PGMPContentChild> endpoint);

Hmm, iirc Bridge was synchronous, but InitGMPContentChild is marked async here. Doesn't that cause any issues?

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 8824262 [details]
Bug 1321871, part 2 - Remove PContent opens of PGMPService.

https://reviewboard.mozilla.org/r/102790/#review107542

::: dom/media/gmp/GMPServiceChild.cpp:435
(Diff revision 1)
>  
>    UniquePtr<GMPServiceChild> serviceChild(new GMPServiceChild());
>  
>    nsCOMPtr<nsIThread> gmpThread;
> -  nsresult rv = gmp->GetThread(getter_AddRefs(gmpThread));
> -  NS_ENSURE_SUCCESS(rv, nullptr);
> +  if (NS_FAILED(gmp->GetThread(getter_AddRefs(gmpThread)))) {
> +    return;

Hmm, dropping these errors is ok?

::: dom/media/gmp/GMPServiceParent.cpp:2130
(Diff revision 1)
> -                           NS_DISPATCH_SYNC);
> +                                NS_DISPATCH_SYNC);
> -  if (NS_FAILED(rv) || !ok) {
> -    return nullptr;
> -  }
>  
> -  return serviceParent.forget();
> +  // XXX The ownership implicitly passes to the IPC code from here, or something.

Might be good to have better documentation of this. ActorDestroy deletes this object. I am a bit worried about ignoring the errors, because it does seem that'll lead us to just not cleaning up now.

Comment 6

[Andrew McCreight [:mccr8]]

I'm not sure about the sync/async thing. I'll try to look over the code and see if I can figure anything out.

I'll clean up the error handling. I'm not sure why I just randomly dropped it there. For some reason I had the idea that the patch was creating new points of failure, but it doesn't look like it.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Peter Van der Beken [:peterv] from comment #4)
> Could you check if you can change the order of those includes and
> remove the comment if so?

It looks like it works, so I'll remove the comment. Thanks for pointing that out. I did notice that comment before but it scared me so I just left it alone.

> I'd name this OpenPGMPContent.

Fixed.

> Can't we call SendInitGMPContentChild directly here?

I can call gmp->Send there directly, but I still have to call another method to increment the child count. I'll just do that. It is clearer.

> Hmm, iirc Bridge was synchronous, but InitGMPContentChild is marked async
> here. Doesn't that cause any issues?

I talked to Bill, and he said bridge uses async messages, which I confirmed, so I think my patch should be okay: http://searchfox.org/mozilla-central/source/ipc/glue/ProtocolUtils.cpp#124

Comment 8

[Andrew McCreight [:mccr8]]

For part 2, I remember now I couldn't figure out how to handle errors, but looking over it again now, I worked it out, so I've restored all the error handling (GMPServiceChild::Create, GMPServiceParent::Create, gmpThread->Dispatch). I think we just crash (before and after my patches) with an IPC error, but I guess it is better than silently ending in a weird state. I improved my XXX comment a little bit.

I'll do another try run before landing tomorrow, to make sure I didn't break anything.

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8824261 [details]
Bug 1321871, part 1 - Replace use of opens and bridges in GMP protocols with endpoints.

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

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8824262 [details]
Bug 1321871, part 2 - Remove PContent opens of PGMPService.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6cd79a1e5076
part 1 - Replace use of opens and bridges in GMP protocols with endpoints. r=peterv
https://hg.mozilla.org/integration/autoland/rev/28273e0dbbb7
part 2 - Remove PContent opens of PGMPService. r=peterv

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6cd79a1e5076
https://hg.mozilla.org/mozilla-central/rev/28273e0dbbb7