Bug 1316215 - Merge SelectGMP and LaunchGMP into one synchronous IPC operation. r=gerald
authorChris Pearce <cpearce@mozilla.com>
Tue, 22 Nov 2016 14:17:59 +1300
changeset 324049 0390e208038169bbdfa832155bef6359af4775fa
parent 324048 efff1ff587e32ef436efe95ef3153b44ff0ab1a5
child 324050 e00b256c87a539e9f9ede45904b9043a9cc6b8f3
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewersgerald
bugs1316215, 1267918
milestone53.0a1
Bug 1316215 - Merge SelectGMP and LaunchGMP into one synchronous IPC operation. r=gerald We were seeing almost permaorange failures in the WebRTC H.264/GMP tests due to the GMP being shutdown in the parent process in between the content process performing an OOP select operation and then performing an OOP launch operation. That is, in GeckoMediaPluginServiceChild::GetContentParent() in between the SendSelectGMP completing and the SendLaunchGMP completing, the GMP would shutdown and so when the launch operation ran in the main process it would fail. The select and launch are seperate operations so that the crash handler can be reported to the content process and an association can be made in the content process between the plugin ID and the crash helper before we try to launch the GMP. This is so that if the GMP crashes on startup, we're ready to handle the crash. However it turns out that if the GMP crashes on startup, the crash report message comes in after another round of the event/IPC message loop. So we actually do have time in the content process to connect the crash helper after the launch fails. So in order to fix the problem of the GMP shutting down in between select and launch, we can partially revert the changes I made in Bug 1267918 to merge selecting and launching GMPs back into a single operation. MozReview-Commit-ID: 5n4T1Gqlvr3
dom/media/gmp/GMPServiceChild.cpp
dom/media/gmp/GMPServiceParent.cpp
dom/media/gmp/GMPServiceParent.h
dom/media/gmp/PGMPService.ipdl
--- a/dom/media/gmp/GMPServiceChild.cpp
+++ b/dom/media/gmp/GMPServiceChild.cpp
@@ -68,38 +68,42 @@ GeckoMediaPluginServiceChild::GetContent
   nsCString api(aAPI);
   nsTArray<nsCString> tags(aTags);
   RefPtr<GMPCrashHelper> helper(aHelper);
   RefPtr<GeckoMediaPluginServiceChild> self(this);
   GetServiceChild()->Then(thread, __func__,
     [self, nodeId, api, tags, helper, rawHolder](GMPServiceChild* child) {
       UniquePtr<MozPromiseHolder<GetGMPContentParentPromise>> holder(rawHolder);
       nsresult rv;
-      uint32_t pluginId = 0;
-      bool ok = child->SendSelectGMP(nodeId, api, tags, &pluginId, &rv);
-      if (!ok || rv == NS_ERROR_ILLEGAL_DURING_SHUTDOWN) {
-        holder->Reject(rv, __func__);
-        return;
-      }
-
-      if (helper) {
-        self->ConnectCrashHelper(pluginId, helper);
-      }
 
       nsTArray<base::ProcessId> alreadyBridgedTo;
       child->GetAlreadyBridgedTo(alreadyBridgedTo);
 
       base::ProcessId otherProcess;
       nsCString displayName;
-      ok = child->SendLaunchGMP(pluginId,
-                                alreadyBridgedTo,
-                                &otherProcess,
-                                &displayName,
-                                &rv);
-      if (!ok || rv == NS_ERROR_ILLEGAL_DURING_SHUTDOWN) {
+      uint32_t pluginId = 0;
+      bool ok = child->SendLaunchGMP(nodeId,
+                                     api,
+                                     tags,
+                                     alreadyBridgedTo,
+                                     &pluginId,
+                                     &otherProcess,
+                                     &displayName,
+                                     &rv);
+      if (helper && pluginId) {
+        // Note: Even if the launch failed, we need to connect the crash
+        // helper so that if the launch failed due to the plugin crashing,
+        // we can report the crash via the crash reporter. The crash
+        // handling notification will arrive shortly if the launch failed
+        // due to the plugin crashing.
+        self->ConnectCrashHelper(pluginId, helper);
+      }
+
+      if (!ok || NS_FAILED(rv)) {
+        LOGD(("GeckoMediaPluginServiceChild::GetContentParent SendLaunchGMP failed rv=%d", rv));
         holder->Reject(rv, __func__);
         return;
       }
 
       RefPtr<GMPContentParent> parent;
       child->GetBridgedGMPContentParent(otherProcess, getter_AddRefs(parent));
       if (!alreadyBridgedTo.Contains(otherProcess)) {
         parent->SetDisplayName(displayName);
--- a/dom/media/gmp/GMPServiceParent.cpp
+++ b/dom/media/gmp/GMPServiceParent.cpp
@@ -1962,69 +1962,52 @@ GeckoMediaPluginServiceParent::GetById(u
 GMPServiceParent::~GMPServiceParent()
 {
   NS_DispatchToMainThread(
     NewRunnableMethod(mService.get(),
                       &GeckoMediaPluginServiceParent::ServiceUserDestroyed));
 }
 
 mozilla::ipc::IPCResult
-GMPServiceParent::RecvSelectGMP(const nsCString& aNodeId,
+GMPServiceParent::RecvLaunchGMP(const nsCString& aNodeId,
                                 const nsCString& aAPI,
                                 nsTArray<nsCString>&& aTags,
+                                nsTArray<ProcessId>&& aAlreadyBridgedTo,
                                 uint32_t* aOutPluginId,
+                                ProcessId* aOutProcessId,
+                                nsCString* aOutDisplayName,
                                 nsresult* aOutRv)
 {
   if (mService->IsShuttingDown()) {
     *aOutRv = NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
     return IPC_OK();
   }
 
   RefPtr<GMPParent> gmp = mService->SelectPluginForAPI(aNodeId, aAPI, aTags);
   if (gmp) {
     *aOutPluginId = gmp->GetPluginId();
-    *aOutRv = NS_OK;
   } else {
     *aOutRv = NS_ERROR_FAILURE;
-  }
-
-  nsCString api = aTags[0];
-  LOGD(("%s: %p returning %p for api %s", __FUNCTION__, (void *)this, (void *)gmp, api.get()));
-
-  return IPC_OK();
-}
-
-mozilla::ipc::IPCResult
-GMPServiceParent::RecvLaunchGMP(const uint32_t& aPluginId,
-                                nsTArray<ProcessId>&& aAlreadyBridgedTo,
-                                ProcessId* aOutProcessId,
-                                nsCString* aOutDisplayName,
-                                nsresult* aOutRv)
-{
-  *aOutRv = NS_OK;
-  if (mService->IsShuttingDown()) {
-    *aOutRv = NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
-    return IPC_OK();
-  }
-
-  RefPtr<GMPParent> gmp(mService->GetById(aPluginId));
-  if (!gmp) {
-    *aOutRv = NS_ERROR_FAILURE;
+    *aOutPluginId = 0;
     return IPC_OK();
   }
 
   if (!gmp->EnsureProcessLoaded(aOutProcessId)) {
-    return IPC_FAIL_NO_REASON(this);
+    *aOutRv = NS_ERROR_FAILURE;
+    return IPC_OK();
   }
 
   *aOutDisplayName = gmp->GetDisplayName();
 
   if (!(aAlreadyBridgedTo.Contains(*aOutProcessId) || gmp->Bridge(this))) {
-    return IPC_FAIL_NO_REASON(this);
+    *aOutRv = NS_ERROR_FAILURE;
+    return IPC_OK();
   }
+
+  *aOutRv = NS_OK;
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult
 GMPServiceParent::RecvGetGMPNodeId(const nsString& aOrigin,
                                    const nsString& aTopLevelOrigin,
                                    const nsString& aGMPName,
                                    const bool& aInPrivateBrowsing,
--- a/dom/media/gmp/GMPServiceParent.h
+++ b/dom/media/gmp/GMPServiceParent.h
@@ -251,24 +251,21 @@ public:
                                            const nsString& aTopLevelOrigin,
                                            const nsString& aGMPName,
                                            const bool& aInPrivateBrowsing,
                                            nsCString* aID) override;
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
   static PGMPServiceParent* Create(Transport* aTransport, ProcessId aOtherPid);
 
-  mozilla::ipc::IPCResult RecvSelectGMP(const nsCString& aNodeId,
+  mozilla::ipc::IPCResult RecvLaunchGMP(const nsCString& aNodeId,
                                         const nsCString& aAPI,
                                         nsTArray<nsCString>&& aTags,
+                                        nsTArray<ProcessId>&& aAlreadyBridgedTo,
                                         uint32_t* aOutPluginId,
-                                        nsresult* aOutRv) override;
-
-  mozilla::ipc::IPCResult RecvLaunchGMP(const uint32_t& aPluginId,
-                                        nsTArray<ProcessId>&& aAlreadyBridgedTo,
                                         ProcessId* aOutID,
                                         nsCString* aOutDisplayName,
                                         nsresult* aOutRv) override;
 
 private:
   void CloseTransport(Monitor* aSyncMonitor, bool* aCompleted);
 
   RefPtr<GeckoMediaPluginServiceParent> mService;
--- a/dom/media/gmp/PGMPService.ipdl
+++ b/dom/media/gmp/PGMPService.ipdl
@@ -11,21 +11,21 @@ namespace mozilla {
 namespace gmp {
 
 sync protocol PGMPService
 {
   parent spawns PGMP as child;
 
 parent:
 
-  sync SelectGMP(nsCString nodeId, nsCString api, nsCString[] tags)
-    returns (uint32_t pluginId, nsresult aResult);
-
-  sync LaunchGMP(uint32_t pluginId, ProcessId[] alreadyBridgedTo)
-    returns (ProcessId id, nsCString displayName, nsresult aResult);
+  sync LaunchGMP(nsCString nodeId,
+                 nsCString api,
+                 nsCString[] tags,
+                 ProcessId[] alreadyBridgedTo)
+    returns (uint32_t pluginId, ProcessId id, nsCString displayName, nsresult aResult);
 
   sync GetGMPNodeId(nsString origin, nsString topLevelOrigin,
                     nsString gmpName,
                     bool inPrivateBrowsing)
     returns (nsCString id);
 };
 
 } // namespace gmp