Bug 1269919 - Stop emitting newSource on the target actors. r=jdescottes
authorAlexandre Poirot <poirot.alex@gmail.com>
Thu, 21 Feb 2019 18:41:08 +0000
changeset 460430 c018b87959c2deb9c8f09aa782bfbc5620f13c21
parent 460429 001ec91a5e5fabfbfc7c36c8b9194ce9c607e51d
child 460431 68628da99bd04b059510bc5aa65d662e121ad73d
push id35593
push userccoroiu@mozilla.com
push dateFri, 22 Feb 2019 16:25:14 +0000
treeherdermozilla-central@a1d118e856dd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdescottes
bugs1269919
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1269919 - Stop emitting newSource on the target actors. r=jdescottes Now that the base Target class is managing the thread client, we no longer have to send "newSource" on the target actor, and instead, listen for newSource directly on the thread client. We should probably align updatedSource and have this event being emitted on the thread actor as well. Depends on D18813 Differential Revision: https://phabricator.services.mozilla.com/D18814
devtools/client/framework/attach-thread.js
devtools/server/actors/thread.js
devtools/shared/fronts/targets/target-mixin.js
devtools/shared/specs/targets/addon.js
devtools/shared/specs/targets/browsing-context.js
devtools/shared/specs/targets/content-process.js
devtools/shared/specs/targets/worker.js
--- a/devtools/client/framework/attach-thread.js
+++ b/devtools/client/framework/attach-thread.js
@@ -1,16 +1,15 @@
 /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
 /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const Services = require("Services");
-
 const {LocalizationHelper} = require("devtools/shared/l10n");
 const L10N = new LocalizationHelper("devtools/client/locales/toolbox.properties");
 
 function handleThreadState(toolbox, event, packet) {
   // Suppress interrupted events by default because the thread is
   // paused/resumed a lot for various actions.
   if (event === "paused" && packet.why.type === "interrupted") {
     return;
@@ -30,64 +29,55 @@ function handleThreadState(toolbox, even
       toolbox.raise();
       toolbox.selectTool("jsdebugger", packet.why.type);
     }
   } else if (event === "resumed") {
     toolbox.unhighlightTool("jsdebugger");
   }
 }
 
