Bug 1281040 - Make sure we detach from worker client when target is destroyed. r=jlongster, a=ritu
authorEddy Bruel <ejpbruel@mozilla.com
Mon, 08 Aug 2016 07:53:37 +0200
changeset 349789 c9e56c940955bbe5cf7070a650de5755047c7414
parent 349788 5203cfbd484a98acb1b68c8082195e51735cf0ff
child 349790 5872b231bb7af5bd859a1113529a30a183761650
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlongster, ritu
bugs1281040
milestone50.0a2
Bug 1281040 - Make sure we detach from worker client when target is destroyed. r=jlongster, a=ritu
devtools/client/debugger/test/mochitest/browser.ini
devtools/client/debugger/test/mochitest/browser_dbg_worker-console-01.js
devtools/client/debugger/test/mochitest/browser_dbg_worker-console-02.js
devtools/client/debugger/test/mochitest/browser_dbg_worker-console-03.js
devtools/client/debugger/test/mochitest/browser_dbg_worker-console.js
devtools/client/debugger/test/mochitest/browser_dbg_worker-window.js
devtools/client/debugger/test/mochitest/head.js
devtools/client/framework/target.js
devtools/server/main.js
--- a/devtools/client/debugger/test/mochitest/browser.ini
+++ b/devtools/client/debugger/test/mochitest/browser.ini
@@ -611,17 +611,21 @@ skip-if = e10s && debug
 [browser_dbg_variables-view-reexpand-03.js]
 skip-if = e10s && debug
 [browser_dbg_variables-view-webidl.js]
 skip-if = e10s && debug
 [browser_dbg_watch-expressions-01.js]
 skip-if = e10s && debug
 [browser_dbg_watch-expressions-02.js]
 skip-if = e10s && debug
