Bug 1224825 - Race condition in MessagePort::close, r=smaug

dom/messagechannel/MessagePort.cpp

@@ -97,6 +97,26 @@ public:
 
   NS_IMETHOD
   Run() override
+  {
+    nsresult rv = SendEvent();
+    NS_WARN_IF(NS_FAILED(rv));
+
+    mPort->MaybeClose();
+
+    return NS_OK;
+  }
+
+  NS_IMETHOD
+  Cancel() override
+  {
+    mPort = nullptr;
+    mData = nullptr;
+    return NS_OK;
+  }
+
+private:
+  nsresult
+  SendEvent()
   {
     nsCOMPtr<nsIGlobalObject> globalObject;
 
@@ -152,15 +172,6 @@ public:
     return NS_OK;
   }
 
-  NS_IMETHOD
-  Cancel() override
-  {
-    mPort = nullptr;
-    mData = nullptr;
-    return NS_OK;
-  }
-
-private:
   ~PostMessageRunnable()
   {}
 
@@ -281,6 +292,7 @@ MessagePort::MessagePort(nsPIDOMWindow* aWindow)
   , mInnerID(0)
   , mMessageQueueEnabled(false)
   , mIsKeptAlive(false)
+  , mClosing(false)
 {
   mIdentifier = new MessagePortIdentifier();
   mIdentifier->neutered() = true;
@@ -457,6 +469,12 @@ MessagePort::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
     return;
   }
 
+  // Just processing pending messages, then closing. This message must be
+  // ignored.
+  if (mClosing) {
+    return;
+  }
+
   RemoveDocFromBFCache();
 
   // Not entangled yet.
@@ -534,6 +552,12 @@ MessagePort::Close()
     return;
   }
 
+  // If we have some messages to send, we cannot close this port immediatelly.
+  if (!mMessages.IsEmpty()) {
+    mClosing = true;
+    return;
+  }
+
   // We don't care about stopping the sending of messages because from now all
   // the incoming messages will be ignored.
   mState = eStateDisentangled;
@@ -636,6 +660,7 @@ void
 MessagePort::MessagesReceived(nsTArray<MessagePortMessage>& aMessages)
 {
   MOZ_ASSERT(mState == eStateEntangled || mState == eStateDisentangling);
+  // We cannot have a next-step because this must be processed by Entangled().
   MOZ_ASSERT(mNextStep == eNextStepNone);
   MOZ_ASSERT(mMessagesForTheOtherPort.IsEmpty());
 
@@ -704,6 +729,11 @@ MessagePort::CloneAndDisentangle(MessagePortIdentifier& aIdentifier)
     return;
   }
 
+  // If Close() has been called.
+  if (mClosing) {
+    return;
+  }
+
   aIdentifier.uuid() = mIdentifier->uuid();
   aIdentifier.destinationUuid() = mIdentifier->destinationUuid();
   aIdentifier.sequenceId() = mIdentifier->sequenceId() + 1;
@@ -896,5 +926,14 @@ MessagePort::ForceClose(const MessagePortIdentifier& aIdentifier)
   ForceCloseHelper::ForceClose(aIdentifier);
 }
 
+void
+MessagePort::MaybeClose()
+{
+  // All the other checks are done in Close():
+  if (mClosing && mMessages.IsEmpty()) {
+    Close();
+  }
+}
+
 } // namespace dom
 } // namespace mozilla

dom/messagechannel/MessagePort.h

@@ -25,6 +25,7 @@ class DispatchEventRunnable;
 class MessagePortChild;
 class MessagePortIdentifier;
 class MessagePortMessage;
+class PostMessageRunnable;
 class SharedMessagePortMessage;
 
 namespace workers {
@@ -36,6 +37,7 @@ class MessagePort final : public DOMEventTargetHelper
                         , public nsIObserver
 {
   friend class DispatchEventRunnable;
+  friend class PostMessageRunnable;
 
 public:
   NS_DECL_NSIIPCBACKGROUNDCHILDCREATECALLBACK
@@ -148,6 +150,8 @@ private:
     return mIsKeptAlive;
   }
 
+  void MaybeClose();
+
   nsAutoPtr<workers::WorkerFeature> mWorkerFeature;
 
   RefPtr<DispatchEventRunnable> mDispatchRunnable;
@@ -176,6 +180,7 @@ private:
   bool mMessageQueueEnabled;
 
   bool mIsKeptAlive;
+  bool mClosing;
 };
 
 } // namespace dom

dom/messagechannel/MessagePortService.cpp

@@ -36,6 +36,9 @@ public:
     : mDestinationUUID(aDestinationUUID)
     , mSequenceID(1)
     , mParent(nullptr)
+    // By default we don't know the next parent.
+    , mWaitingForNewParent(true)
+    , mNextStepCloseAll(false)
   {
     MOZ_COUNT_CTOR(MessagePortServiceData);
   }
@@ -62,6 +65,9 @@ public:
 
   FallibleTArray<NextParent> mNextParents;
   FallibleTArray<RefPtr<SharedMessagePortMessage>> mMessages;
