From d4700814c9f918516c358841096e3dcdaca53d92 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Thu, 18 Jan 2018 11:49:34 -0800 Subject: [PATCH] Bug 1426467: Re-enqueue messages from workers when debugger pause ends; don't run them immediately. r=bkelly MozReview-Commit-ID: 1Yyjqz5S6tZ --HG-- extra : rebase_source : 4f54bd6a6a3385ba4e424ac5ffe4354db4592b45 --- devtools/server/tests/mochitest/chrome.ini | 5 + .../mochitest/suspendTimeouts_content.html | 1 + .../mochitest/suspendTimeouts_content.js | 63 ++++++++ .../tests/mochitest/suspendTimeouts_worker.js | 12 ++ .../tests/mochitest/test_suspendTimeouts.html | 20 +++ .../tests/mochitest/test_suspendTimeouts.js | 139 ++++++++++++++++++ dom/workers/WorkerPrivate.cpp | 10 +- 7 files changed, 247 insertions(+), 3 deletions(-) create mode 100644 devtools/server/tests/mochitest/suspendTimeouts_content.html create mode 100644 devtools/server/tests/mochitest/suspendTimeouts_content.js create mode 100644 devtools/server/tests/mochitest/suspendTimeouts_worker.js create mode 100644 devtools/server/tests/mochitest/test_suspendTimeouts.html create mode 100644 devtools/server/tests/mochitest/test_suspendTimeouts.js diff --git a/devtools/server/tests/mochitest/chrome.ini b/devtools/server/tests/mochitest/chrome.ini index e7a8f82dfbdd..2859afa66cec 100644 --- a/devtools/server/tests/mochitest/chrome.ini +++ b/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] diff --git a/devtools/server/tests/mochitest/suspendTimeouts_content.html b/devtools/server/tests/mochitest/suspendTimeouts_content.html new file mode 100644 index 000000000000..f3969fc10c1f --- /dev/null +++ b/devtools/server/tests/mochitest/suspendTimeouts_content.html @@ -0,0 +1 @@ + diff --git a/devtools/server/tests/mochitest/suspendTimeouts_content.js b/devtools/server/tests/mochitest/suspendTimeouts_content.js new file mode 100644 index 000000000000..911dda8027a5 --- /dev/null +++ b/devtools/server/tests/mochitest/suspendTimeouts_content.js @@ -0,0 +1,63 @@ +"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. +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). +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. +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(); +} diff --git a/devtools/server/tests/mochitest/suspendTimeouts_worker.js b/devtools/server/tests/mochitest/suspendTimeouts_worker.js new file mode 100644 index 000000000000..fdbf17586e85 --- /dev/null +++ b/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}`); + }; +}; diff --git a/devtools/server/tests/mochitest/test_suspendTimeouts.html b/devtools/server/tests/mochitest/test_suspendTimeouts.html new file mode 100644 index 000000000000..f1e46dcb00c3 --- /dev/null +++ b/devtools/server/tests/mochitest/test_suspendTimeouts.html @@ -0,0 +1,20 @@ + + + + + + Mozilla Bug 1426467 + + + + +
+
+
+ + diff --git a/devtools/server/tests/mochitest/test_suspendTimeouts.js b/devtools/server/tests/mochitest/test_suspendTimeouts.js new file mode 100644 index 000000000000..ab67c9a5fdd1 --- /dev/null +++ b/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(); + } + } +}; diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp index decb33a23d7f..1936eeceec3e 100644 --- a/dom/workers/WorkerPrivate.cpp +++ b/dom/workers/WorkerPrivate.cpp @@ -1929,8 +1929,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()); @@ -1938,7 +1942,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())); } } }