-[browser_dbg_worker-console.js]
+[browser_dbg_worker-console-01.js]
+skip-if = e10s && debug
+[browser_dbg_worker-console-02.js]
+skip-if = e10s && debug
+[browser_dbg_worker-console-03.js]
 skip-if = e10s && debug
 [browser_dbg_worker-source-map.js]
 skip-if = e10s && debug
 [browser_dbg_worker-window.js]
 skip-if = e10s && debug
 [browser_dbg_WorkerActor.attach.js]
 skip-if = e10s && debug
 [browser_dbg_WorkerActor.attachThread.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/debugger/test/mochitest/browser_dbg_worker-console-01.js
@@ -0,0 +1,21 @@
+// Check to make sure that a worker can be attached to a toolbox
+// and that the console works.
+
+var TAB_URL = EXAMPLE_URL + "doc_WorkerActor.attachThread-tab.html";
+var WORKER_URL = "code_WorkerActor.attachThread-worker.js";
+
+add_task(function* testNormalExecution() {
+  let {client, tab, tabClient, workerClient, toolbox, gDebugger} =
+    yield initWorkerDebugger(TAB_URL, WORKER_URL);
+
+  let jsterm = yield getSplitConsole(toolbox);
+  let executed = yield jsterm.execute("this.location.toString()");
+  ok(executed.textContent.includes(WORKER_URL),
+      "Evaluating the global's location works");
+
+  terminateWorkerInTab(tab, WORKER_URL);
+  yield waitForWorkerClose(workerClient);
+  yield gDevTools.closeToolbox(TargetFactory.forWorker(workerClient));
+  yield close(client);
+  yield removeTab(tab);
+});
new file mode 100644
--- /dev/null
+++ b/devtools/client/debugger/test/mochitest/browser_dbg_worker-console-02.js
@@ -0,0 +1,59 @@
+// Check to make sure that a worker can be attached to a toolbox
+// and that the console works.
+
+var TAB_URL = EXAMPLE_URL + "doc_WorkerActor.attachThread-tab.html";
+var WORKER_URL = "code_WorkerActor.attachThread-worker.js";
+
+add_task(function* testWhilePaused() {
+  let {client, tab, tabClient, workerClient, toolbox, gDebugger} =
+    yield initWorkerDebugger(TAB_URL, WORKER_URL);
+
+  let gTarget = gDebugger.gTarget;
+  let gResumeButton = gDebugger.document.getElementById("resume");
+  let gResumeKey = gDebugger.document.getElementById("resumeKey");
+
+  // Execute some basic math to make sure evaluations are working.
+  let jsterm = yield getSplitConsole(toolbox);
+  let executed = yield jsterm.execute("10000+1");
+  ok(executed.textContent.includes("10001"), "Text for message appeared correct");
+
+  // Pause the worker by waiting for next execution and then sending a message to
+  // it from the main thread.
+  let oncePaused = gTarget.once("thread-paused");
+  EventUtils.sendMouseEvent({ type: "mousedown" }, gResumeButton, gDebugger);
+  once(gDebugger.gClient, "willInterrupt").then(() => {
+    info("Posting message to worker, then waiting for a pause");
+    postMessageToWorkerInTab(tab, WORKER_URL, "ping");
+  });
+  yield oncePaused;
+
+  let command1 = jsterm.execute("10000+2");
+  let command2 = jsterm.execute("10000+3");
+  let command3 = jsterm.execute("foobar"); // throw an error
+
+  info("Trying to get the result of command1");
+  executed = yield command1;
+  ok(executed.textContent.includes("10002"),
+      "command1 executed successfully");
+
+  info("Trying to get the result of command2");
+  executed = yield command2;
+  ok(executed.textContent.includes("10003"),
+      "command2 executed successfully");
+
+  info("Trying to get the result of command3");
+  executed = yield command3;
+  // XXXworkers This is failing until Bug 1215120 is resolved.
+  todo(executed.textContent.includes("ReferenceError: foobar is not defined"),
+      "command3 executed successfully");
+
+  let onceResumed = gTarget.once("thread-resumed");
+  EventUtils.sendMouseEvent({ type: "mousedown" }, gResumeButton, gDebugger);
+  yield onceResumed;
+
+  terminateWorkerInTab(tab, WORKER_URL);
+  yield waitForWorkerClose(workerClient);
+  yield gDevTools.closeToolbox(TargetFactory.forWorker(workerClient));
+  yield close(client);
+  yield removeTab(tab);
+});
new file mode 100644
--- /dev/null
+++ b/devtools/client/debugger/test/mochitest/browser_dbg_worker-console-03.js
@@ -0,0 +1,46 @@
+// Check to make sure that a worker can be attached to a toolbox
+// and that the console works.
+
+var TAB_URL = EXAMPLE_URL + "doc_WorkerActor.attachThread-tab.html";
+var WORKER_URL = "code_WorkerActor.attachThread-worker.js";
+
+// Test to see if creating the pause from the console works.
+add_task(function* testPausedByConsole() {
+  let {client, tab, tabClient, workerClient, toolbox, gDebugger} =
+    yield initWorkerDebugger(TAB_URL, WORKER_URL);
+
+  let gTarget = gDebugger.gTarget;
+  let gResumeButton = gDebugger.document.getElementById("resume");
+  let gResumeKey = gDebugger.document.getElementById("resumeKey");
+
+  let jsterm = yield getSplitConsole(toolbox);
+  let executed = yield jsterm.execute("10000+1");
+  ok(executed.textContent.includes("10001"),
+      "Text for message appeared correct");
+
+  let oncePaused = gTarget.once("thread-paused");
+  EventUtils.sendMouseEvent({ type: "mousedown" }, gResumeButton, gDebugger);
+  let pausedExecution = jsterm.execute("10000+2");
+
+  info("Executed a command with 'break on next' active, waiting for pause");
+  yield oncePaused;
+
+  executed = yield jsterm.execute("10000+3");
+  ok(executed.textContent.includes("10003"),
+      "Text for message appeared correct");
+
+  info("Waiting for a resume");
+  let onceResumed = gTarget.once("thread-resumed");
+  EventUtils.sendMouseEvent({ type: "mousedown" }, gResumeButton, gDebugger);
+  yield onceResumed;
+
+  executed = yield pausedExecution;
+  ok(executed.textContent.includes("10002"),
+      "Text for message appeared correct");
+
+  terminateWorkerInTab(tab, WORKER_URL);
+  yield waitForWorkerClose(workerClient);
+  yield gDevTools.closeToolbox(TargetFactory.forWorker(workerClient));
+  yield close(client);
+  yield removeTab(tab);
+});
deleted file mode 100644
--- a/devtools/client/debugger/test/mochitest/browser_dbg_worker-console.js
+++ /dev/null
@@ -1,145 +0,0 @@
-// Check to make sure that a worker can be attached to a toolbox
-// and that the console works.
-
-var TAB_URL = EXAMPLE_URL + "doc_WorkerActor.attachThread-tab.html";
-var WORKER_URL = "code_WorkerActor.attachThread-worker.js";
-
-function* initWorkerDebugger(TAB_URL, WORKER_URL) {
-  if (!DebuggerServer.initialized) {
-    DebuggerServer.init();
-    DebuggerServer.addBrowserActors();
-  }
-
-  let client = new DebuggerClient(DebuggerServer.connectPipe());
-  yield connect(client);
-
-  let tab = yield addTab(TAB_URL);
-  let { tabs } = yield listTabs(client);
-  let [, tabClient] = yield attachTab(client, findTab(tabs, TAB_URL));
-
-  yield createWorkerInTab(tab, WORKER_URL);
-
-  let { workers } = yield listWorkers(tabClient);
-  let [, workerClient] = yield attachWorker(tabClient,
-                                             findWorker(workers, WORKER_URL));
-
-  let toolbox = yield gDevTools.showToolbox(TargetFactory.forWorker(workerClient),
-                                            "jsdebugger",
-                                            Toolbox.HostType.WINDOW);
-
-  let debuggerPanel = toolbox.getCurrentPanel();
-  let gDebugger = debuggerPanel.panelWin;
-
-  return {client, tab, tabClient, workerClient, toolbox, gDebugger};
-}
-
-add_task(function* testNormalExecution() {
-  let {client, tab, tabClient, workerClient, toolbox, gDebugger} =
-    yield initWorkerDebugger(TAB_URL, WORKER_URL);
-
-  let jsterm = yield getSplitConsole(toolbox);
-  let executed = yield jsterm.execute("this.location.toString()");
-  ok(executed.textContent.includes(WORKER_URL),
-      "Evaluating the global's location works");
-
-  yield gDevTools.closeToolbox(TargetFactory.forWorker(workerClient));
-  terminateWorkerInTab(tab, WORKER_URL);
-  yield waitForWorkerClose(workerClient);
-  yield close(client);
-  yield removeTab(tab);
-});
-
-add_task(function* testWhilePaused() {
-  let {client, tab, tabClient, workerClient, toolbox, gDebugger} =
-    yield initWorkerDebugger(TAB_URL, WORKER_URL);
-
-  let gTarget = gDebugger.gTarget;
-  let gResumeButton = gDebugger.document.getElementById("resume");
-  let gResumeKey = gDebugger.document.getElementById("resumeKey");
-
-  // Execute some basic math to make sure evaluations are working.
-  let jsterm = yield getSplitConsole(toolbox);
-  let executed = yield jsterm.execute("10000+1");
-  ok(executed.textContent.includes("10001"), "Text for message appeared correct");
-
-  // Pause the worker by waiting for next execution and then sending a message to
-  // it from the main thread.
-  let oncePaused = gTarget.once("thread-paused");
-  EventUtils.sendMouseEvent({ type: "mousedown" }, gResumeButton, gDebugger);
-  once(gDebugger.gClient, "willInterrupt").then(() => {
-    info("Posting message to worker, then waiting for a pause");
-    postMessageToWorkerInTab(tab, WORKER_URL, "ping");
-  });
-  yield oncePaused;
-
-  let command1 = jsterm.execute("10000+2");
-  let command2 = jsterm.execute("10000+3");
-  let command3 = jsterm.execute("foobar"); // throw an error
-
-  info("Trying to get the result of command1");
-  executed = yield command1;
-  ok(executed.textContent.includes("10002"),
-      "command1 executed successfully");
-
-  info("Trying to get the result of command2");
-  executed = yield command2;
-  ok(executed.textContent.includes("10003"),
-      "command2 executed successfully");
-
-  info("Trying to get the result of command3");
-  executed = yield command3;
-  // XXXworkers This is failing until Bug 1215120 is resolved.
-  todo(executed.textContent.includes("ReferenceError: foobar is not defined"),
-      "command3 executed successfully");
-
-  let onceResumed = gTarget.once("thread-resumed");
-  EventUtils.sendMouseEvent({ type: "mousedown" }, gResumeButton, gDebugger);
-  yield onceResumed;
-
-  yield gDevTools.closeToolbox(TargetFactory.forWorker(workerClient));
-  terminateWorkerInTab(tab, WORKER_URL);
-  yield waitForWorkerClose(workerClient);
-  yield close(client);
-  yield removeTab(tab);
-});
-
-// Test to see if creating the pause from the console works.
-add_task(function* testPausedByConsole() {
-  let {client, tab, tabClient, workerClient, toolbox, gDebugger} =
-    yield initWorkerDebugger(TAB_URL, WORKER_URL);
-
-  let gTarget = gDebugger.gTarget;
-  let gResumeButton = gDebugger.document.getElementById("resume");
-  let gResumeKey = gDebugger.document.getElementById("resumeKey");
-
-  let jsterm = yield getSplitConsole(toolbox);
-  let executed = yield jsterm.execute("10000+1");
-  ok(executed.textContent.includes("10001"),
-      "Text for message appeared correct");
-
-  let oncePaused = gTarget.once("thread-paused");
-  EventUtils.sendMouseEvent({ type: "mousedown" }, gResumeButton, gDebugger);
-  let pausedExecution = jsterm.execute("10000+2");
-
-  info("Executed a command with 'break on next' active, waiting for pause");
-  yield oncePaused;
-
-  executed = yield jsterm.execute("10000+3");
-  ok(executed.textContent.includes("10003"),
-      "Text for message appeared correct");
-
-  info("Waiting for a resume");
-  let onceResumed = gTarget.once("thread-resumed");
-  EventUtils.sendMouseEvent({ type: "mousedown" }, gResumeButton, gDebugger);
-  yield onceResumed;
-
-  executed = yield pausedExecution;
-  ok(executed.textContent.includes("10002"),
-      "Text for message appeared correct");
-
-  yield gDevTools.closeToolbox(TargetFactory.forWorker(workerClient));
-  terminateWorkerInTab(tab, WORKER_URL);
-  yield waitForWorkerClose(workerClient);
-  yield close(client);
-  yield removeTab(tab);
-});
--- a/devtools/client/debugger/test/mochitest/browser_dbg_worker-window.js
+++ b/devtools/client/debugger/test/mochitest/browser_dbg_worker-window.js
@@ -39,13 +39,14 @@ add_task(function* () {
      "worker URL in host title");
 
   let toolTabs = toolbox.doc.querySelectorAll(".devtools-tab");
   let activeTools = [...toolTabs].map(tab=>tab.getAttribute("toolid"));
 
   is(activeTools.join(","), "webconsole,jsdebugger,scratchpad,options",
     "Correct set of tools supported by worker");
 
-  yield toolbox.destroy();
   terminateWorkerInTab(tab, WORKER_URL);
   yield waitForWorkerClose(workerClient);
   yield close(client);
+
+  yield toolbox.destroy();
 });
