Bug 1550771 - Deadlock in SharedMemoryBasic_mach triggered by AV1 playback r=jld a=jcristau
authorHaik Aftandilian <haftandilian@mozilla.com>
Wed, 22 May 2019 01:33:46 +0000
changeset 536452 3b40f41fc5bef54def5ea1d0ccc0727f142dfdbd
parent 536451 daa553c05384776bc8c79f5ac4e5f3570d91a320
child 536453 d6c54c50418ca7f3b4714a13a97455540e963785
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjld, jcristau
bugs1550771
milestone68.0
Bug 1550771 - Deadlock in SharedMemoryBasic_mach triggered by AV1 playback r=jld a=jcristau Don't hold gMutex when calling HandleSharePortsMessage() from PortServerThread to avoid deadlock. Differential Revision: https://phabricator.services.mozilla.com/D31694
ipc/glue/SharedMemoryBasic_mach.mm
--- a/ipc/glue/SharedMemoryBasic_mach.mm
+++ b/ipc/glue/SharedMemoryBasic_mach.mm
@@ -227,18 +227,22 @@ MemoryPorts* GetMemoryPortsForPid(pid_t 
     SetupMachMemory(pid, ports_in_receiver, ports_in_sender, ports_out_sender, ports_out_receiver,
                     false);
     MOZ_ASSERT(gMemoryCommPorts.find(pid) != gMemoryCommPorts.end());
   }
   return &gMemoryCommPorts.at(pid);
 }
 
 // We just received a port representing a region of shared memory, reply to
-// the process that set it with the mach_port_t that represents it in this process.
-// That will be the Handle to be shared over normal IPC
+// the process that set it with the mach_port_t that represents it in this
+// process. That will be the Handle to be shared over normal IPC.
+//
+// WARNING: this function is called while gMutex is not held and must not
+// reference structures protected by gMutex. See the deadlock warning in
+// ShareToProcess().
 void HandleSharePortsMessage(MachReceiveMessage* rmsg, MemoryPorts* ports) {
   mach_port_t port = rmsg->GetTranslatedPort(0);
   uint64_t* serial = reinterpret_cast<uint64_t*>(rmsg->GetData());
   MachSendMessage msg(kReturnIdMsg);
   // Construct the reply message, echoing the serial, and adding the port
   SharePortsReply replydata;
   replydata.port = port;
   replydata.serial = *serial;
@@ -347,40 +351,60 @@ static void* PortServerThread(void* argu
       delete ports;
       return nullptr;
     }
     if (rmsg.GetMessageID() == kWaitForTexturesMsg) {
       layers::TextureSync::HandleWaitForTexturesMessage(&rmsg, ports);
     } else if (rmsg.GetMessageID() == kUpdateTextureLocksMsg) {
       layers::TextureSync::DispatchCheckTexturesForUnlock();
     } else {
-      StaticMutexAutoLock smal(gMutex);
       switch (rmsg.GetMessageID()) {
-        case kSharePortsMsg:
+        case kSharePortsMsg: {
+          // Don't acquire gMutex here while calling HandleSharePortsMessage()
+          // to avoid deadlock. If gMutex is held by ShareToProcess(), we will
+          // block and create the following deadlock chain.
+          //
+          // 1) local:PortServerThread() blocked on local:gMutex held by
+          // 2) local:ShareToProcess() waiting for reply from
+          // 3) peer:PortServerThread() blocked on peer:gMutex held by
+          // 4) peer:ShareToProcess() waiting for reply from 1.
+          //
+          // It's safe to call HandleSharePortsMessage() without gMutex
+          // because HandleSharePortsMessage() only sends an outgoing message
+          // without referencing data structures protected by gMutex. The
+          // |ports| struct is deallocated on this thread in the kShutdownMsg
+          // message handling before this thread exits.
           HandleSharePortsMessage(&rmsg, ports);
           break;
-        case kGetPortsMsg:
+        }
+        case kGetPortsMsg: {
+          StaticMutexAutoLock smal(gMutex);
           HandleGetPortsMessage(&rmsg, ports);
           break;
-        case kCleanupMsg:
+        }
+        case kCleanupMsg: {
+          StaticMutexAutoLock smal(gMutex);
           if (gParentPid == 0) {
             LOG_ERROR("Cleanup message not valid for parent process");
             continue;
           }
 
           pid_t* pid;
           if (rmsg.GetDataLength() != sizeof(pid_t)) {
             LOG_ERROR("Improperly formatted message\n");
             continue;
           }
           pid = reinterpret_cast<pid_t*>(rmsg.GetData());
           SharedMemoryBasic::CleanupForPid(*pid);
           break;
-        default:
+        }
+        default: {
+          // gMutex not required
           LOG_ERROR("Unknown message\n");
+        }
       }
     }
   }
 }
 
 void SharedMemoryBasic::SetupMachMemory(pid_t pid, ReceivePort* listen_port,
                                         MachPortSender* listen_port_ack, MachPortSender* send_port,
                                         ReceivePort* send_port_ack, bool pidIsParent) {