:ochameau noticed that after bug 1177279, we are emitting duplicate newSource events. I believe there were added as an attempt at compatibility, but as :ochameau says, we only need to support new client with old servers, not the other way. So, the client should listen in both possible event locations, but the server should emit in only one place. From comments in bug 1247084: ---- I'm trying to speedup the browser toolbox, and I see tons of newSource messages. Like double-tons of them This bug seems to be about that. These duplicated messages seems to be introduced in bug 1177279, which is recent. Could we get rid of them? Is this only to support older clients? We are not supporting old client on a more recent server, only the opposite. So we could get rid of it... I'm talking about this: https://hg.mozilla.org/mozilla-central/annotate/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/devtools/server/actors/script.js#l1903 onSourceEvent: function (name, source) { this.conn.send({ from: this._parent.actorID, type: name, source: source.form() }); // For compatibility and debugger still using `newSource` on the thread client, // still emit this event here. Clean up in bug 1247084 if (name === "newSource") { this.conn.send({ from: this.actorID, type: name, source: source.form() }); } ----

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #0) > :ochameau noticed that after bug 1177279, we are emitting duplicate > newSource events. > > I believe there were added as an attempt at compatibility, but as :ochameau > says, we only need to support new client with old servers, not the other > way. So, the client should listen in both possible event locations, but the > server should emit in only one place. > > From comments in bug 1247084: > > ---- > I'm trying to speedup the browser toolbox, and I see tons of newSource > messages. Like double-tons of them > > This bug seems to be about that. > These duplicated messages seems to be introduced in bug 1177279, which is > recent. > > Could we get rid of them? Is this only to support older clients? We are not > supporting old client on a more recent server, only the opposite. So we > could get rid of it... > > I'm talking about this: > https://hg.mozilla.org/mozilla-central/annotate/ > 21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/devtools/server/actors/script. > js#l1903 > onSourceEvent: function (name, source) { > this.conn.send({ > from: this._parent.actorID, > type: name, > source: source.form() > }); > > // For compatibility and debugger still using `newSource` on the thread > client, > // still emit this event here. Clean up in bug 1247084 > if (name === "newSource") { > this.conn.send({ > from: this.actorID, > type: name, > source: source.form() > }); > } > ---- TLDR: if you want to get rid of this redundant notification, we need to fix the sources story for workers. This is related to the question of whether source actors belong to the thread actor or the tab actor. Previously, source actors belonged to the thread actor, so we emitted the new source notification there. At some point, we decided that source actors should belong to the tab actor instead, and we started to emit the new source notification there. The reason that we still need to emit the new source notification on the thread actor is that we never completed this migration of source actor from the thread to the tab actor. Source actors are owned by the TabSources object. This object may live on the tab actor, but it still depends on the thread actor to notify it when new sources are introduced. The main reason for this is that the the thread actor owns the Debugger instance from which we these notifications are obtained. One way to remove this dependency would be to give the tab actor its own Debugger instance. However, things are further complicated by workers. Currently, we treat workers as separate tabs: each worker has a thread actor, but also a mock tab actor, which contains the TabSources object for the workers. However, this mock tab actor is never exposed to the client, which only ever sees the thread actor for each worker. If we would remove the new source notification from the thread actor, we would currently have no way to be notify the client of new sources in workers. We could expose the mock tab actor object for each worker, and treat them as fully fledged tabs. However, since we eventually want to move towards an integrated UI for worker debugging, the preferred solution would be to include the sources for each worker in the TabSources object for the tab that owns them. This is not completely trivial: not only would each worker have to notify its tab when a new source is introduced, we'd also need to gracefully handle the case where the worker (and therefore, all source actors introduced by that worker) goes away.

I'm not following every detail of this very complex half done refactoring, but you seem to confirm that it is not related to support client connected to older server/actors? Today client codebase expect these events to be fired twice?

Joe what do you want to do for 48?

I'm going to defer to Eddy's P3, so - Not for for 48.

This bug has a priority set that is not P1, so it's not an urgent fix.

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/001ec91a5e5f Unify all the attachThread methods on Target class. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/c018b87959c2 Stop emitting newSource on the target actors. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/68628da99bd0 Prevent test harness from creating target on teardown. r=jdescottes