From a8aec05d11cd428bf76a2e006fc465828f0d6c99 Mon Sep 17 00:00:00 2001
From: "J. Ryan Stinnett" <jryans@gmail.com>
Date: Fri, 11 Nov 2016 18:24:58 -0600
Subject: [PATCH] Bug 1315391 - Rename all `disconnect` methods to `destroy` in
 actors. r=ochameau

Ever since protocol.js was added as a way to create DevTools actors, we've had
lots of confusion about the correct way to implement actor destruction.  If your
actor's _parent_ was the legacy kind, you had to use `disconnect`.  If it was
protocol.js, you had to use `destroy`.

There is no reason for this madness, which makes reasoning about destruction
quite hard.  Here we rename `disconnect` to `destroy` so there is only one name
for every destruction path.

MozReview-Commit-ID: C1Yw9NfUUR2

--HG--
extra : rebase_source : 4d018622b7547d404510e0b563c6324c0127aafc
---
 devtools/server/actors/addon.js                  | 10 ++++------
 devtools/server/actors/animation.js              |  8 --------
 devtools/server/actors/breakpoint.js             |  2 +-
 devtools/server/actors/child-process.js          |  2 +-
 devtools/server/actors/common.js                 | 16 ++++++++++------
 devtools/server/actors/emulation.js              |  4 ----
 devtools/server/actors/frame.js                  |  2 +-
 devtools/server/actors/gcli.js                   |  4 ----
 devtools/server/actors/inspector.js              |  6 ------
 devtools/server/actors/monitor.js                |  2 +-
 devtools/server/actors/object.js                 |  4 ++--
 devtools/server/actors/performance-entries.js    |  4 ----
 devtools/server/actors/performance.js            |  8 --------
 devtools/server/actors/profiler.js               |  8 --------
 devtools/server/actors/reflow.js                 |  9 ---------
 devtools/server/actors/root.js                   |  8 ++++----
 devtools/server/actors/script.js                 | 10 +++++-----
 devtools/server/actors/source.js                 |  2 +-
 devtools/server/actors/stylesheets.js            |  8 --------
 devtools/server/actors/timeline.js               |  9 ---------
 devtools/server/actors/webbrowser.js             |  7 +++----
 devtools/server/actors/webconsole.js             |  3 +--
 devtools/server/actors/worker.js                 |  8 --------
 devtools/server/main.js                          |  8 ++++----
 .../tests/mochitest/test_connectToChild.html     | 16 ++++++++--------
 .../server/tests/unit/test_longstringactor.js    |  6 +++---
 26 files changed, 49 insertions(+), 125 deletions(-)

diff --git a/devtools/server/actors/addon.js b/devtools/server/actors/addon.js
index 7f152e984539..9355f2093cfc 100644
--- a/devtools/server/actors/addon.js
+++ b/devtools/server/actors/addon.js
@@ -70,7 +70,6 @@ BrowserAddonActor.prototype = {
     return this._sources;
   },
 
