Bug 1426467: Re-enqueue messages from workers when debugger pause ends; don't run them immediately. r=bkelly

MozReview-Commit-ID: 1Yyjqz5S6tZ

devtools/server/tests/mochitest/chrome.ini

@@ -24,9 +24,13 @@ support-files =
   large-image.jpg
   memory-helpers.js
   nonchrome_unsafeDereference.html
+  suspendTimeouts_content.html
+  suspendTimeouts_content.js
+  suspendTimeouts_worker.js
   small-image.gif
   setup-in-child.js
   setup-in-parent.js
+  test_suspendTimeouts.js
   webconsole-helpers.js
   webextension-helpers.js
 [test_animation_actor-lifetime.html]
@@ -109,3 +113,4 @@ skip-if = !e10s # test is designed to work on e10s only
 [test_webextension-addon-debugging-reload.html]
 skip-if = !e10s # test is designed to work on e10s only
 [test_websocket-server.html]
+[test_suspendTimeouts.html]

devtools/server/tests/mochitest/suspendTimeouts_content.html

@@ -0,0 +1 @@
+<script src='suspendTimeouts_content.js'></script>

devtools/server/tests/mochitest/suspendTimeouts_content.js

@@ -0,0 +1,66 @@
+"use strict";
+
+// To make it easier to follow, this code is arranged so that the functions are
+// arranged in the order they are called.
+
+const worker = new Worker("suspendTimeouts_worker.js");
+worker.onerror = error => {
+  const message =
+      `error from worker: ${error.filename}:${error.lineno}: ${error.message}`;
+  throw new Error(message);
+};
+
+// Create a message channel. Send one end to the worker, and return the other to
+// the mochitest.
+/* exported create_channel */
+function create_channel() {
+  const { port1, port2 } = new MessageChannel();
+  info(`sending port to worker`);
+  worker.postMessage({ mochitestPort: port1 }, [ port1 ]);
+  return port2;
+}
+
+// Provoke the worker into sending us a message, and then refuse to receive said
+// message, causing it to be delayed for later delivery.
+//
+// The worker will also post a message to the MessagePort we sent it earlier.
+// That message should not be delayed, as it is handled by the mochitest window,
+// not the content window. Its receipt signals that the test can assume that the
+// runnable for step 2) is in the main thread's event queue, so the test can
+// prepare for step 3).
+/* exported start_worker */
+function start_worker() {
+  worker.onmessage = handle_echo;
+
+  // This should prevent worker.onmessage from being called, until
+  // resumeTimeouts is called.
+  suspendTimeouts();
+
+  // The worker should echo this message back to us and to the mochitest.
+  worker.postMessage("HALLOOOOOO"); // suitable message for echoing
+  info(`posted message to worker`);
+}
+
+var resumeTimeouts_has_returned = false;
+
+// Resume timeouts. After this call, the worker's message should not be
+// delivered to our onmessage handler until control returns to the event loop.
+/* exported resume_timeouts */
+function resume_timeouts() {
+  resumeTimeouts(); // onmessage handlers should not run from this call.
+
+  resumeTimeouts_has_returned = true;
+
+  // When this JavaScript invocation returns to the main thread's event loop,
+  // only then should onmessage handlers be invoked.
+}
+
+// The buggy code calls this handler from the resumeTimeouts call, before the
+// main thread returns to the event loop. The correct code calls this only once
+// the JavaScript invocation that called resumeTimeouts has run to completion.
+function handle_echo({data}) {
+  ok(resumeTimeouts_has_returned, "worker message delivered from main event loop");
+
+  // Finish the mochitest.
+  finish();
+}

devtools/server/tests/mochitest/suspendTimeouts_worker.js

@@ -0,0 +1,12 @@
+"use strict";
+
+// Once content sends us a port connected to the mochitest, we simply echo every
+// message we receive back to content and the mochitest.
+onmessage = ({ data: { mochitestPort }}) => {
+  onmessage = ({ data }) => {
+    // Send a message to both content and the mochitest, which the main thread's
+    // event loop will attempt to deliver as step 2).
+    postMessage(`worker echo to content: ${data}`);
+    mochitestPort.postMessage(`worker echo to port: ${data}`);
+  };
+};

