1035206 - Implement a set of actors for worker debugging

Status: RESOLVED WONTFIX

(DevTools :: Debugger, defect, P2)

Description

[Eddy Bruel [:ejpbruel]]

We need to extend the debugger server protocol with a set of worker actors that allow a client to:
- Get a list of workers
- Get notified when the list of workers changes
- Attach to/detach from a worker

As part of this task, we need to be able to run the debugger server in a worker, and implement a transport that can talk to it.

When this bug is complete, the client/server part of our implementation of worker debugging should be done.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Implement TabActor.listWorkers

This patch extends TabActor so that it can handle requests to list all workers in the corresponding tab, and send notifications to the client if the list changes. I've reused the live list implementation that we use in RootActor, and generalised it so that we can filter the list by different criteria. This will allow us to reuse the live list for shared workers and list workers later on.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Implement WorkerActor.attach

This patch implements WorkerActor, and the logic for handling attach and detach requests (without actually attaching to a worker yet, the next patch will take care of that).

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Implement WorkerDebuggerActor.attach

This patch implements the logic for setting up a connection to a debugger server in a worker. I've put in plenty of comments so hopefully the code will be self explanatory. The main logic is in DebuggerServer.connectToWorkerThread.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8451714 [details] [diff] [review]
Implement TabActor.listWorkers

Review of attachment 8451714 [details] [diff] [review]:
-----------------------------------------------------------------

I spotted a few issues, but in general this looks very nice.

::: toolkit/devtools/client/dbg-client.jsm
@@ +218,5 @@
>    "newScript": "newScript",
>    "newSource": "newSource",
>    "tabDetached": "tabDetached",
>    "tabListChanged": "tabListChanged",
> +  "workerListChanged": "workerListChanged",

We no longer need to keep adding these. Ever since bug 797621 landed, you can use:

connection.sendActorEvent(actorID, "workerListChanged", eventObject)