--- a/devtools/client/debugger/test/mochitest/head.js
+++ b/devtools/client/debugger/test/mochitest/head.js
@@ -1319,8 +1319,38 @@ function waitForDispatch(panel, type, ev
     info("Waiting for " + type + " to dispatch " + eventRepeat + " time(s)");
     while (count < eventRepeat) {
       yield _afterDispatchDone(controller, actionType);
       count++;
       info(type + " dispatched " + count + " time(s)");
     }
   });
 }
+
+function* initWorkerDebugger(TAB_URL, WORKER_URL) {
+  if (!DebuggerServer.initialized) {
+    DebuggerServer.init();
+    DebuggerServer.addBrowserActors();
+  }
+
+  let client = new DebuggerClient(DebuggerServer.connectPipe());
+  yield connect(client);
+
+  let tab = yield addTab(TAB_URL);
+  let { tabs } = yield listTabs(client);
+  let [, tabClient] = yield attachTab(client, findTab(tabs, TAB_URL));
+
+  yield createWorkerInTab(tab, WORKER_URL);
+
+  let { workers } = yield listWorkers(tabClient);
+  let [, workerClient] = yield attachWorker(tabClient,
+                                             findWorker(workers, WORKER_URL));
+
+  let toolbox = yield gDevTools.showToolbox(TargetFactory.forWorker(workerClient),
+                                            "jsdebugger",
+                                            Toolbox.HostType.WINDOW);
+
+  let debuggerPanel = toolbox.getCurrentPanel();
+  let gDebugger = debuggerPanel.panelWin;
+
+  return {client, tab, tabClient, workerClient, toolbox, gDebugger};
+}
+
--- a/devtools/client/framework/target.js
+++ b/devtools/client/framework/target.js
@@ -762,17 +762,19 @@ WorkerTarget.prototype = {
   get activeTab() {
     return this._workerClient;
   },
 
   get client() {
     return this._workerClient.client;
   },
 
-  destroy: function () {},
+  destroy: function () {
+    this._workerClient.detach();
+  },
 
   hasActor: function (name) {
     // console is the only one actor implemented by WorkerActor
     if (name == "console") {
       return true;
     }
     return false;
   },
--- a/devtools/server/main.js
+++ b/devtools/server/main.js
@@ -857,20 +857,35 @@ var DebuggerServer = {
           dbg.removeListener(listener);
 
           // Step 7: Create a transport for the connection to the worker.
           let transport = new WorkerDebuggerTransport(dbg, id);
           transport.ready();
           transport.hooks = {
             onClosed: () => {
               if (!dbg.isClosed) {
-                dbg.postMessage(JSON.stringify({
-                  type: "disconnect",
-                  id,
-                }));
+                // If the worker happens to be shutting down while we are trying
+                // to close the connection, there is a small interval during
+                // which no more runnables can be dispatched to the worker, but
+                // the worker debugger has not yet been closed. In that case,
+                // the call to postMessage below will fail. The onClosed hook on
+                // DebuggerTransport is not supposed to throw exceptions, so we
+                // need to make sure to catch these early.
+                try {
+                  dbg.postMessage(JSON.stringify({
+                    type: "disconnect",
+                    id,
+                  }));
+                } catch (e) {
+                  // We can safely ignore these exceptions. The only time the
+                  // call to postMessage can fail is if the worker is either
+                  // shutting down, or has finished shutting down. In both
+                  // cases, there is nothing to clean up, so we don't care
+                  // whether this message arrives or not.
+                }
               }
 
               connection.cancelForwarding(id);
             },
 
             onPacket: (packet) => {
               // Ensure that any packets received from the server on the worker
               // thread are forwarded to the client on the main thread, as if
@@ -1438,16 +1453,36 @@ DebuggerServerConnection.prototype = {
    *
    * @param ActorPool actorPool
    *        The ActorPool instance you want to remove.
    * @param boolean noCleanup [optional]
    *        True if you don't want to disconnect each actor from the pool, false
    *        otherwise.
    */
   removeActorPool(actorPool, noCleanup) {
+    // When a connection is closed, it removes each of its actor pools. When an
+    // actor pool is removed, it calls the disconnect method on each of its
+    // actors. Some actors, such as ThreadActor, manage their own actor pools.
+    // When the disconnect method is called on these actors, they manually
+    // remove their actor pools. Consequently, this method is reentrant.
+    //
+    // In addition, some actors, such as ThreadActor, perform asynchronous work
+    // (in the case of ThreadActor, because they need to resume), before they
+    // remove each of their actor pools. Since we don't wait for this work to
+    // be completed, we can end up in this function recursively after the
+    // connection already set this._extraPools to null.
+    //
+    // This is a bug: if the disconnect method can perform asynchronous work,
+    // then we should wait for that work to be completed before setting this.
+    // _extraPools to null. As a temporary solution, it should be acceptable
+    // to just return early (if this._extraPools has been set to null, all
+    // actors pools for this connection should already have been removed).
+    if (this._extraPools === null) {
+      return;
+    }
     let index = this._extraPools.lastIndexOf(actorPool);
     if (index > -1) {
       let pool = this._extraPools.splice(index, 1);
       if (!noCleanup) {
         pool.forEach(p => p.destroy());
       }
     }
   },