+
+  bool mWaitingForNewParent;
+  bool mNextStepCloseAll;
 };
 
 /* static */ MessagePortService*
@@ -113,31 +119,49 @@ MessagePortService::RequestEntangling(MessagePortParent* aParent,
   // This is a security check.
   if (!data->mDestinationUUID.Equals(aDestinationUUID)) {
     MOZ_ASSERT(false, "DestinationUUIDs do not match!");
+    CloseAll(aParent->ID());
     return false;
   }
 
   if (aSequenceID < data->mSequenceID) {
     MOZ_ASSERT(false, "Invalid sequence ID!");
+    CloseAll(aParent->ID());
     return false;
   }
 
   if (aSequenceID == data->mSequenceID) {
     if (data->mParent) {
       MOZ_ASSERT(false, "Two ports cannot have the same sequenceID.");
+      CloseAll(aParent->ID());
       return false;
     }
 
     // We activate this port, sending all the messages.
     data->mParent = aParent;
+    data->mWaitingForNewParent = false;
     FallibleTArray<MessagePortMessage> array;
     if (!SharedMessagePortMessage::FromSharedToMessagesParent(aParent,
                                                               data->mMessages,
                                                               array)) {
+      CloseAll(aParent->ID());
       return false;
     }
 
     data->mMessages.Clear();
-    return aParent->Entangled(array);
+
+    // We can entangle the port.
+    if (!aParent->Entangled(array)) {
+      CloseAll(aParent->ID());
+      return false;
+    }
+
+    // If we were waiting for this parent in order to close this channel, this
+    // is the time to do it.
+    if (data->mNextStepCloseAll) {
+      CloseAll(aParent->ID());
+    }
+
+    return true;
   }
 
   // This new parent will be the next one when a Disentangle request is
@@ -145,6 +169,7 @@ MessagePortService::RequestEntangling(MessagePortParent* aParent,
   MessagePortServiceData::NextParent* nextParent =
     data->mNextParents.AppendElement(mozilla::fallible);
   if (!nextParent) {
+    CloseAll(aParent->ID());
     return false;
   }
 
@@ -193,6 +218,7 @@ MessagePortService::DisentanglePort(
   // We didn't find the parent.
   if (!nextParent) {
     data->mMessages.SwapElements(aMessages);
+    data->mWaitingForNewParent = true;
     data->mParent = nullptr;
     return true;
   }
@@ -267,6 +293,20 @@ MessagePortService::CloseAll(const nsID& aUUID)
   }
 
   nsID destinationUUID = data->mDestinationUUID;
+
+  // If we have informations about the other port and that port has some
+  // pending messages to deliver but the parent has not processed them yet,
+  // because its entangling request didn't arrive yet), we cannot close this
+  // channel.
+  MessagePortServiceData* destinationData;
+  if (mPorts.Get(destinationUUID, &destinationData) &&
+      !destinationData->mMessages.IsEmpty() &&
+      destinationData->mWaitingForNewParent) {
+    MOZ_ASSERT(!destinationData->mNextStepCloseAll);
+    destinationData->mNextStepCloseAll = true;
+    return;
+  }
+
   mPorts.Remove(aUUID);
 
   CloseAll(destinationUUID);

dom/messagechannel/tests/mochitest.ini

@@ -24,3 +24,4 @@ support-files =
 [test_messageChannel_any.html]
 [test_messageChannel_forceClose.html]
 [test_messageChannel_bug1178076.html]
+[test_messageChannel_bug1224825.html]

dom/messagechannel/tests/test_messageChannel_bug1224825.html

@@ -0,0 +1,58 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1224825
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1224825</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1224825">Mozilla Bug 1224825</a>
+<div id="content"></div>
+<pre id="test">
+</pre>
+  <script type="application/javascript">
+
+  var MAX = 100;
+
+  function runTest() {
+    var worker = new Worker('data:javascript,onmessage = function(e) { e.ports[0].onmessage = function(evt) { postMessage(evt.data);}}');
+
+    var count = 0;
+    worker.onmessage = function(e) {
+      is(e.data, count, "Correct value expected!");
+      ok(count < MAX,"No count > MAX messages!");
+      if (++count == MAX) {
+
+        SimpleTest.requestFlakyTimeout("Testing an event not happening");
+        setTimeout(function() {
+          SimpleTest.finish();
+        }, 200);
+
+        info("All the messages correctly received");
+      }
+    }
+
+    var mc = new MessageChannel();
+    worker.postMessage(42, [mc.port2]);
+
+    for (var i = 0; i < MAX; ++i) {
+      mc.port1.postMessage(i);
+    }
+
+    mc.port1.close();
+
+    for (var i = 0; i < MAX * 2; ++i) {
+      mc.port1.postMessage(i);
+    }
+
+  }
+
+  SimpleTest.waitForExplicitFinish();
+  runTest();
+  </script>
+</body>
+</html>