::: toolkit/devtools/server/actors/webbrowser.js
@@ +877,5 @@
>  
> +  onListWorkers: function (aRequest) {
> +    if (this.exited) {
> +      return { type: "exited" };
> +    }

I think the state check that we need to do here is:

if (!this.attached) {
  return { error: "wrongState" };
}

The request should fail in both "detached" and "exited" states.

@@ +898,5 @@
> +      this.conn.addActorPool(this._workerActorPool);
> +      this._workerList.onListChanged = this._onWorkerListChanged;
> +      return {
> +        "from": this.actorID,
> +        "workers": [actor.form() for (actor of actors)],

Better use the ES6 style for comprehensions.

@@ +907,5 @@
> +  _onWorkerListChanged: function () {
> +    this.conn.send({
> +      from: this.actorID,
> +      type: "workerListChanged",
> +    });

This can now become:
this.conn.sendActorEvent(this.actorID, "workerListChanged");

@@ +1173,5 @@
>    "detach": TabActor.prototype.onDetach,
>    "reload": TabActor.prototype.onReload,
>    "navigateTo": TabActor.prototype.onNavigateTo,
> +  "reconfigure": TabActor.prototype.onReconfigure,
> +  "listWorkers": TabActor.prototype.onListWorkers,

Not your fault, but can we keep this sorted, please?

::: toolkit/devtools/server/actors/worker.js
@@ +7,5 @@
> +"use strict"
> +
> +const { Ci } = require("chrome");
> +const Services = require("Services");
> +const { ActorPool } = require("devtools/server/actors/common");

You don't seem to be using ActorPool anywhere.

@@ +42,5 @@
> +  let e = Services.wdm.getEnumerator();
> +  while (e.hasMoreElements()) {
> +    let dbg = e.getNext().QueryInterface(Ci.nsIWorkerDebugger);
> +    if (matchWorkerDebugger(dbg, options)) {
> +      yield dbg;

Any particular reason you didn't use the new function* syntax? The existing similar code was there before support for the new syntax was added IIRC.

@@ +76,5 @@
> +  this._options = options;
> +  this._actors = new Map();
> +  this._actorsWillChange = false;
> +  this._onListChanged = null;
> +  this._onListChangedDidFire = false;

I think it would be a good thing to maintain naming consistency with webbrowser.js. Also, _mustNotify, _notifyListChanged seem more clear to me.

@@ +108,5 @@
> +
> +  this._actorsWillChange = false;
> +  this._onListChangedDidFire = false;
> +
> +  return Promise.resolve([actor for (actor of this._actors.values())]);

You meant to use Promise.all here, right?

Also, we now support the ES6 comprehension style, don't we?

@@ +111,5 @@
> +
> +  return Promise.resolve([actor for (actor of this._actors.values())]);
> +};
> +
> +Object.defineProperty(WorkerList.prototype, "onListChanged", {

This should be made enumerable and configurable.

@@ +123,5 @@
> +    if (this._onListChanged !== onListChanged) {
> +      if (onListChanged === null) {
> +        Services.wdm.removeListener(this);
> +      } else if (typeof onListChanged === "function") {
> +        Services.wdm.addListener(this);

Since one of these 2 options is guaranteed to be true, I would omit the second conditional as a micro-optimization, since it is implied (probably add it as a comment).

@@ +136,5 @@
> +    this._handleListChange();
> +  }
> +});
> +
> +// Called by nsIWorkerDebuggerManager when a worker debugger is registered.

Nit: /** */ style for method comments in this file please.

@@ +161,5 @@
> +  this._handleListChange();
> +};
> +
> +// Notify the client that the list changed, if necessary.
> +WorkerList.prototype._handleListChange = function () {

I believe that copying BrowserTabList.prototype._notifyListChanged verbatim would be better in this case. Better yet move the function in actors/common.js and use it from both places.

::: toolkit/devtools/server/tests/mochitest/helpers.js
@@ +60,5 @@
> +    });
> +  });
> +}
> +
> +function listWorkers(tabClient, url) {

url is not used in this function.

@@ +81,5 @@
> +
> +function waitForWorkerListChanged(client) {
> +  return new Promise(function (resolve) {
> +    client.addListener("workerListChanged", function onWorkerListChanged() {
> +      client.removeListener("workerListChanged", onWorkerListChanged);

You can use addOneTimeListener and avoid the removeListener call.

::: toolkit/devtools/server/tests/mochitest/test_TabActor.listWorkers.html
@@ +5,5 @@
> +        <title>Test for TabActor.listWorkers</title>
> +        <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
> +        <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +        <script type="application/javascript;version=1.8" src="helpers.js"></script>
> +        <script type="application/javascript;version=1.8">

I don't think you need the version specifiers any more.

@@ +13,5 @@
> +const TAB_URL = BASE_URL + "TabActor.listWorkers-tab.html";
> +const WORKER1_URL = "TabActor.listWorkers-worker1.js";
> +const WORKER2_URL = "TabActor.listWorkers-worker2.js";
> +
> +function test() {

function*

::: toolkit/modules/Services.jsm
@@ +93,5 @@
>  #ifdef MOZ_METRO
>    ["metro", "@mozilla.org/windows-metroutils;1", "nsIWinMetroUtils"],
>  #endif
>  #endif
> +  ["wdm", "@mozilla.org/dom/workers/workerdebuggermanager;1", "nsIWorkerDebuggerManager"],

This needs approval from a toolkit peer.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8451716 [details] [diff] [review]
Implement WorkerActor.attach

Review of attachment 8451716 [details] [diff] [review]:
-----------------------------------------------------------------

This needs rebasing, but I have a question about the "attach" request and a few other comments.

::: toolkit/devtools/client/dbg-client.jsm
@@ +218,5 @@
>    "newScript": "newScript",
>    "newSource": "newSource",
>    "tabDetached": "tabDetached",
>    "tabListChanged": "tabListChanged",
> +  "workerDetached": "workerDetached",

Again, no reason for this any more. Just use sendActorEvent.

@@ +259,5 @@
>  
>    // Map actor ID to client instance for each actor type.
> +  this._tabClients = new Map;
> +  this._addonClients = new Map;
> +  this._workerClients = new Map;

This will need rebasing on top of bug 797621. There is a single map now and things are much simplified in general.

@@ +526,5 @@
> +   * @param function aOnResponse
> +   *        Called with the response packet and a WorkerClient
> +   *        (which will be undefined on error).
> +   */
> +  attachWorker: function DC_attachWorker(aWorkerActor, aOnResponse) {

Nit: we no longer use names for function expressions in these cases, since the engine can reliably infer them.

@@ +530,5 @@
> +  attachWorker: function DC_attachWorker(aWorkerActor, aOnResponse) {
> +    let packet = {
> +      to: aWorkerActor,
> +      type: "attach"
> +    }

This should try to reuse any existing worker clients from previous attach requests, just like attachThread does. The App Manager needs this.

@@ +1388,5 @@
> + *        The debugger client parent.
> + * @param aForm object
> + *        The protocol form for this worker.
> + */
> +function WorkerClient(aClient, aForm) {

Nit: since there is no need for the entire form, the constructor should take the actor ID as an argument.

::: toolkit/devtools/server/actors/worker.js
@@ +57,5 @@
>  function WorkerActor(connection, dbg) {
>    this._connection = connection;
>    this._dbg = dbg;
> +  this._isClosed = false;
> +  this._isAttached = false;

Nit: _closed & _attached

@@ +96,5 @@
> +
> +  this._attach();
> +
> +  return {
> +    type: "workerAttached",

"workerAttached" is redundant, "attached" should be enough. But isn't this method going to pause the worker thread, so that breakpoints can be set and sources can be retrieved? If so, it would need to return a "paused" packet, just like the main thread actor.

::: toolkit/devtools/server/tests/mochitest/chrome.ini
@@ +15,5 @@
>    TabActor.listWorkers-tab.html
>    TabActor.listWorkers-worker1.js
>    TabActor.listWorkers-worker2.js
> +  WorkerActor.attach-tab.html
> +  WorkerActor.attach-worker.js

No point in copying existing files verbatim if you are not going to modify them. Just refer to the existing ones.

::: toolkit/devtools/server/tests/mochitest/helpers.js
@@ +90,5 @@
>  
> +function attachWorker(client, workerActor) {
> +  return new Promise(function (resolve) {
> +    client.attachWorker(workerActor, function (response, workerClient) {
> +      resolve(workerClient);

All these helpers should reject the promise when response.error is returned.

@@ +98,5 @@
> +
> +function detachWorker(workerClient) {
> +  return new Promise(function (resolve) {
> +    workerClient.detach(function (response) {
> +      resolve(!response.error);

A more thorough pattern for all these helpers:
response.error ? reject(response.error) : resolve();

::: toolkit/devtools/server/tests/mochitest/test_WorkerActor.attach.html
@@ +22,5 @@
> +
> +  let tab = yield addTab(Services.wm.getMostRecentWindow("navigator:browser"),
> +                         TAB_URL);
> +  let tabActor = yield findTab(client, TAB_URL);
> +  let tabClient = yield attachTab(client, tabActor);

I wonder if we should make attachTab smarter, so that it could handle the findTab call internally if the argument is a URL. It seems that this addTab/findTab/attachTab pattern keeps popping up.

@@ +29,5 @@
> +  tab.linkedBrowser.contentWindow.postMessage("create " + WORKER_URL, "*");
> +  yield waitForWorkerListChanged(client);
> +  let workerActor = yield findWorker(tabClient, WORKER_URL);
> +
> +  let workerClient = yield attachWorker(client, workerActor);

The same could be done for attachWorker and findWorker if it's going to become common.

@@ +43,5 @@
> +
> +  tab.linkedBrowser.contentWindow.postMessage("destroy " + WORKER_URL, "*");
> +  yield waitForWorkerDetached(client, workerActor);
> +  ok(!(yield attachWorker(client, workerActor)),
> +     "should not be able to attach to closed worker");

Nit: "...to *a* closed worker"

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8451793 [details] [diff] [review]
Implement WorkerDebuggerActor.attach

Review of attachment 8451793 [details] [diff] [review]:
-----------------------------------------------------------------

Do we have a test for multiple simultaneous connections to a worker debugger? We need one.

::: toolkit/devtools/client/dbg-client.jsm
@@ +1392,5 @@
>  function WorkerClient(aClient, aForm) {
>    this.client = aClient;
>    this._actor = aForm.from;
> +  this._threadActor = aForm.threadActor;
> +  this.thread = null;

Ah, so you do need the entire form object after all. Ignore my previous comment.

@@ +1412,5 @@
> +   *        (which will be undefined on error).
> +   */
> +  attachThread: function(aOptions={}, aOnResponse) {
> +    if (this.thread) {
> +      setTimeout(() => aOnResponse({}, this.thread), 0);

At some point we should convert all of these with your DevToolsUtils.executeSoon, but it doesn't have to be now.

@@ +1424,5 @@
> +    };
> +    this.request(packet, (aResponse) => {
> +      if (!aResponse.error) {
> +        this.thread = new ThreadClient(this, this._threadActor);
> +        this.client._threadClients.set(this._threadActor, this.thread);

Needs rebasin'. We have the registerClient API now.

::: toolkit/devtools/server/actors/script.js
@@ +4983,5 @@
> +
> +update(WorkerDebuggerActor.prototype, {
> +  constructor: WorkerDebuggerActor,
> +
> +  actorPrefix: "workerThread",

Better settle on a single name, either WorkerThreadActor or workerDebugger before this becomes another ThreadActor/context embarrassment :)

@@ +4987,5 @@
> +  actorPrefix: "workerThread",
> +
> +  globalManager: {
> +    findGlobals: function () {
> +      this.globalDebugObject = this._addDebuggees(this.global);

Just call this.addDebuggee directly, since there are no iframes involved here.

@@ +4991,5 @@
> +      this.globalDebugObject = this._addDebuggees(this.global);
> +    },
> +
> +    onNewGlobal: function (aGlobal) {
> +      throw new Error("should only have one global per worker!");

Add a dump call too (like in uncaughtExceptionHook), because I'm not sure this exception will be reported anywhere.

::: toolkit/devtools/server/actors/worker.js
@@ +82,3 @@
>  // Called by nsIWorkerDebugger when the worker debugger is disabled.
>  WorkerActor.prototype.onClosed = function (dbg) {
> +  this._dbg.removeListener(this);

Shouldn't this move to _detach? Otherwise client.close() will trigger actor.disconnect() for all actors, and the listener won't be removed in that case.

@@ +95,5 @@
> +WorkerActor.prototype.onError = function (message, filename, line, lineNumber,
> +                                          columnNumber) {
> +  let string = "ERROR:" + filename + ":" + lineNumber + ":" + message + "\n";
> +  dump(string);
> +  reportError(string);

I assume columnNumber is always 0 in workers? If not you should definitely add it to the logged message.

@@ +127,5 @@
>  };
>  
>  WorkerActor.prototype._attach = function () {
> +  if (this._transport) {
> +    return promise.resolve(true);

This reminds me that I saw a Promise identifier in the previous patch somewhere and I had forgotten that you map Promise to promise in the loader.

::: toolkit/devtools/server/main.js
@@ +668,5 @@
> +    this._checkInit();
> +
> +    let transport = new WorkerDebuggerServerTransport(port, prefix);
> +    return this._onConnection(transport, prefix, true);
> +  },

This looks like it could be easily merged with connectToParent, if there was a type check on the arguments. No need to introduce needless complexity.

@@ +670,5 @@
> +    let transport = new WorkerDebuggerServerTransport(port, prefix);
> +    return this._onConnection(transport, prefix, true);
> +  },
> +
> +  connectToWorkerThread: function (connection, dbg) {

This method is in dire need of a descriptive comment that includes a mention of what it returns. Also, connectToWorker seems more concise and equally descriptive, no?

@@ +691,5 @@
> +      type: "connect",
> +      prefix: prefix,
> +    }));
> +
> +    return new Promise((resolve) => {

This seems prone to races, no? The message is posted before the listener is in place, so presumably it could be delivered before that.

@@ +699,5 @@
> +
> +          // The debugger was closed before it had a chance to initialize its
> +          // end of the connection. Return null to indicate that the connection
> +          // failed to be established.
> +          resolve(null);

What are the circumstances under which this could happen? I'm not sure I have the full picture here. Is it because of the race that I mentioned?

@@ +702,5 @@
> +          // failed to be established.
> +          resolve(null);
> +        },
> +        onMessage: (message) => {
> +          let packet = JSON.parse(message);

This could throw and not be reported anywhere. Use makeInfallible please.

@@ +731,5 @@
> +                  prefix: prefix,
> +                }));
> +              }
> +
> +              // Cancel the forwarding for actors which id starts with |prefix|.

Typo: "whose id starts with"

::: toolkit/devtools/server/tests/mochitest/test_WorkerDebuggerActor.attach.html
@@ +31,5 @@
> +  let workerActor = yield findWorker(tabClient, WORKER_URL);
> +  let workerClient = yield attachWorker(client, workerActor);
> +  let threadClient = yield attachThread(workerClient);
> +  ok(threadClient,
> +     "should be able to attach to thread");

Nit: "to *the* thread"

::: toolkit/devtools/server/worker.js
@@ +15,5 @@
> +const { DebuggerServer } = worker.require("devtools/server/main");
> +
> +// The debugger script may be loaded more than once for the same worker, so
> +// initialize the server unless it has already been initialized.
> +if (!DebuggerServer.initialized) {

Then maybe we want to move the rest of the require() calls above into the conditional branch?

@@ +25,5 @@
> +  };
> +
> +  // Set up an event listener to listen for connect/disconnect messages.
> +  addEventListener("message", function onMessage(event) {
> +    let packet = JSON.parse(event.data);

If parsing throws is the error logged anywhere? Why don't you wrap the callback with DevToolsUtils.makeInfallible?

@@ +27,5 @@
> +  // Set up an event listener to listen for connect/disconnect messages.
> +  addEventListener("message", function onMessage(event) {
> +    let packet = JSON.parse(event.data);
> +    switch (packet.type) {
> +    case "connect":

Nit: case statements are indented more than the switch statement.

::: toolkit/devtools/transport/transport.js
@@ +738,5 @@
> +  this._dbg = dbg;
> +  this._prefix = prefix;
> +}
> +
> +WorkerDebuggerClientTransport.prototype.ready = function () {

Could you avoid deviating from the existing style in this file where the entire prototype object is set once, instead of each property separately? Makes the code easier to compare and follow.

@@ +763,5 @@
> +  throw new Error("can't send bulk data to a worker thread!");
> +};
> +
> +// Called by nsIWorkerDebugger when a message is received.
> +WorkerDebuggerClientTransport.prototype.onMessage = function (message) {

This needs makeInfallible, too.

@@ +765,5 @@
> +
> +// Called by nsIWorkerDebugger when a message is received.
> +WorkerDebuggerClientTransport.prototype.onMessage = function (message) {
> +  let packet = JSON.parse(message);
> +  if (packet.type !== "message" || packet.prefix != this._prefix) {

I think we should send back unrecognizedPacketType in the first case, what do you think?

@@ +790,5 @@
> + *
> + * On the main thread, WorkerDebuggerClientTransport should be used instead
> + * (see above).
> + */
> +function WorkerDebuggerServerTransport(port, prefix) {

Why do we need different transports for client and server? They seem to be almost identical and it's not something we do in any other transport.

::: toolkit/devtools/worker-loader.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +(function () {

Why the IIFE? Is this loaded more than once in some scope? At any rate, a comment above it would be good.

@@ +292,3 @@
>  
>    // Create a sandbox with the given name and prototype.
> +  const createSandbox = function createSandbox(name, prototype) {

No need to name the function expression, SpiderMonkey can infer them now.

@@ +343,5 @@
> +
> +  XPCInspector.prototype.enterNestedEventLoop = function (requestor) {
> +    this._requestors.push(requestor);
> +    this._self.enterEventLoop();
> +  };

If you look at nsIJSInspector.idl, both enterNestedEventLoop and exitNestedEventLoop must return the new eventLoopNestLevel.

@@ +368,5 @@
> +    globals: {
> +      "isWorker": true,
> +      "Debugger": self.Debugger,
> +      "dump": self.dump,
> +      "Promise": self.Promise,

Alex moved some of these globals to modules, so this needs rebasing.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

This was great stuff, kudos! It looks very close to ready.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #4)
> @@ +123,5 @@
> > +    if (this._onListChanged !== onListChanged) {
> > +      if (onListChanged === null) {
> > +        Services.wdm.removeListener(this);
> > +      } else if (typeof onListChanged === "function") {
> > +        Services.wdm.addListener(this);
> 
> Since one of these 2 options is guaranteed to be true, I would omit the
> second conditional as a micro-optimization, since it is implied (probably
> add it as a comment).

dbg_assert on the condition seems like a good fit.

Comment 9

[Eddy Bruel [:ejpbruel]]

(In reply to Panos Astithas [:past] from comment #4)
> Comment on attachment 8451714 [details] [diff] [review]
> Implement TabActor.listWorkers
> 
> Review of attachment 8451714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I spotted a few issues, but in general this looks very nice.
> 
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +218,5 @@
> >    "newScript": "newScript",
> >    "newSource": "newSource",
> >    "tabDetached": "tabDetached",
> >    "tabListChanged": "tabListChanged",
> > +  "workerListChanged": "workerListChanged",
> 
> We no longer need to keep adding these. Ever since bug 797621 landed, you
> can use:
> 
> connection.sendActorEvent(actorID, "workerListChanged", eventObject)
> 

Turns out this is broken, and doesn't actually work. I'll include a fix in my upcoming patch.

> ::: toolkit/devtools/server/actors/webbrowser.js
> @@ +877,5 @@
> >  
> > +  onListWorkers: function (aRequest) {
> > +    if (this.exited) {
> > +      return { type: "exited" };
> > +    }
> 
> I think the state check that we need to do here is:
> 
> if (!this.attached) {
>   return { error: "wrongState" };
> }
> 
> The request should fail in both "detached" and "exited" states.
> 

Actually, we agreed on irc that it makes sense to be able to list workers on an unattached tab. This doesn't cost us anything, since we don't set up any listeners until the first call to listWorkers, and the listener is removed the first time workerListChanged is fired.

What *doesnt* make sense however, is listing workers on a tab that no longer exist. Hence that check.

> @@ +898,5 @@
> > +      this.conn.addActorPool(this._workerActorPool);
> > +      this._workerList.onListChanged = this._onWorkerListChanged;
> > +      return {
> > +        "from": this.actorID,
> > +        "workers": [actor.form() for (actor of actors)],
> 
> Better use the ES6 style for comprehensions.
> 

I wasn't aware that ES6 adopted a different syntax for comprehensions. Will do.

> @@ +907,5 @@
> > +  _onWorkerListChanged: function () {
> > +    this.conn.send({
> > +      from: this.actorID,
> > +      type: "workerListChanged",
> > +    });
> 
> This can now become:
> this.conn.sendActorEvent(this.actorID, "workerListChanged");
> 

Fine by me.

> @@ +1173,5 @@
> >    "detach": TabActor.prototype.onDetach,
> >    "reload": TabActor.prototype.onReload,
> >    "navigateTo": TabActor.prototype.onNavigateTo,
> > +  "reconfigure": TabActor.prototype.onReconfigure,
> > +  "listWorkers": TabActor.prototype.onListWorkers,
> 
> Not your fault, but can we keep this sorted, please?
>

What do you mean 'keep sorted'? This list wasn't sorted in the first place :-P (nor are the actual methods). But, I agree that having them sorted is cleaner. I'll include that in the next patch.
 
> ::: toolkit/devtools/server/actors/worker.js
> @@ +7,5 @@
> > +"use strict"
> > +
> > +const { Ci } = require("chrome");
> > +const Services = require("Services");
> > +const { ActorPool } = require("devtools/server/actors/common");
> 
> You don't seem to be using ActorPool anywhere.
> 

Good catch, I must have forgotten to remove that.

> @@ +42,5 @@
> > +  let e = Services.wdm.getEnumerator();
> > +  while (e.hasMoreElements()) {
> > +    let dbg = e.getNext().QueryInterface(Ci.nsIWorkerDebugger);
> > +    if (matchWorkerDebugger(dbg, options)) {
> > +      yield dbg;
> 
> Any particular reason you didn't use the new function* syntax? The existing
> similar code was there before support for the new syntax was added IIRC.
>

I was still familiarizing myself with Task.js when I wrote these tests and didn't know about the existence of the new function* syntax until later on. I must have forgotten to update it. Again, good catch :-)
 
> @@ +76,5 @@
> > +  this._options = options;
> > +  this._actors = new Map();
> > +  this._actorsWillChange = false;
> > +  this._onListChanged = null;
> > +  this._onListChangedDidFire = false;
> 
> I think it would be a good thing to maintain naming consistency with
> webbrowser.js. Also, _mustNotify, _notifyListChanged seem more clear to me.
> 

I actually prefer these names myself. Would you mind if I changed the ones in webbrowser.js to match these? Or do you feel strongly about this?

> @@ +108,5 @@
> > +
> > +  this._actorsWillChange = false;
> > +  this._onListChangedDidFire = false;
> > +
> > +  return Promise.resolve([actor for (actor of this._actors.values())]);
> 
> You meant to use Promise.all here, right?
> 
> Also, we now support the ES6 comprehension style, don't we?
> 

I don't think so actually. Isn't Promise.all supposed to take an array of promises? My intention was to resolve the promise to an array of worker actors. Am I missing something here?

> @@ +111,5 @@
> > +
> > +  return Promise.resolve([actor for (actor of this._actors.values())]);
> > +};
> > +
> > +Object.defineProperty(WorkerList.prototype, "onListChanged", {
> 
> This should be made enumerable and configurable.
>

Sure.

> @@ +123,5 @@
> > +    if (this._onListChanged !== onListChanged) {
> > +      if (onListChanged === null) {
> > +        Services.wdm.removeListener(this);
> > +      } else if (typeof onListChanged === "function") {
> > +        Services.wdm.addListener(this);
> 
> Since one of these 2 options is guaranteed to be true, I would omit the
> second conditional as a micro-optimization, since it is implied (probably
> add it as a comment).
>

Fine by me.
 
> @@ +136,5 @@
> > +    this._handleListChange();
> > +  }
> > +});
> > +
> > +// Called by nsIWorkerDebuggerManager when a worker debugger is registered.
> 
> Nit: /** */ style for method comments in this file please.
>

You had no problem with me adopting the // style for worker-loader.js. I really don't care what style we adopt, but I'd like to know up front what style I should use where. Do we have any conventions for this? If not, can we decide on some? :)
 
> @@ +161,5 @@
> > +  this._handleListChange();
> > +};
> > +
> > +// Notify the client that the list changed, if necessary.
> > +WorkerList.prototype._handleListChange = function () {
> 
> I believe that copying BrowserTabList.prototype._notifyListChanged verbatim
> would be better in this case. Better yet move the function in
> actors/common.js and use it from both places.
> 

Why is that? I looked at the how BrowserTabList handles the onListChanged notification, and I found its semantics to be a bit unclear. For instance, what happens if BrowserTabList needs to send a notification, and currently has no handler set, but then somebody assigns a handler to it? In my opinion, the pending notification should be sent right away in that case. I went through some care to make sure edge cases like this are handled properly.

I'm all in favor of unifying these functions and moving them to common.js, but given the above, I'd like to propose that we take the version used by WorkerActor instead.

> ::: toolkit/devtools/server/tests/mochitest/helpers.js
> @@ +60,5 @@
> > +    });
> > +  });
> > +}
> > +
> > +function listWorkers(tabClient, url) {
> 
> url is not used in this function.
> 

Oops.

> @@ +81,5 @@
> > +
> > +function waitForWorkerListChanged(client) {
> > +  return new Promise(function (resolve) {
> > +    client.addListener("workerListChanged", function onWorkerListChanged() {
> > +      client.removeListener("workerListChanged", onWorkerListChanged);
> 
> You can use addOneTimeListener and avoid the removeListener call.
> 
> ::: toolkit/devtools/server/tests/mochitest/test_TabActor.listWorkers.html
> @@ +5,5 @@
> > +        <title>Test for TabActor.listWorkers</title>
> > +        <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
> > +        <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> > +        <script type="application/javascript;version=1.8" src="helpers.js"></script>
> > +        <script type="application/javascript;version=1.8">
> 
> I don't think you need the version specifiers any more.
>

Unfortunately, it turns out we do.
 
> @@ +13,5 @@
> > +const TAB_URL = BASE_URL + "TabActor.listWorkers-tab.html";
> > +const WORKER1_URL = "TabActor.listWorkers-worker1.js";
> > +const WORKER2_URL = "TabActor.listWorkers-worker2.js";
> > +
> > +function test() {
> 
> function*
> 
> ::: toolkit/modules/Services.jsm
> @@ +93,5 @@
> >  #ifdef MOZ_METRO
> >    ["metro", "@mozilla.org/windows-metroutils;1", "nsIWinMetroUtils"],
> >  #endif
> >  #endif
> > +  ["wdm", "@mozilla.org/dom/workers/workerdebuggermanager;1", "nsIWorkerDebuggerManager"],
> 
> This needs approval from a toolkit peer.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Eddy Bruel [:ejpbruel] from comment #9)
> (In reply to Panos Astithas [:past] from comment #4)
> > The request should fail in both "detached" and "exited" states.
> 
> Actually, we agreed on irc that it makes sense to be able to list workers on
> an unattached tab. This doesn't cost us anything, since we don't set up any
> listeners until the first call to listWorkers, and the listener is removed
> the first time workerListChanged is fired.
> 
> What *doesnt* make sense however, is listing workers on a tab that no longer
> exist. Hence that check.

I can't remember this discussion, could you summarize the use cases that this would support? In all the other cases I'm familiar with, the act of detaching from an actor indicates that there is no longer a need to hold on to resources, until a client attaches again. The difference from the exited state is that in the latter it is not even possible to attach again.

> > @@ +76,5 @@
> > > +  this._options = options;
> > > +  this._actors = new Map();
> > > +  this._actorsWillChange = false;
> > > +  this._onListChanged = null;
> > > +  this._onListChangedDidFire = false;
> > 
> > I think it would be a good thing to maintain naming consistency with
> > webbrowser.js. Also, _mustNotify, _notifyListChanged seem more clear to me.
> > 
> 
> I actually prefer these names myself. Would you mind if I changed the ones
> in webbrowser.js to match these? Or do you feel strongly about this?

Jim added the webbrowser.js names, so it appears that the votes so far are 2 to 1 :-) But I won't insist on this, if others agree with you.

However, my preference in this case is not based on popularity metrics:
- mustNotify is shorter than actorsWillChange and additionally indicates why we need to store that value
- mustNotify is solely used for the cases where you have both actorsWillChange and onListChangedDidFire
- notifyListChanged is more descriptive compared to handleListChange in that it explains how the list change will be handled

I'm certainly interested though in any contrarian arguments on code clarity; more on that below.

> > You meant to use Promise.all here, right?
> 
> I don't think so actually. Isn't Promise.all supposed to take an array of
> promises? My intention was to resolve the promise to an array of worker
> actors. Am I missing something here?

No, you are right. I was confused.

> > > +// Called by nsIWorkerDebuggerManager when a worker debugger is registered.
> > 
> > Nit: /** */ style for method comments in this file please.
> >
> 
> You had no problem with me adopting the // style for worker-loader.js. I
> really don't care what style we adopt, but I'd like to know up front what
> style I should use where. Do we have any conventions for this? If not, can
> we decide on some? :)

O RLY?

https://bugzilla.mozilla.org/show_bug.cgi?id=1003095#c10

As a general rule of thumb though: if the different style is only in code *you* wrote, the convention should be pretty clear :)

> > @@ +161,5 @@
> > > +  this._handleListChange();
> > > +};
> > > +
> > > +// Notify the client that the list changed, if necessary.
> > > +WorkerList.prototype._handleListChange = function () {
> > 
> > I believe that copying BrowserTabList.prototype._notifyListChanged verbatim
> > would be better in this case. Better yet move the function in
> > actors/common.js and use it from both places.
> > 
> 
> Why is that? I looked at the how BrowserTabList handles the onListChanged
> notification, and I found its semantics to be a bit unclear. For instance,
> what happens if BrowserTabList needs to send a notification, and currently
> has no handler set, but then somebody assigns a handler to it? In my
> opinion, the pending notification should be sent right away in that case. I
> went through some care to make sure edge cases like this are handled
> properly.
> 
> I'm all in favor of unifying these functions and moving them to common.js,
> but given the above, I'd like to propose that we take the version used by
> WorkerActor instead.

I don't think a notification has to be sent if the event happened while nobody was listening. The code must take care to set the onListChanged handler every time there is some entity interested in these events. I think Jim's comments are pretty clear on that:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/root.js#55

Such events would only be of interest after a list iteration, which would return the updated actors anyway. Why would a previously triggered event be useful?


Agreed on all the rest.

Comment 11

[Eddy Bruel [:ejpbruel]]

(In reply to Panos Astithas [:past] from comment #10)
> (In reply to Eddy Bruel [:ejpbruel] from comment #9)
> > (In reply to Panos Astithas [:past] from comment #4)
> > > The request should fail in both "detached" and "exited" states.
> > 
> > Actually, we agreed on irc that it makes sense to be able to list workers on
> > an unattached tab. This doesn't cost us anything, since we don't set up any
> > listeners until the first call to listWorkers, and the listener is removed
> > the first time workerListChanged is fired.
> > 
> > What *doesnt* make sense however, is listing workers on a tab that no longer
> > exist. Hence that check.
> 
> I can't remember this discussion, could you summarize the use cases that
> this would support? In all the other cases I'm familiar with, the act of
> detaching from an actor indicates that there is no longer a need to hold on
> to resources, until a client attaches again. The difference from the exited
> state is that in the latter it is not even possible to attach again.
> 

Since workers can have child workers, WorkerActor will also support a listWorkers request. It would be nice if we could recursively list all workers in a tab and their descendants without having to attach to each worker, so I wanted to keep attaching and listing workers as separate actions. In my mind, attaching to a worker is something you only want to do if you have the intention of debugging it. For TabActor, that distinction makes less sense, but it's probably worthwhile to keep symmetry with WorkerActor here.

> > > @@ +76,5 @@
> > > > +  this._options = options;
> > > > +  this._actors = new Map();
> > > > +  this._actorsWillChange = false;
> > > > +  this._onListChanged = null;
> > > > +  this._onListChangedDidFire = false;
> > > 
> > > I think it would be a good thing to maintain naming consistency with
> > > webbrowser.js. Also, _mustNotify, _notifyListChanged seem more clear to me.
> > > 
> > 
> > I actually prefer these names myself. Would you mind if I changed the ones
> > in webbrowser.js to match these? Or do you feel strongly about this?
> 
> Jim added the webbrowser.js names, so it appears that the votes so far are 2
> to 1 :-) But I won't insist on this, if others agree with you.
> 

I'll see if I can convince Jim. If not, I'll happily go with your suggestion.

> However, my preference in this case is not based on popularity metrics:
> - mustNotify is shorter than actorsWillChange and additionally indicates why
> we need to store that value
> - mustNotify is solely used for the cases where you have both
> actorsWillChange and onListChangedDidFire
> - notifyListChanged is more descriptive compared to handleListChange in that
> it explains how the list change will be handled
> 
> I'm certainly interested though in any contrarian arguments on code clarity;
> more on that below.

I would counter that notifyListChanged is a bit misleading, since it doesn't *always* notify the client. Perhaps it should have been more appropriately called maybeNotifyListChanged or notifyListChangedIfNeeded, but that's a bit too nitpicky for my own taste.

More generally, I have really bad experiences working with code that used name length as the only metric when picking variables names (i.e. all variables were single letters). As a result, I have a strong preference for minimizing ambiguity over minimizing name length (in this case, handleListChange may have been more accurate, but also less descriptive, so I'm not sure if it was a good choice).

The point that mustNotify indicates why a value must be stored is an interesting one, and is something I've never really took into consideration when picking variables name. I'm inclined to agree with you on that one. My preference has always been to name variable names so that the condition checks in which they are used are as self documenting as I can make them (*why* do I need to notify? because the list of actors did change).

In any case, I'll let Jim make the call on what direction we want to take this. Either keep the old names for both Actors, or adopt the new ones I coined up. Jim, what's your opinion on this?

> 
> > > You meant to use Promise.all here, right?
> > 
> > I don't think so actually. Isn't Promise.all supposed to take an array of
> > promises? My intention was to resolve the promise to an array of worker
> > actors. Am I missing something here?
> 
> No, you are right. I was confused.
> 
> > > > +// Called by nsIWorkerDebuggerManager when a worker debugger is registered.
> > > 
> > > Nit: /** */ style for method comments in this file please.
> > >
> > 
> > You had no problem with me adopting the // style for worker-loader.js. I
> > really don't care what style we adopt, but I'd like to know up front what
> > style I should use where. Do we have any conventions for this? If not, can
> > we decide on some? :)
> 
> O RLY?
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1003095#c10
> 
> As a general rule of thumb though: if the different style is only in code
> *you* wrote, the convention should be pretty clear :)
> 

Ouch. I burned myself pretty badly there, didn't I? :)

You're absolutely right, I'll adopt the existing code style.

> > > @@ +161,5 @@
> > > > +  this._handleListChange();
> > > > +};
> > > > +
> > > > +// Notify the client that the list changed, if necessary.
> > > > +WorkerList.prototype._handleListChange = function () {
> > > 
> > > I believe that copying BrowserTabList.prototype._notifyListChanged verbatim
> > > would be better in this case. Better yet move the function in
> > > actors/common.js and use it from both places.
> > > 
> > 
> > Why is that? I looked at the how BrowserTabList handles the onListChanged
> > notification, and I found its semantics to be a bit unclear. For instance,
> > what happens if BrowserTabList needs to send a notification, and currently
> > has no handler set, but then somebody assigns a handler to it? In my
> > opinion, the pending notification should be sent right away in that case. I
> > went through some care to make sure edge cases like this are handled
> > properly.
> > 
> > I'm all in favor of unifying these functions and moving them to common.js,
> > but given the above, I'd like to propose that we take the version used by
> > WorkerActor instead.
> 
> I don't think a notification has to be sent if the event happened while
> nobody was listening. The code must take care to set the onListChanged
> handler every time there is some entity interested in these events. I think
> Jim's comments are pretty clear on that:
> 
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> root.js#55
> 
> Such events would only be of interest after a list iteration, which would
> return the updated actors anyway. Why would a previously triggered event be
> useful?
> 

Yeah, it might not be important. Perhaps I overengineered this part a bit. I saw some code where the semantics didn't seem well defined, and tried to narrow them down, even though there is nothing reliant on that.

Perhaps it's best if we let Jim make the call on this one as well?

> 
> Agreed on all the rest.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Eddy Bruel [:ejpbruel] from comment #11)
> Since workers can have child workers, WorkerActor will also support a
> listWorkers request. It would be nice if we could recursively list all
> workers in a tab and their descendants without having to attach to each
> worker, so I wanted to keep attaching and listing workers as separate
> actions. In my mind, attaching to a worker is something you only want to do
> if you have the intention of debugging it. For TabActor, that distinction
> makes less sense, but it's probably worthwhile to keep symmetry with
> WorkerActor here.

Ah child workers, that makes sense. Could you add this comment to the code as well?

Comment 13

[Eddy Bruel [:ejpbruel]]

(In reply to Panos Astithas [:past] from comment #12)
> (In reply to Eddy Bruel [:ejpbruel] from comment #11)
> > Since workers can have child workers, WorkerActor will also support a
> > listWorkers request. It would be nice if we could recursively list all
> > workers in a tab and their descendants without having to attach to each
> > worker, so I wanted to keep attaching and listing workers as separate
> > actions. In my mind, attaching to a worker is something you only want to do
> > if you have the intention of debugging it. For TabActor, that distinction
> > makes less sense, but it's probably worthwhile to keep symmetry with
> > WorkerActor here.
> 
> Ah child workers, that makes sense. Could you add this comment to the code
> as well?

Sure thing!

Comment 14

[Eddy Bruel [:ejpbruel]]

Attachment: Fix a bug in the sendActorEvent API

This patch fixes some bugs in the sendActorEvent API from bug 797621. I'm filing this patch separately because all the other patches will be blocked on bug 757133 for a long time, and this patch doesn't actually depend on that.

Note that this is just a quick fix. For a proper solution, I've filed bug 1042642. See that bug for details of what's going on.

Comment 15

[Eddy Bruel [:ejpbruel]]

Attachment: Replace occurrences of setTimeout in dbg-client.jsm with executeSoon

This patch does exactly what the title suggests. I'm again filing this separately because it doesn't depend on any of the stuff in bug 757133, and I'd like to land what I can.

Comment 16

[Eddy Bruel [:ejpbruel]]

Attachment: Implement TabActor.listWorkers

Note that I didn't address some of the style comments yet because I still need to get jimb's vote on those. I will of course change those once we make a decision one way or the other.

Comment 17

[Eddy Bruel [:ejpbruel]]

Attachment: Implement WorkerActor.attach

I *think* I addressed all of your review comments, but there were quite a few of them. My apologies up front if I missed any of them. Feel free to point them out again in that case.

Comment 18

[Eddy Bruel [:ejpbruel]]

Attachment: Implement WorkerThreadActor.attach

You mentioned a couple of times that you were unsure if certain errors would be reported, so I'd like to clear up how error handling *should* work in a worker debugger: if a worker debugger script encounters an uncaught exception, the worker debugger should call the error handler defined in worker.js, which in turn will report the error to the console and dump it to the terminal.

Comment 19

[Eddy Bruel [:ejpbruel]]

Attachment: Implement WorkerThreadActor.attach

Looks like I forgot to uncomment some lines in the test. Still seems to work though.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8460846 [details] [diff] [review]
Replace occurrences of setTimeout in dbg-client.jsm with executeSoon

Review of attachment 8460846 [details] [diff] [review]:
-----------------------------------------------------------------

You should now be able to remove the Timer.jsm import statement, too.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8460858 [details] [diff] [review]
Implement TabActor.listWorkers

Review of attachment 8460858 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/webbrowser.js
@@ +905,5 @@
> +  },
> +
> +  _onWorkerListChanged: function () {
> +    this.conn.sendActorEvent(this.actorID, "workerListChanged");
> +    this._workerList.onListChanged = null;

It would be safer to nullify the callback before sending the event, to guarantee that the event listeners won't have a chance to observe an intermediate state.

::: toolkit/devtools/server/actors/worker.js
@@ +161,5 @@
> +  this._actorsWillChange = true;
> +  this._handleListChange();
> +};
> +
> +/** 

Trailing whitespace.

::: toolkit/modules/Services.jsm
@@ +92,5 @@
>  #ifdef MOZ_METRO
>    ["metro", "@mozilla.org/windows-metroutils;1", "nsIWinMetroUtils"],
>  #endif
>  #endif
> +  ["wdm", "@mozilla.org/dom/workers/workerdebuggermanager;1", "nsIWorkerDebuggerManager"],

r? :Mossop?

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8460860 [details] [diff] [review]
Implement WorkerActor.attach

Review of attachment 8460860 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/client/dbg-client.jsm
@@ +504,5 @@
> +   * @param function aOnResponse
> +   *        Called with the response packet and a WorkerClient
> +   *        (which will be undefined on error).
> +   */
> +  attachWorker: function (aWorkerActor, aOnResponse) {

Use the "noop" function as the default value for aOnResponse as elsewhere in this file.

::: toolkit/devtools/server/actors/worker.js
@@ +102,5 @@
> +
> +  this._attach();
> +
> +  return {
> +    type: "workerAttached",

I still prefer "attached" to "workerAttached".

::: toolkit/devtools/server/tests/mochitest/helpers.js
@@ +13,5 @@
> +  return promise.then(function () {
> +    return true;
> +  }, function () {
> +    return false;
> +  });

Or even:
return promise.then(() => true, () => false);

@@ +132,5 @@
>  
> +function detachWorker(workerClient) {
> +  return new Promise(function (resolve, reject) {
> +    workerClient.detach(function (response) {
> +      response.error ? reject(response.error) : resolve();

Nit: I know you are not using the returned value currently, but using "resolve(response)" or "resolve({})" would allow call sites to also use the familiar "if (!result.error)" pattern without unexpected "result is undefined" errors.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8460905 [details] [diff] [review]
Implement WorkerThreadActor.attach

Review of attachment 8460905 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/client/dbg-client.jsm
@@ +1473,5 @@
> +   * @param function aOnResponse
> +   *        Called with the response packet and a ThreadClient
> +   *        (which will be undefined on error).
> +   */
> +  attachThread: function(aOptions={}, aOnResponse) {

Default value of "noop" for aOnResponse please.

@@ +1475,5 @@
> +   *        (which will be undefined on error).
> +   */
> +  attachThread: function(aOptions={}, aOnResponse) {
> +    if (this.thread) {
> +      setTimeout(() => aOnResponse({}, this.thread), 0);

In the other patch you convert all of these to executeSoon :-)

::: toolkit/devtools/server/actors/script.js
@@ +5060,5 @@
> +
> +  actorPrefix: "workerThread",
> +
> +  globalManager: {
> +    findGlobals: function () {

This will need rebasing after bug 1039952 lands.

::: toolkit/devtools/server/actors/worker.js
@@ +97,1 @@
>    if (this._detach()) {

You could just call disconnect() here, if you made it return the result of _detach().

@@ +139,5 @@
> +  if (this._transport) {
> +    return promise.resolve(true);
> +  }
> +
> +  dump("CONNECTING TO WORKER...\n");

Forgotten debug code?

@@ +147,5 @@
> +    this._threadActor = result.threadActor;
> +    return true;
> +  }, () => {
> +    return false;
> +  });

Or simply: () => false

::: toolkit/devtools/server/main.js
@@ +685,5 @@
> +      type: "connect",
> +      prefix: prefix,
> +    }));
> +
> +    return new Promise((resolve, reject) => {

Lowercase promise, unless you like mixing promise implementations.

::: toolkit/devtools/server/tests/mochitest/helpers.js
@@ +140,5 @@
>  
> +function attachThread(workerClient, options) {
> +  return new Promise(function (resolve) {
> +    workerClient.attachThread(options, function (response, threadClient) {
> +      resolve(threadClient);

This also needs the test for error response and reject or resolve accordingly.

::: toolkit/devtools/server/tests/mochitest/test_WorkerThreadActor.attach.html
@@ +46,5 @@
> +      ok(true, "should receive paused notification for thread 1");
> +      is(packet.type, "paused");
> +      is(packet.why.type, "debuggerStatement");
> +
> +      threadClient2.addOneTimeListener("paused", function (event, packet) {

I'm sure this works, bit it would be better if the test didn't rely on a particular delivery order for the paused events.

::: toolkit/devtools/transport/transport.js
@@ +727,5 @@
> + * main thread and worker thread, respectively. |prefix| should be a string
> + * that is included in each message to distinguish between multiple connections
> + * to the same worker.
> + *
> + * This transport exhanges messages of the form { type: "message",

Typo: "exchanges"

::: toolkit/devtools/worker-loader.js
@@ +155,5 @@
>      try {
>        loadInSandbox(url, sandbox);
>      } catch (error) {
> +      if (String(error).indexOf("Script file not found") >= 0) {
> +        dump(new Error().stack + "\n");

Forgotten debug code?

Comment 24

[Eddy Bruel [:ejpbruel]]

 https://hg.mozilla.org/integration/fx-team/rev/15a5d2dcdb90

Comment 25

[Eddy Bruel [:ejpbruel]]

 https://hg.mozilla.org/integration/fx-team/rev/1fe3eed021cb

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15a5d2dcdb90
https://hg.mozilla.org/mozilla-central/rev/1fe3eed021cb

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Still more to land.

Comment 28

[Jim Blandy :jimb]

> > I don't think a notification has to be sent if the event happened while
> > nobody was listening. The code must take care to set the onListChanged
> > handler every time there is some entity interested in these events. I think
> > Jim's comments are pretty clear on that:
> > 
> > http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> > root.js#55
> > 
> > Such events would only be of interest after a list iteration, which would
> > return the updated actors anyway. Why would a previously triggered event be
> > useful?
> > 
> 
> Yeah, it might not be important. Perhaps I overengineered this part a bit. I
> saw some code where the semantics didn't seem well defined, and tried to
> narrow them down, even though there is nothing reliant on that.

I don't think notifying the live list's user of changes that occurred before the onListChanged handler was set should be necessary. If the list's user hasn't iterated over the list, it can't possibly have out-of-date information, so it can't need to be notified.

The design goal for live lists is to let the client cache lists (tabs; workers; and so on) without the caches going stale. It's ultra-lame when a developer has to hit 'refresh' to get the current list of something; displays should be live and current. The live list design is supposed to enable live displays while still minimizing the server's need to observe/listen/etc. It sends notifications only for lists that have actually been requested; and even then only a single notification. Once that notification has been sent, the server can unplug all its observers/listeners/etc., since the client has been told that its cache is no longer current. And if the client has lost interest, it can just 

Perhaps it's just my inexperience, but when I was writing chrome JS, I had a hard time reassuring myself that I wasn't leaving observers around that I didn't need. Those can accumulate, slow down the system, leak, and cause weird behavior. The 'live list' thing was my attempt to make something so simple to use that I couldn't screw it up. Live lists turn out to be hard to implement, but my hope was that it'd be easier to test a simple interface than a complicated one.

Comment 29

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8460858 [details] [diff] [review]
Implement TabActor.listWorkers

Requesting additional review from Mossop for the Services.jsm part, just in case.

Comment 30

[Dave Townsend [:mossop]]

Comment on attachment 8460858 [details] [diff] [review]
Implement TabActor.listWorkers

Review of attachment 8460858 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Services.jsm
@@ +92,5 @@
>  #ifdef MOZ_METRO
>    ["metro", "@mozilla.org/windows-metroutils;1", "nsIWinMetroUtils"],
>  #endif
>  #endif
> +  ["wdm", "@mozilla.org/dom/workers/workerdebuggermanager;1", "nsIWorkerDebuggerManager"],

We generally only add stuff here that is likely to see widespread use. Here I see this entry only used three times in the same file and seems like we could just add a shortcut to that file instead. Anything I'm missing?

Comment 31

[Dave Townsend [:mossop]]

Comment on attachment 8460858 [details] [diff] [review]
Implement TabActor.listWorkers

Review of attachment 8460858 [details] [diff] [review]:
-----------------------------------------------------------------

Which I guess is me saying you can do without this for now unless you want to give me a stronger rationale and re-request review.

Comment 32

[Eddy Bruel [:ejpbruel]]

This bug is horribly outdated because the platform API took so long to land. I'm going to close this bug, and open new ones to land the server, client, and UI parts of the worker debugger, respectively.

For an overview of the work going on in this area, please refer to bug 1003097.

Comment 33

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.