Closed Bug 1321871 Opened 8 years ago Closed 7 years ago

Replace use of opens and bridges in GMP protocols with endpoints

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(2 files)

Bug 1321871, part 1 - Replace use of opens and bridges in GMP protocols with endpoints.
59 bytes, text/x-review-board-request
peterv
: review+
Details
Bug 1321871, part 2 - Remove PContent opens of PGMPService.
59 bytes, text/x-review-board-request
peterv
: review+
Details 
      No description provided.
Summary: Replace use of opens and bridges in PGMPContent with endpoints → Replace use of opens and bridges in PGMPContent and PGMPService with endpoints
Summary: Replace use of opens and bridges in PGMPContent and PGMPService with endpoints → Replace use of opens and bridges in GMP protocols with endpoints
Rank: 35
Priority: -- → P3
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?
Attachment #8824261 - Flags: review?(peterv) → review+
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.
Attachment #8824262 - Flags: review?(peterv) → review+
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.
(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
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.
Pushed by amccreight@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/6cd79a1e5076
https://hg.mozilla.org/mozilla-central/rev/28273e0dbbb7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: