Bug 1364624 - Part 1: Manually manage locks in MessageChannel::Close. r=froydnj, a=RyanVM
authorEric Rahm <erahm@mozilla.com>
Thu, 26 Apr 2018 17:14:22 -0700
changeset 473331 9c083d037ce2528c1914c683902c0198afb7cd25
parent 473330 dee67f5780bf85a02e8ec5b40dac24391dcd018d
child 473332 7a8b8efc8098c0c6a9038e405c3175935e60340d
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj, RyanVM
bugs1364624
milestone61.0
Bug 1364624 - Part 1: Manually manage locks in MessageChannel::Close. r=froydnj, a=RyanVM This switches over to manually managing the locking in MessageChannel::Close in order to avoid a deadlock on msvc opt builds. It has the added benefit of avoid a superfluous lock/unlock pair.
ipc/glue/MessageChannel.cpp
--- a/ipc/glue/MessageChannel.cpp
+++ b/ipc/glue/MessageChannel.cpp
@@ -8,16 +8,17 @@
 #include "mozilla/ipc/MessageChannel.h"
 
 #include "mozilla/Assertions.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/dom/ScriptSettings.h"
 #include "mozilla/ipc/ProtocolUtils.h"
 #include "mozilla/Logging.h"
 #include "mozilla/Move.h"
+#include "mozilla/ScopeExit.h"
 #include "mozilla/Sprintf.h"
 #include "mozilla/Telemetry.h"
 #include "mozilla/TimeStamp.h"
 #include "nsAppRunner.h"
 #include "mozilla/UniquePtr.h"
 #include "nsAutoPtr.h"
 #include "nsDebug.h"
 #include "nsISupportsImpl.h"
@@ -2697,26 +2698,36 @@ MessageChannel::CloseWithTimeout()
 }
 
 void
 MessageChannel::Close()
 {
     AssertWorkerThread();
 
     {
-        MonitorAutoLock lock(*mMonitor);
+        // We don't use MonitorAutoLock here as that causes some sort of
+        // deadlock in the error/timeout-with-a-listener state below when
+        // compiling an optimized msvc build.
+        mMonitor->Lock();
+
+        // Instead just use a ScopeExit to manage the unlock.
+        RefPtr<RefCountedMonitor> monitor(mMonitor);
+        auto exit = MakeScopeExit([m = Move(monitor)] () {
+          m->Unlock();
+        });
 
         if (ChannelError == mChannelState || ChannelTimeout == mChannelState) {
             // See bug 538586: if the listener gets deleted while the
             // IO thread's NotifyChannelError event is still enqueued
             // and subsequently deletes us, then the error event will
             // also be deleted and the listener will never be notified
             // of the channel error.
             if (mListener) {
-                MonitorAutoUnlock unlock(*mMonitor);
+                exit.release(); // Explicitly unlocking, clear scope exit.
+                mMonitor->Unlock();
                 NotifyMaybeChannelError();
             }
             return;
         }
 
         if (ChannelOpening == mChannelState) {
             // SynchronouslyClose() waits for an ack from the other side, so
             // the opening sequence should complete before this returns.