Bug 1281040 - Make sure we detach from worker client when target is destroyed. r=jlongster

devtools/client/debugger/test/mochitest/browser.ini

@@ -616,7 +616,11 @@ skip-if = e10s && debug
 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

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);
+});

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);
+});

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);
+});

devtools/client/debugger/test/mochitest/browser_dbg_worker-console.js

@@ -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);
-});

devtools/client/debugger/test/mochitest/browser_dbg_worker-window.js

@@ -44,8 +44,9 @@ add_task(function* () {
   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();
 });

devtools/client/debugger/test/mochitest/head.js

@@ -1324,3 +1324,33 @@ function waitForDispatch(panel, type, eventRepeat = 1) {
     }
   });
 }
+
+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};
+}
+

devtools/client/framework/target.js

@@ -767,7 +767,9 @@ WorkerTarget.prototype = {
     return this._workerClient.client;
   },
 
-  destroy: function () {},
+  destroy: function () {
+    this._workerClient.detach();
+  },
 
   hasActor: function (name) {
     // console is the only one actor implemented by WorkerActor

devtools/server/main.js

@@ -863,10 +863,25 @@ var DebuggerServer = {
           transport.hooks = {
             onClosed: () => {
               if (!dbg.isClosed) {
+                // 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);
@@ -1444,6 +1459,26 @@ DebuggerServerConnection.prototype = {
    *        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);