devtools/server/tests/mochitest/test_suspendTimeouts.html

@@ -0,0 +1,20 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1426467
+
+When we use windowUtils.resumeTimeouts to resume timeouts in a window, that call
+should not immediately dispatch `onmessage` handlers for messages from workers.
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Mozilla Bug 1426467</title>
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
+</head>
+<body>
+<pre id="test">
+<script src='test_suspendTimeouts.js'></script>
+</pre>
+</body>
+</html>

devtools/server/tests/mochitest/test_suspendTimeouts.js

@@ -0,0 +1,139 @@
+"use strict";
+
+// The debugger uses nsIDOMWindowUtils::suspendTimeouts and ...::resumeTimeouts
+// to ensure that content event handlers do not run while a JavaScript
+// invocation is stepping or paused at a breakpoint. If a worker thread sends
+// messages to the content while it is paused, those messages are deferred in a
+// private queue in the worker for later delivery.
+//
+// Bug 1426467 is that calling nsIDOMWindowUtils::resumeTimeouts actually
+// delivers the deferred messages itself, calling the content's 'onmessage'
+// handler. But the debugger calls suspend/resume around each individual
+// interruption of the debuggee -- each step, say -- meaning that hitting the
+// "step" button would cause you to step from the debuggee directly into an
+// onmessage handler.
+//
+// In other words, delivering deferred messages from resumeTimeouts, as it is
+// used by the debugger, breaks the run-to-completion rule. They must not be
+// delivered until after the JavaScript invocation at hand is complete. That's
+// what this test checks.
+//
+// In this test, the following steps must take place in order:
+//
+// 1) The content page must call suspendTimeouts.
+// 2) A runnable conveying a message from the worker thread must attempt to
+//    deliver the message, see that the content page has suspended such things,
+//    and hold the message for later delivery.
+// 3) The content page must call resumeTimeouts.
+//
+// In a correct implementation, the message delayed in step 2) is delivered only
+// after the main thread returns to the event loop after calling resumeTimeouts
+// in step 3). In the buggy implementation, the onmessage handler is called
+// directly from the call to resumeTimeouts, so that the onmessage handlers run
+// in the midst of whatever JavaScript invocation resumed timeouts (say,
+// stepping in the debugger), a violation of the run-to-completion rule.
+//
+// In this specific bug, the handlers are called from resumeTimeouts, but
+// really, running them any time before that invocation returns to the main
+// event loop would be a bug.
+//
+// Posting the message and calling resumeTimeouts take place in different
+// threads, but if 2) and 3) don't occur in that order, the worker's message
+// will never be delayed and the test will pass spuriously. But the worker
+// can't communicate with the content page directly, to let it know that it
+// should proceed with step 3): the purpose of suspendTimeouts is to pause
+// all such communication.
+//
+// So instead, the content page creates a MessageChannel, and passes one
+// MessagePort to the worker and the other to this mochitest (which has its
+// own window, separate from the one calling suspendTimeouts). The worker
+// notifies the mochitest when it has posted the message, and then the
+// mochitest calls into the content to carry out step 3).
+
+// To help you follow all the callbacks and event handlers, this code pulls out
+// event handler functions so that control flows from top to bottom.
+
+const nsIInterfaceRequestor = Components.interfaces.nsIInterfaceRequestor;
+const nsIDOMWindowUtils = Components.interfaces.nsIDOMWindowUtils;
+
+window.onload = function () {
+  // This mochitest is not complete until we call SimpleTest.finish. Don't just
+  // exit as soon as we return to the main event loop.
+  SimpleTest.waitForExplicitFinish();
+
+  let iframe = document.createElement("iframe");
+  iframe.src = "http://mochi.test:8888/chrome/devtools/server/tests/mochitest/suspendTimeouts_content.html";
+  iframe.onload = iframe_onload_handler;
+  document.body.appendChild(iframe);
+
+  function iframe_onload_handler() {
+    const content = iframe.contentWindow.wrappedJSObject;
+
+    const windowUtils = iframe.contentWindow.QueryInterface(nsIInterfaceRequestor)
+        .getInterface(nsIDOMWindowUtils);
+
+    // Hand over the suspend and resume functions to the content page, along
+    // with some testing utilities.
+    content.suspendTimeouts = function () {
+      SimpleTest.info("test_suspendTimeouts", "calling suspendTimeouts");
+      windowUtils.suspendTimeouts();
+    };
+    content.resumeTimeouts = function () {
+      windowUtils.resumeTimeouts();
+      SimpleTest.info("test_suspendTimeouts", "resumeTimeouts called");
+    };
+    content.info = function (message) {
+      SimpleTest.info("suspendTimeouts_content.js", message);
+    };
+    content.ok = SimpleTest.ok;
+    content.finish = finish;
+
+    SimpleTest.info("Disappointed with National Tautology Day? Well, it is what it is.");
+
+    // Once the worker has sent a message to its parent (which should get delayed),
+    // it sends us a message directly on this channel.
+    const workerPort = content.create_channel();
+    workerPort.onmessage = handle_worker_echo;
+
+    // Have content send the worker a message that it should echo back to both
+    // content and us. The echo to content should get delayed; the echo to us
+    // should cause our handle_worker_echo to be called.
+    content.start_worker();
+
+    function handle_worker_echo({ data }) {
+      info(`mochitest received message from worker: ${data}`);
+
+      // As it turns out, it's not correct to assume that, if the worker posts a
+      // message to its parent via the global `postMessage` function, and then
+      // posts a message to the mochitest via the MessagePort, those two
+      // messages will be delivered in the order they were sent.
+      //
+      // - Messages sent via the worker's global's postMessage go through two
+      //   ThrottledEventQueues (one in the worker, and one on the parent), and
+      //   eventually find their way into the thread's primary event queue,
+      //   which is a PrioritizedEventQueue.
+      //
+      // - Messages sent via a MessageChannel whose ports are owned by different
+      //   threads are passed as IPDL messages.
+      //
+      // There's basically no reliable way to ensure that delivery to content
+      // has been attempted and the runnable deferred; there are too many
+      // variables affecting the order in which things are processed. Delaying
+      // for a second is the best I could think of.
+      //
+      // Fortunately, this tactic failing can only cause spurious test passes
+      // (the runnable never gets deferred, so things work by accident), not
+      // spurious failures. Without some kind of trustworthy notification that
+      // the runnable has been deferred, perhaps via some special white-box
+      // testing API, we can't do better.
+      setTimeout(() => {
+        content.resume_timeouts();
+      }, 1000);
+    }
+
+    function finish(message) {
+      SimpleTest.info("suspendTimeouts_content.js", "called finish");
+      SimpleTest.finish();
+    }
+  }
+};

dom/workers/WorkerPrivate.cpp

@@ -1926,8 +1926,12 @@ WorkerPrivate::ParentWindowResumed()
     }
   }
 
-  // Execute queued runnables before waking up, otherwise the worker could post
-  // new messages before we run those that have been queued.
+  // When a debugger pause has ended, we must not execute the queued runnables
+  // immediately as we do in Thaw, as we are still in the midst of whatever
+  // JavaScript execution the debugger interrupted. Re-queueing them is not
+  // correct either, as doing so can re-order messages in some cases (bug
+  // 1433317), but executing them immediately can re-order them as well; see bug
+  // 1426467 comment 14 for an example.
   if (!IsFrozen() && !mQueuedRunnables.IsEmpty()) {
     MOZ_ASSERT(IsDedicatedWorker());
 
@@ -1935,7 +1939,7 @@ WorkerPrivate::ParentWindowResumed()
     mQueuedRunnables.SwapElements(runnables);
 
     for (uint32_t index = 0; index < runnables.Length(); index++) {
-      runnables[index]->Run();
+      MOZ_ALWAYS_SUCCEEDS(DispatchToMainThread(runnables[index].forget()));
     }
   }
 }