Bug 1364624 - Part 1: Manually manage locks in MessageChannel::Close. r=froydnj, a=jcristau
authorEric Rahm <erahm@mozilla.com>
Thu, 26 Apr 2018 17:14:22 -0700
changeset 802212 539d7238171dd4dd23246877deed0126ec544901
parent 802211 3f72cc97499ccc2896140b842256b119f5361f9c
child 802213 54937944b92295a04f8a7d661301c867bb4b303a
push id111850
push userbmo:tom@mozilla.com
push dateThu, 31 May 2018 16:41:37 +0000
reviewersfroydnj, jcristau
bugs1364624
milestone60.0.2
Bug 1364624 - Part 1: Manually manage locks in MessageChannel::Close. r=froydnj, a=jcristau 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"
@@ -2699,26 +2700,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.