-function attachThread(toolbox) {
+async function attachThread(toolbox) {
   const target = toolbox.target;
-
-  const autoBlackBox = false;
-  const ignoreFrameEnvironment = true;
-
-  const threadOptions = { autoBlackBox, ignoreFrameEnvironment };
+  const threadOptions = {
+    autoBlackBox: false,
+    ignoreFrameEnvironment: true,
+  };
+  const [, threadClient] = await target.attachThread(threadOptions);
+  if (!threadClient.paused) {
+    throw new Error("Thread in wrong state when starting up, should be paused.");
+  }
 
-  return new Promise((resolve, reject) => {
-    const handleResponse = ([res, threadClient]) => {
-      if (res.error) {
-        reject(new Error("Couldn't attach to thread: " + res.error));
-        return;
-      }
-
-      threadClient.addListener("paused", handleThreadState.bind(null, toolbox));
-      threadClient.addListener("resumed", handleThreadState.bind(null, toolbox));
-
-      if (!threadClient.paused) {
-        reject(new Error("Thread in wrong state when starting up, should be paused"));
-      }
+  // These flags need to be set here because the client sends them
+  // with the `resume` request. We make sure to do this before
+  // resuming to avoid another interrupt. We can't pass it in with
+  // `threadOptions` because the resume request will override them.
+  threadClient.pauseOnExceptions(
+    Services.prefs.getBoolPref("devtools.debugger.pause-on-exceptions"),
+    Services.prefs.getBoolPref("devtools.debugger.ignore-caught-exceptions")
+  );
 
-      // These flags need to be set here because the client sends them
-      // with the `resume` request. We make sure to do this before
-      // resuming to avoid another interrupt. We can't pass it in with
-      // `threadOptions` because the resume request will override them.
-      threadClient.pauseOnExceptions(
-        Services.prefs.getBoolPref("devtools.debugger.pause-on-exceptions"),
-        Services.prefs.getBoolPref("devtools.debugger.ignore-caught-exceptions")
+  try {
+    await threadClient.resume();
+  } catch (ex) {
+    // Interpret a possible error thrown by ThreadActor.resume
+    if (ex.error === "wrongOrder") {
+      const box = toolbox.getNotificationBox();
+      box.appendNotification(
+        L10N.getStr("toolbox.resumeOrderWarning"),
+        "wrong-resume-order",
+        "",
+        box.PRIORITY_WARNING_HIGH
       );
-
-      threadClient.resume(res => {
-        if (res.error === "wrongOrder") {
-          const box = toolbox.getNotificationBox();
-          box.appendNotification(
-            L10N.getStr("toolbox.resumeOrderWarning"),
-            "wrong-resume-order",
-            "",
-            box.PRIORITY_WARNING_HIGH
-          );
-        }
-
-        resolve(threadClient);
-      });
-    };
-
-    target.attachThread(threadOptions).then(handleResponse);
-  });
+    } else {
+      throw ex;
+    }
+  }
+  threadClient.addListener("paused", handleThreadState.bind(null, toolbox));
+  threadClient.addListener("resumed", handleThreadState.bind(null, toolbox));
+  return threadClient;
 }
 
 function detachThread(threadClient) {
   threadClient.removeListener("paused");
   threadClient.removeListener("resumed");
 }
 
 module.exports = { attachThread, detachThread };
--- a/devtools/server/actors/thread.js
+++ b/devtools/server/actors/thread.js
@@ -1719,38 +1719,29 @@ const ThreadActor = ActorClassWithSpec(t
    *        A Debugger.Object instance whose referent is the global object.
    */
   onNewScript: function(script, global) {
     this._addSource(script.source);
   },
 
   /**
    * A function called when there's a new source from a thread actor's sources.
-   * Emits `newSource` on the target actor.
+   * Emits `newSource` on the thread actor.
    *
    * @param {SourceActor} source
    */
   onNewSourceEvent: function(source) {
     // Bug 1516197: New sources are likely detected due to either user
     // interaction on the page, or devtools requests sent to the server.
     // We use executeSoon because we don't want to block those operations
     // by sending packets in the middle of them.
     DevToolsUtils.executeSoon(() => {
-      const type = "newSource";
-      this.conn.send({
-        from: this._parent.actorID,
-        type,
-        source: source.form(),
-      });
-
-      // For compatibility and debugger still using `newSource` on the thread client,
-      // still emit this event here. Clean up in bug 1247084
       this.conn.send({
         from: this.actorID,
-        type,
+        type: "newSource",
         source: source.form(),
       });
     });
   },
 
   /**
    * A function called when there's an updated source from a thread actor' sources.
    * Emits `updatedSource` on the target actor.
--- a/devtools/shared/fronts/targets/target-mixin.js
+++ b/devtools/shared/fronts/targets/target-mixin.js
@@ -41,17 +41,21 @@ loader.lazyRequireGetter(this, "getFront
 function TargetMixin(parentClass) {
   class Target extends parentClass {
     constructor(client, form) {
       super(client, form);
 
       this._forceChrome = false;
 
       this.destroy = this.destroy.bind(this);
+      this._onNewSource = this._onNewSource.bind(this);
+      this._onUpdatedSource = this._onUpdatedSource.bind(this);
+
       this.activeConsole = null;
+      this.threadClient = null;
 
       this._client = client;
 
       // Cache of already created targed-scoped fronts
       // [typeName:string => Front instance]
       this.fronts = new Map();
       // Temporary fix for bug #1493131 - inspector has a different life cycle
       // than most other fronts because it is closely related to the toolbox.
@@ -374,19 +378,38 @@ function TargetMixin(parentClass) {
     async attachThread(options = {}) {
       if (!this._threadActor) {
         throw new Error("TargetMixin sub class should set _threadActor before calling " +
                         "attachThread");
       }
       const [response, threadClient] =
         await this._client.attachThread(this._threadActor, options);
       this.threadClient = threadClient;
+
+      this.threadClient.addListener("newSource", this._onNewSource);
+
+      // "updatedSource" is emitted by the thread actor, but on its parent actor.
+      // i.e. the target actor. So we have to listen on the target actor, but ideally,
+      // the actor should emit this event itself.
+      this.on("updatedSource", this._onUpdatedSource);
+
       return [response, threadClient];
     }
 
+    // Listener for "newSource" event fired by the thread actor
+    _onNewSource(type, packet) {
+      this.emit("source-updated", packet);
+    }
+
+    // Listener for "updatedSource" event fired by the thread actor in the name of the
+    // target actor
+    _onUpdatedSource(packet) {
+      this.emit("source-updated", packet);
+    }
+
     /**
      * Listen to the different events.
      */
     _setupListeners() {
       this.tab.addEventListener("TabClose", this);
       this.tab.ownerDocument.defaultView.addEventListener("unload", this);
       this.tab.addEventListener("TabRemotenessChange", this);
     }
@@ -404,33 +427,31 @@ function TargetMixin(parentClass) {
 
     /**
      * Setup listeners for remote debugging, updating existing ones as necessary.
      */
     _setupRemoteListeners() {
       this.client.addListener("closed", this.destroy);
 
       this.on("tabDetached", this.destroy);
-
-      // These events should be ultimately listened from the thread client as
-      // they are coming from it and no longer go through the Target Actor/Front.
-      this._onSourceUpdated = packet => this.emit("source-updated", packet);
-      this.on("newSource", this._onSourceUpdated);
-      this.on("updatedSource", this._onSourceUpdated);
     }
 
     /**
      * Teardown listeners for remote debugging.
      */
     _teardownRemoteListeners() {
       // Remove listeners set in _setupRemoteListeners
       this.client.removeListener("closed", this.destroy);
       this.off("tabDetached", this.destroy);
-      this.off("newSource", this._onSourceUpdated);
-      this.off("updatedSource", this._onSourceUpdated);
+
+      // Remove listeners set in attachThread
+      if (this.threadClient) {
+        this.threadClient.removeListener("newSource", this._onNewSource);
+        this.off("updatedSource", this._onUpdatedSource);
+      }
 
       // Remove listeners set in attachConsole
       if (this.activeConsole && this._onInspectObject) {
         this.activeConsole.off("inspectObject", this._onInspectObject);
       }
     }
 
     /**
--- a/devtools/shared/specs/targets/addon.js
+++ b/devtools/shared/specs/targets/addon.js
@@ -1,14 +1,14 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
-const {Option, RetVal, generateActorSpec} = require("devtools/shared/protocol");
+const {RetVal, generateActorSpec} = require("devtools/shared/protocol");
 
 const addonTargetSpec = generateActorSpec({
   typeName: "addonTarget",
 
   methods: {
     attach: {
       request: {},
       response: RetVal("json"),
@@ -25,20 +25,11 @@ const addonTargetSpec = generateActorSpe
       request: {},
       response: {},
     },
     push: {
       request: {},
       response: RetVal("json"),
     },
   },
-
-  events: {
-    // newSource is being sent by ThreadActor in the name of its parent,
-    // i.e. AddonTargetActor
-    newSource: {
-      type: "newSource",
-      source: Option(0, "json"),
-    },
-  },
 });
 
 exports.addonTargetSpec = addonTargetSpec;
--- a/devtools/shared/specs/targets/browsing-context.js
+++ b/devtools/shared/specs/targets/browsing-context.js
@@ -131,19 +131,15 @@ const browsingContextTargetSpecPrototype
       type: "tabDetached",
       // This is to make browser_dbg_navigation.js to work as it expect to
       // see a packet object when listening for tabDetached
       from: Option(0, "string"),
     },
     workerListChanged: {
       type: "workerListChanged",
     },
-    newSource: {
-      type: "newSource",
-      source: Option(0, "json"),
-    },
   },
 };
 
 const browsingContextTargetSpec = generateActorSpec(browsingContextTargetSpecPrototype);
 
 exports.browsingContextTargetSpecPrototype = browsingContextTargetSpecPrototype;
 exports.browsingContextTargetSpec = browsingContextTargetSpec;
--- a/devtools/shared/specs/targets/content-process.js
+++ b/devtools/shared/specs/targets/content-process.js
@@ -1,14 +1,14 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
-const {types, Option, RetVal, generateActorSpec} = require("devtools/shared/protocol");
+const {types, RetVal, generateActorSpec} = require("devtools/shared/protocol");
 
 types.addDictType("contentProcessTarget.workers", {
   error: "nullable:string",
   workers: "nullable:array:workerTarget",
 });
 
 const contentProcessTargetSpec = generateActorSpec({
   typeName: "contentProcessTarget",
@@ -16,21 +16,15 @@ const contentProcessTargetSpec = generat
   methods: {
     listWorkers: {
       request: {},
       response: RetVal("contentProcessTarget.workers"),
     },
   },
 
   events: {
-    // newSource is being sent by ThreadActor in the name of its parent,
-    // i.e. ContentProcessTargetActor
-    newSource: {
-      type: "newSource",
-      source: Option(0, "json"),
-    },
     workerListChanged: {
       type: "workerListChanged",
     },
   },
 });
 
 exports.contentProcessTargetSpec = contentProcessTargetSpec;
--- a/devtools/shared/specs/targets/worker.js
+++ b/devtools/shared/specs/targets/worker.js
@@ -1,14 +1,14 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
-const {Option, Arg, RetVal, generateActorSpec} = require("devtools/shared/protocol");
+const {Arg, RetVal, generateActorSpec} = require("devtools/shared/protocol");
 
 const workerTargetSpec = generateActorSpec({
   typeName: "workerTarget",
 
   methods: {
     attach: {
       request: {},
       response: RetVal("json"),
@@ -31,18 +31,12 @@ const workerTargetSpec = generateActorSp
 
   events: {
     // WorkerTargetActor still uses old sendActorEvent function,
     // but it should use emit instead.
     // Do not emit a `close` event as Target class emit this event on destroy
     "worker-close": {
       type: "close",
     },
-    // newSource is being sent by ThreadActor in the name of its parent,
-    // i.e. WorkerTargetActor
-    newSource: {
-      type: "newSource",
-      source: Option(0, "json"),
-    },
   },
 });
 
 exports.workerTargetSpec = workerTargetSpec;