-
   form: function BAA_form() {
     assert(this.actorID, "addon should have an actorID.");
     if (!this._consoleActor) {
@@ -95,7 +94,7 @@ BrowserAddonActor.prototype = {
     };
   },
 
-  disconnect: function BAA_disconnect() {
+  destroy() {
     this.conn.removeActorPool(this._contextPool);
     this._contextPool = null;
     this._consoleActor = null;
@@ -140,7 +139,7 @@ BrowserAddonActor.prototype = {
       this.conn.send({ from: this.actorID, type: "tabDetached" });
     }
 
-    this.disconnect();
+    this.destroy();
   },
 
   onAttach: function BAA_onAttach() {
@@ -309,9 +308,8 @@ update(AddonConsoleActor.prototype, {
   /**
    * Destroy the current AddonConsoleActor instance.
    */
-  disconnect: function ACA_disconnect()
-  {
-    WebConsoleActor.prototype.disconnect.call(this);
+  destroy() {
+    WebConsoleActor.prototype.destroy.call(this);
     this.addon = null;
   },
 
diff --git a/devtools/server/actors/animation.js b/devtools/server/actors/animation.js
index 642c4bcafba6..f40d3fd3f510 100644
--- a/devtools/server/actors/animation.js
+++ b/devtools/server/actors/animation.js
@@ -497,14 +497,6 @@ var AnimationsActor = exports.AnimationsActor = protocol.ActorClassWithSpec(anim
     this.tabActor = this.observer = this.actors = this.walker = null;
   },
 
-  /**
-   * Since AnimationsActor doesn't have a protocol.js parent actor that takes
-   * care of its lifetime, implementing disconnect is required to cleanup.
-   */
-  disconnect: function () {
-    this.destroy();
-  },
-
   /**
    * Clients can optionally call this with a reference to their WalkerActor.
    * If they do, then AnimationPlayerActor's forms are going to also include
diff --git a/devtools/server/actors/breakpoint.js b/devtools/server/actors/breakpoint.js
index 547dcd0f14f1..8435bbb45150 100644
--- a/devtools/server/actors/breakpoint.js
+++ b/devtools/server/actors/breakpoint.js
@@ -54,7 +54,7 @@ let BreakpointActor = ActorClassWithSpec(breakpointSpec, {
     this.isPending = true;
   },
 
-  disconnect: function () {
+  destroy: function () {
     this.removeScripts();
   },
 
diff --git a/devtools/server/actors/child-process.js b/devtools/server/actors/child-process.js
index 7b0e2eaf855b..df053db0ae85 100644
--- a/devtools/server/actors/child-process.js
+++ b/devtools/server/actors/child-process.js
@@ -121,7 +121,7 @@ ChildProcessActor.prototype = {
     this._workerList.onListChanged = null;
   },
 
-  disconnect: function () {
+  destroy: function () {
     this.conn.removeActorPool(this._contextPool);
     this._contextPool = null;
 
diff --git a/devtools/server/actors/common.js b/devtools/server/actors/common.js
index 0177c67491e2..a40bbd81eab2 100644
--- a/devtools/server/actors/common.js
+++ b/devtools/server/actors/common.js
@@ -254,13 +254,17 @@ ActorPool.prototype = {
   },
 
   /**
-   * Remove an actor from the pool. If the actor has a disconnect method, call
-   * it.
+   * Remove an actor from the pool. If the actor has a destroy method, call it.
    */
-  removeActor: function AP_remove(aActor) {
-    delete this._actors[aActor.actorID];
-    if (aActor.disconnect) {
-      aActor.disconnect();
+  removeActor(actor) {
+    delete this._actors[actor.actorID];
+    if (actor.destroy) {
+      actor.destroy();
+      return;
+    }
+    // Obsolete destruction method name (might still be used by custom actors)
+    if (actor.disconnect) {
+      actor.disconnect();
     }
   },
 
diff --git a/devtools/server/actors/emulation.js b/devtools/server/actors/emulation.js
index b69183305c38..c866eda15d16 100644
--- a/devtools/server/actors/emulation.js
+++ b/devtools/server/actors/emulation.js
@@ -31,10 +31,6 @@ let EmulationActor = protocol.ActorClassWithSpec(emulationSpec, {
     this.simulatorCore = new SimulatorCore(tabActor.chromeEventHandler);
   },
 
-  disconnect() {
-    this.destroy();
-  },
-
   destroy() {
     this.clearDPPXOverride();
     this.clearNetworkThrottling();
diff --git a/devtools/server/actors/frame.js b/devtools/server/actors/frame.js
index fefcad1b036a..4511bcebf57a 100644
--- a/devtools/server/actors/frame.js
+++ b/devtools/server/actors/frame.js
@@ -44,7 +44,7 @@ let FrameActor = ActorClassWithSpec(frameSpec, {
    * Finalization handler that is called when the actor is being evicted from
    * the pool.
    */
-  disconnect: function () {
+  destroy: function () {
     this.conn.removeActorPool(this._frameLifetimePool);
     this._frameLifetimePool = null;
   },
diff --git a/devtools/server/actors/gcli.js b/devtools/server/actors/gcli.js
index 651825a28d24..7282c3619255 100644
--- a/devtools/server/actors/gcli.js
+++ b/devtools/server/actors/gcli.js
@@ -25,10 +25,6 @@ const GcliActor = ActorClassWithSpec(gcliSpec, {
     this._requisitionPromise = undefined; // see _getRequisition()
   },
 
-  disconnect: function () {
-    return this.destroy();
-  },
-
   destroy: function () {
     Actor.prototype.destroy.call(this);
 
diff --git a/devtools/server/actors/inspector.js b/devtools/server/actors/inspector.js
index 20a227a40dd0..e0a93d3f244c 100644
--- a/devtools/server/actors/inspector.js
+++ b/devtools/server/actors/inspector.js
@@ -2641,12 +2641,6 @@ exports.InspectorActor = protocol.ActorClassWithSpec(inspectorSpec, {
     this.tabActor = null;
   },
 
-  // Forces destruction of the actor and all its children
-  // like highlighter, walker and style actors.
-  disconnect: function () {
-    this.destroy();
-  },
-
   get window() {
     return this.tabActor.window;
   },
diff --git a/devtools/server/actors/monitor.js b/devtools/server/actors/monitor.js
index 17b076a9e1f2..4684abf1be79 100644
--- a/devtools/server/actors/monitor.js
+++ b/devtools/server/actors/monitor.js
@@ -48,7 +48,7 @@ MonitorActor.prototype = {
     return {};
   },
 
-  disconnect: function () {
+  destroy: function () {
     this.stop();
   },
 
diff --git a/devtools/server/actors/object.js b/devtools/server/actors/object.js
index 1f417b951593..3ab44bd7f174 100644
--- a/devtools/server/actors/object.js
+++ b/devtools/server/actors/object.js
@@ -2081,9 +2081,9 @@ function LongStringActor(string) {
 LongStringActor.prototype = {
   actorPrefix: "longString",
 
-  disconnect: function () {
+  destroy: function () {
     // Because longStringActors is not a weak map, we won't automatically leave
-    // it so we need to manually leave on disconnect so that we don't leak
+    // it so we need to manually leave on destroy so that we don't leak
     // memory.
     this._releaseActor();
   },
diff --git a/devtools/server/actors/performance-entries.js b/devtools/server/actors/performance-entries.js
index 89434324ae4a..e6a34b0b8f23 100644
--- a/devtools/server/actors/performance-entries.js
+++ b/devtools/server/actors/performance-entries.js
@@ -42,10 +42,6 @@ var PerformanceEntriesActor = ActorClassWithSpec(performanceSpec, {
     }
   },
 
-  disconnect: function () {
-    this.destroy();
-  },
-
   destroy: function () {
     this.stop();
     Actor.prototype.destroy.call(this);
diff --git a/devtools/server/actors/performance.js b/devtools/server/actors/performance.js
index 8b294a4de55f..ac65320de195 100644
--- a/devtools/server/actors/performance.js
+++ b/devtools/server/actors/performance.js
@@ -53,14 +53,6 @@ var PerformanceActor = ActorClassWithSpec(performanceSpec, {
     events.on(this.bridge, "*", this._onRecorderEvent);
   },
 
-  /**
-   * `disconnect` method required to call destroy, since this
-   * actor is not managed by a parent actor.
-   */
-  disconnect: function () {
-    this.destroy();
-  },
-
   destroy: function () {
     events.off(this.bridge, "*", this._onRecorderEvent);
     this.bridge.destroy();
diff --git a/devtools/server/actors/profiler.js b/devtools/server/actors/profiler.js
index c4b594408cca..bb23b6641d5c 100644
--- a/devtools/server/actors/profiler.js
+++ b/devtools/server/actors/profiler.js
@@ -25,14 +25,6 @@ var ProfilerActor = exports.ProfilerActor = ActorClassWithSpec(profilerSpec, {
     events.on(this.bridge, "*", this._onProfilerEvent);
   },
 
-  /**
-   * `disconnect` method required to call destroy, since this
-   * actor is not managed by a parent actor.
-   */
-  disconnect: function () {
-    this.destroy();
-  },
-
   destroy: function () {
     events.off(this.bridge, "*", this._onProfilerEvent);
     this.bridge.destroy();
diff --git a/devtools/server/actors/reflow.js b/devtools/server/actors/reflow.js
index 0ebe00207f2f..286b0d08ba77 100644
--- a/devtools/server/actors/reflow.js
+++ b/devtools/server/actors/reflow.js
@@ -46,15 +46,6 @@ var ReflowActor = exports.ReflowActor = protocol.ActorClassWithSpec(reflowSpec,
     this._isStarted = false;
   },
 
-  /**
-   * The reflow actor is the first (and last) in its hierarchy to use
-   * protocol.js so it doesn't have a parent protocol actor that takes care of
-   * its lifetime. So it needs a disconnect method to cleanup.
-   */
-  disconnect: function () {
-    this.destroy();
-  },
-
   destroy: function () {
     this.stop();
     releaseLayoutChangesObserver(this.tabActor);
diff --git a/devtools/server/actors/root.js b/devtools/server/actors/root.js
index 1b6c00acc753..9ff3b978974a 100644
--- a/devtools/server/actors/root.js
+++ b/devtools/server/actors/root.js
@@ -47,7 +47,7 @@ loader.lazyGetter(this, "ppmm", () => {
  *       reply whose value is the name of an actor constructed by
  *       |A[P]|.
  *
- *     - onShutdown: a function to call when the root actor is disconnected.
+ *     - onShutdown: a function to call when the root actor is destroyed.
  *
  * Instance properties:
  *
@@ -80,7 +80,7 @@ loader.lazyGetter(this, "ppmm", () => {
  * The root actor registers an 'onListChanged' handler on the appropriate
  * list when it may need to send the client 'tabListChanged' notifications,
  * and is careful to remove the handler whenever it does not need to send
- * such notifications (including when it is disconnected). This means that
+ * such notifications (including when it is destroyed). This means that
  * live list implementations can use the state of the handler property (set
  * or null) to install and remove observers and event listeners.
  *
@@ -206,9 +206,9 @@ RootActor.prototype = {
   },
 
   /**
-   * Disconnects the actor from the browser window.
+   * Destroys the actor from the browser window.
    */
-  disconnect: function () {
+  destroy: function () {
     /* Tell the live lists we aren't watching any more. */
     if (this._parameters.tabList) {
       this._parameters.tabList.onListChanged = null;
diff --git a/devtools/server/actors/script.js b/devtools/server/actors/script.js
index e8e39546c24d..2f0726bf53f1 100644
--- a/devtools/server/actors/script.js
+++ b/devtools/server/actors/script.js
@@ -561,8 +561,8 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
     });
   },
 
-  disconnect: function () {
-    dumpn("in ThreadActor.prototype.disconnect");
+  destroy: function () {
+    dumpn("in ThreadActor.prototype.destroy");
     if (this._state == "paused") {
       this.onResume();
     }
@@ -592,10 +592,10 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
   },
 
   /**
-   * Disconnect the debugger and put the actor in the exited state.
+   * destroy the debugger and put the actor in the exited state.
    */
   exit: function () {
-    this.disconnect();
+    this.destroy();
     this._state = "exited";
   },
 
@@ -654,7 +654,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
   },
 
   onDetach: function (aRequest) {
-    this.disconnect();
+    this.destroy();
     this._state = "detached";
     this._debuggerSourcesSeen = null;
 
diff --git a/devtools/server/actors/source.js b/devtools/server/actors/source.js
index e76c14fe8798..bd39cb951dda 100644
--- a/devtools/server/actors/source.js
+++ b/devtools/server/actors/source.js
@@ -226,7 +226,7 @@ let SourceActor = ActorClassWithSpec(sourceSpec, {
     };
   },
 
-  disconnect: function () {
+  destroy: function () {
     if (this.registeredPool && this.registeredPool.sourceActors) {
       delete this.registeredPool.sourceActors[this.actorID];
     }
diff --git a/devtools/server/actors/stylesheets.js b/devtools/server/actors/stylesheets.js
index f20634e6cb54..c3c7e64610df 100644
--- a/devtools/server/actors/stylesheets.js
+++ b/devtools/server/actors/stylesheets.js
@@ -258,14 +258,6 @@ var StyleSheetActor = protocol.ActorClassWithSpec(styleSheetSpec, {
     }
   },
 
-  /**
-   * Since StyleSheetActor doesn't have a protocol.js parent actor that take
-   * care of its lifetime, implementing disconnect is required to cleanup.
-   */
-  disconnect: function () {
-    this.destroy();
-  },
-
   initialize: function (aStyleSheet, aParentActor, aWindow) {
     protocol.Actor.prototype.initialize.call(this, null);
 
diff --git a/devtools/server/actors/timeline.js b/devtools/server/actors/timeline.js
index 221454144586..4c1d52d741fc 100644
--- a/devtools/server/actors/timeline.js
+++ b/devtools/server/actors/timeline.js
@@ -39,15 +39,6 @@ var TimelineActor = exports.TimelineActor = protocol.ActorClassWithSpec(timeline
     events.on(this.bridge, "*", this._onTimelineEvent);
   },
 
-  /**
-   * The timeline actor is the first (and last) in its hierarchy to use
-   * protocol.js so it doesn't have a parent protocol actor that takes care of
-   * its lifetime. So it needs a disconnect method to cleanup.
-   */
-  disconnect: function () {
-    this.destroy();
-  },
-
   /**
    * Destroys this actor, stopping recording first.
    */
diff --git a/devtools/server/actors/webbrowser.js b/devtools/server/actors/webbrowser.js
index 0edcdc18756e..05fc8dfe94f4 100644
--- a/devtools/server/actors/webbrowser.js
+++ b/devtools/server/actors/webbrowser.js
@@ -484,8 +484,7 @@ BrowserTabList.prototype._handleActorClose = function (actor, browser) {
 /**
  * Make sure we are listening or not listening for activity elsewhere in
  * the browser, as appropriate. Other than setting up newly created XUL
- * windows, all listener / observer connection and disconnection should
- * happen here.
+ * windows, all listener / observer management should happen here.
  */
 BrowserTabList.prototype._checkListening = function () {
   /*
@@ -887,7 +886,7 @@ function TabActor(connection) {
   this._onWorkerActorListChanged = this._onWorkerActorListChanged.bind(this);
 }
 
-// XXX (bug 710213): TabActor attach/detach/exit/disconnect is a
+// XXX (bug 710213): TabActor attach/detach/exit/destroy is a
 // *complete* mess, needs to be rethought asap.
 
 TabActor.prototype = {
@@ -1137,7 +1136,7 @@ TabActor.prototype = {
   /**
    * Called when the actor is removed from the connection.
    */
-  disconnect() {
+  destroy() {
     this.exit();
   },
 
diff --git a/devtools/server/actors/webconsole.js b/devtools/server/actors/webconsole.js
index 9712ff32d831..95926f6e649f 100644
--- a/devtools/server/actors/webconsole.js
+++ b/devtools/server/actors/webconsole.js
@@ -336,8 +336,7 @@ WebConsoleActor.prototype =
   /**
    * Destroy the current WebConsoleActor instance.
    */
-  disconnect: function WCA_disconnect()
-  {
+  destroy() {
     if (this.consoleServiceListener) {
       this.consoleServiceListener.destroy();
       this.consoleServiceListener = null;
diff --git a/devtools/server/actors/worker.js b/devtools/server/actors/worker.js
index 1937229d52ee..aeb40abcc437 100644
--- a/devtools/server/actors/worker.js
+++ b/devtools/server/actors/worker.js
@@ -123,10 +123,6 @@ let WorkerActor = protocol.ActorClassWithSpec(workerSpec, {
     }
   },
 
-  disconnect() {
-    this.destroy();
-  },
-
   connect(options) {
     if (!this._attached) {
       return { error: "wrongState" };
@@ -446,10 +442,6 @@ protocol.ActorClassWithSpec(serviceWorkerRegistrationSpec, {
     this._activeWorker = null;
   },
 
-  disconnect() {
-    this.destroy();
-  },
-
   /**
    * Standard observer interface to listen to push messages and changes.
    */
diff --git a/devtools/server/main.js b/devtools/server/main.js
index 94ecde6629d3..45cb7fde66b4 100644
--- a/devtools/server/main.js
+++ b/devtools/server/main.js
@@ -1494,14 +1494,14 @@ 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
+   *        True if you don't want to destroy 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
+    // actor pool is removed, it calls the destroy 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
+    // When the destroy 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
@@ -1510,7 +1510,7 @@ DebuggerServerConnection.prototype = {
     // 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,
+    // This is a bug: if the destroy 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
diff --git a/devtools/server/tests/mochitest/test_connectToChild.html b/devtools/server/tests/mochitest/test_connectToChild.html
index 3bda7b56627e..aca2e8d3e309 100644
--- a/devtools/server/tests/mochitest/test_connectToChild.html
+++ b/devtools/server/tests/mochitest/test_connectToChild.html
@@ -42,7 +42,7 @@ function runTests() {
   let mm = iframe.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader.messageManager;
 
   // Register a test actor in the child process so that we can know if and when
-  // this fake actor is disconnected.
+  // this fake actor is destroyed.
   mm.loadFrameScript("data:text/javascript,new " + function FrameScriptScope() {
     const { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
     const { DebuggerServer } = require("devtools/server/main");
@@ -55,8 +55,8 @@ function runTests() {
     TestActor.prototype = {
       actorPrefix: "test",
 
-      disconnect: function () {
-        sendAsyncMessage("test-actor-disconnected", null);
+      destroy: function () {
+        sendAsyncMessage("test-actor-destroyed", null);
       },
       hello: function () {
         return {msg:"world"};
@@ -85,7 +85,7 @@ function runTests() {
       ok(actor.testActor, "Got the test actor");
 
       // Ensure sending at least one request to our actor,
-      // otherwise it won't be instanciated, nor be disconnected...
+      // otherwise it won't be instanciated, nor be destroyed...
       client.request({
         to: actor.testActor,
         type: "hello",
@@ -95,10 +95,10 @@ function runTests() {
         client.close();
 
         // Ensure that our test actor got cleaned up;
-        // its disconnect method should be called
-        mm.addMessageListener("test-actor-disconnected", function listener() {
-          mm.removeMessageListener("test-actor-disconnected", listener);
-          ok(true, "Actor is cleaned  up");
+        // its destroy method should be called
+        mm.addMessageListener("test-actor-destroyed", function listener() {
+          mm.removeMessageListener("test-actor-destroyed", listener);
+          ok(true, "Actor is cleaned up");
 
           secondClient(actor.testActor);
         });
diff --git a/devtools/server/tests/unit/test_longstringactor.js b/devtools/server/tests/unit/test_longstringactor.js
index 18b92891007b..18f01e363576 100644
--- a/devtools/server/tests/unit/test_longstringactor.js
+++ b/devtools/server/tests/unit/test_longstringactor.js
@@ -7,7 +7,7 @@
 const { LongStringActor } = require("devtools/server/actors/object");
 
 function run_test() {
-  test_LSA_disconnect();
+  test_LSA_destroy();
   test_LSA_grip();
   test_LSA_onSubstring();
 }
@@ -27,12 +27,12 @@ function makeMockLongStringActor()
   return actor;
 }
 
-function test_LSA_disconnect()
+function test_LSA_destroy()
 {
   let actor = makeMockLongStringActor();
   do_check_eq(actor.registeredPool.longStringActors[TEST_STRING], actor);
 
-  actor.disconnect();
+  actor.destroy();
   do_check_eq(actor.registeredPool.longStringActors[TEST_STRING], void 0);
 }