899052 - Implement add-on thread actors

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Comment 1

[Eddy Bruel [:ejpbruel]]

I was pondering the question whether we also need a concept of addon actors, in
addition to addon thread actors. The main rationale for having such actors would
be that addons can be enabled/disabled, and we want the client to be able to do
so remotely. Moreover, addon actors should be able to notify the client when they
are enabled/disabled. Compare this to how tab actors allow the client to reload or
navigate tabs, and notify the client of navigation events.

For SDK add-ons at least, when the add-on is disabled, the function shutdown()
in bootstrap.js is called, which nukes all sandboxes. According to MDN, nuking
a sandbox forces the it to be freed immediately. If the add-on was being
debugged, the debugger is holding a weak reference to all sandboxes, so this
shouldn't be a problem. If the add-on is later enabled again, any new globals
created by the add-on are again added as debuggees by the add-on thread actor
via its global manager, so this shouldn't be a problem either.

Based on the above, addon thread actors should be able to deal with the
disabling/enabling of addons without any special handling. However, when I
looked at the code for tab actors, I noticed that it contained special handling
to update the list of debuggees in the tab thread actor whenever a pageload
finishes. Again based on the above, shouldn't the global manager be able to
deal with that situation on its own? The debugger only holds weak references
to the globals from the previous page, and any globals for the new page can
be detected via the onNewGlobal callback.

I feel like I am missing something, so my question is why we need this special
handling for tab actors, and if we also need something similar for addon thread
actors. And if we don't, does it still make sense to have a concept of addon
actors? Or should we just make the root actor reply to the listAddons message
by sending back a list of addon thread actors to which the client can attach?

Comment 2

[Jim Blandy :jimb]

The way the global actor is wired up in the current code doesn't seem really optimal to me; I don't think the add-on actor should imitate the content tab actor / content thread actor structure too closely. Yes, I think the add-on actor could handle these issues itself.

When deciding which actors to expose, we should only be thinking about the conversations clients might need to have with them, and the lifetimes we might want potential child actors to have, because those are the only aspects of actors that are visible across the protocol. Considerations like global tracking, which we can handle on the server without bothering the client, shouldn't have any effect on the protocol.

So as to whether we want to have separate add-on actors and add-on thread actors:

Are there messages we want to exchange with add-on actors that wouldn't really make sense to send to the thread-like actor? If so, then having separate add-on actors and add-on thread actors would make sense.

Another question: would it be valuable to have an actor whose lifetime matches that of the add-on? That is, an actor which is destroyed when the add-on is disabled, and a new actor is created when it's enabled? Such actors' lifetimes would be supersets of the add-on thread actors, which would be destroyed when we tell the UI that we're no longer debugging that add-on.

If the answers to both questions are "no", then there's no need for add-on actors distinct from the add-on thread actors.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Promisify the live list interface

To get a list of installed add-ons we have to use AddonManager.getAllAddons. However, that function is inherently asynchronous, whereas the live list interface is inherently synchronous (i.e. based on iterators).

To work around this, I suggest promisifying the live list interface, so we can use it for both the listTabs and listAddons requests.

Comment 4

[Dave Camp (:dcamp)]

Since it's a live list, would it be reasonable to just return an empty list and then notify of changes when the promise resolves?

Comment 5

[Dave Camp (:dcamp)]

I guess that would make the first request to list always be empty, which isn't so nice.

Comment 6

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

Comment on attachment 788240 [details] [diff] [review]
Promisify the live list interface

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

r+ with green try push. (Eddy: you only need to test xpcshell and mochitest-bc).

(In reply to Dave Camp (:dcamp) from comment #4)
> Since it's a live list, would it be reasonable to just return an empty list
> and then notify of changes when the promise resolves?

As you mention, always returning an empty list would be meh. I'd rather turn tab stuff into unsolicited notifications. But then we also need to figure out how to handle selected state if we switch to that style, right?

Either way, I don't think that this is the right bug for these changes. If either of you feel strongly about it, file a follow up.

Comment 7

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> I'd rather turn tab stuff into unsolicited notifications.

And to clarify, I mean with no initial request to start receiving them. Not sure if I was clear originally.

Comment 8

[Jim Blandy :jimb]

At the protocol level, what I like about the one-shot notifications, which live lists are meant to support, is that they don't require any subscribe/unsubscribe machinery; you don't need to define deltas, and make sure they're complete and accurate; and they can't flood. You get live displays, built on something really dumb and simple.

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: patch

This patch implements the listAddons request on the root actor, which allows clients to obtain a live list of actors installed addons. The addon actors themselves are still vacuous: attach/detach logic for these will be implemented in a subsequent patch.

Note that each addon actor registers itself as a listener on the addon manager and thus receives notifications for *all* addons (there is no way to  register a listener for a specific addon). This may be inefficient in theory, but should be ok in practice, since the number of addon events is expected to be small.

I've also added listeners on the root client for list change notifications, because it seemed like those were missing.

Comment 10

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

Comment on attachment 789096 [details] [diff] [review]
patch

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

I have a few questions below, it is quite possible that you did things this way for a reason that I am missing (in which case we should consider whether it deserves a comment), otherwise please change those parts of the code. Will r+ (with nits fixed) if there is a good reason for those things.

::: browser/devtools/debugger/test/browser_dbg_listaddons.js
@@ +36,5 @@
> +      }
> +      ok(!addonListChanged, "Should not yet be notified that list of addons changed.");
> +      ok(gAddon1Actor, "Should find an addon actor for addon1.");
> +      test_second_addon();
> +    }); 

nit: trailing space here

@@ +62,5 @@
> +      ok(addonListChanged, "Should be notified that list of addons changed.");
> +      ok(foundAddon1, "Should find an addon actor for addon1.");
> +      ok(gAddon2Actor, "Should find an actor for addon2.");
> +      test_remove_addon();
> +    }); 

nit: another trailing space

@@ +76,5 @@
> +        if (addon.url == ADDON1_URL) {
> +          foundAddon1 = true;
> +        }
> +      }
> +      ok(!foundAddon1, "Addon1 should be gone");

Should we test that we receive an addonListChanged event fires when we remove an addon?

::: browser/devtools/debugger/test/head.js
@@ +18,5 @@
>  Cu.import("resource://gre/modules/devtools/dbg-client.jsm", tempScope);
>  Cu.import("resource:///modules/source-editor.jsm", tempScope);
>  Cu.import("resource:///modules/devtools/gDevTools.jsm", tempScope);
>  Cu.import("resource://gre/modules/devtools/Loader.jsm", tempScope);
> +Cu.import("resource://gre/modules/AddonManager.jsm", tempScope);

Don't you have to unpack AddonManager from tempScope after this? (let AddonManager = tempScope.AddonManager) That's what all the other imports are doing, and I don't see why this would be different. Maybe I'm just confused.

::: toolkit/devtools/client/dbg-client.jsm
@@ +1062,5 @@
> +   * Called when the root actor notifies that the list of addons has been changed.
> +   */
> +  onAddonListChanged: function RC_onAddonListChanged(aEvent, aResponse) {
> +    this.notify("addonListChanged", aResponse);
> +  }

Why do we have to proxy events from the debugger client to the root client? Can't we just listen on the debugger client directly like we do for the other unsolicited notifications? This feels like unnecessary code.

::: toolkit/devtools/server/actors/root.js
@@ +286,5 @@
> +
> +    return addonList.getList().then((addonActors) => {
> +      let addonActorPool = new ActorPool(this.conn);
> +      for (let addonActor of addonActors) {
> +          addonActorPool.addActor(addonActor);

nit: 2 spaces indent pls

@@ +290,5 @@
> +          addonActorPool.addActor(addonActor);
> +      }
> +
> +      if (this._addonActorPool) {
> +        this.conn.removeActorPool(this._addonActorPool);

Why are we blowing away and re-creating `this._addonActorPool` on every "listAddons" request? Wouldn't it make more sense to re-use the same one and only add the addon actor to the pool if `!this._addonActorPool.has(addonActor)`?

::: toolkit/devtools/server/actors/webbrowser.js
@@ +837,5 @@
> +        let actor = this._actorByAddonId.get(addon.id);
> +        if (!actor) {
> +            actor = new BrowserAddonActor(this._connection, addon);
> +            this._actorByAddonId.set(addon.id, actor);
> +        }

nit: 2 spaces indent pls

@@ +855,5 @@
> +    this._onListChanged = v;
> +    if (this._onListChanged) {
> +      AddonManager.addAddonListener(this);
> +    } else {
> +      AddonManager.removeAddonListener(this);

Cool

@@ +886,5 @@
> +  get url() {
> +    return this._addon.sourceURI ? this._addon.sourceURI.spec : undefined;
> +  },
> +
> +  grip: function BAA_grip() {

s/grip/form/ please

Forms are how we represent a "thing" over the RDP, grips are how we represent js values over the RDP. (We don't get this right 100% of the time on the server already, but I'd like to make sure we don't worsen the problem).

I know it is confusing terminology, and personally I think we should rename all these grip and form methods to toJSON, but that is for a different bug, and I'm not sure Jim would agree or not.

Comment 11

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #10)
> Comment on attachment 789096 [details] [diff] [review]
> patch
> 
> Review of attachment 789096 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a few questions below, it is quite possible that you did things this
> way for a reason that I am missing (in which case we should consider whether
> it deserves a comment), otherwise please change those parts of the code.
> Will r+ (with nits fixed) if there is a good reason for those things.
> 
> ::: browser/devtools/debugger/test/browser_dbg_listaddons.js
> @@ +36,5 @@
> > +      }
> > +      ok(!addonListChanged, "Should not yet be notified that list of addons changed.");
> > +      ok(gAddon1Actor, "Should find an addon actor for addon1.");
> > +      test_second_addon();
> > +    }); 
> 
> nit: trailing space here
> 
> @@ +62,5 @@
> > +      ok(addonListChanged, "Should be notified that list of addons changed.");
> > +      ok(foundAddon1, "Should find an addon actor for addon1.");
> > +      ok(gAddon2Actor, "Should find an actor for addon2.");
> > +      test_remove_addon();
> > +    }); 
> 
> nit: another trailing space
> 
> @@ +76,5 @@
> > +        if (addon.url == ADDON1_URL) {
> > +          foundAddon1 = true;
> > +        }
> > +      }
> > +      ok(!foundAddon1, "Addon1 should be gone");
> 
> Should we test that we receive an addonListChanged event fires when we
> remove an addon?
> 
> ::: browser/devtools/debugger/test/head.js
> @@ +18,5 @@
> >  Cu.import("resource://gre/modules/devtools/dbg-client.jsm", tempScope);
> >  Cu.import("resource:///modules/source-editor.jsm", tempScope);
> >  Cu.import("resource:///modules/devtools/gDevTools.jsm", tempScope);
> >  Cu.import("resource://gre/modules/devtools/Loader.jsm", tempScope);
> > +Cu.import("resource://gre/modules/AddonManager.jsm", tempScope);
> 
> Don't you have to unpack AddonManager from tempScope after this? (let
> AddonManager = tempScope.AddonManager) That's what all the other imports are
> doing, and I don't see why this would be different. Maybe I'm just confused.
> 
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +1062,5 @@
> > +   * Called when the root actor notifies that the list of addons has been changed.
> > +   */
> > +  onAddonListChanged: function RC_onAddonListChanged(aEvent, aResponse) {
> > +    this.notify("addonListChanged", aResponse);
> > +  }
> 
> Why do we have to proxy events from the debugger client to the root client?
> Can't we just listen on the debugger client directly like we do for the
> other unsolicited notifications? This feels like unnecessary code.
> 
> ::: toolkit/devtools/server/actors/root.js
> @@ +286,5 @@
> > +
> > +    return addonList.getList().then((addonActors) => {
> > +      let addonActorPool = new ActorPool(this.conn);
> > +      for (let addonActor of addonActors) {
> > +          addonActorPool.addActor(addonActor);
> 
> nit: 2 spaces indent pls
> 
> @@ +290,5 @@
> > +          addonActorPool.addActor(addonActor);
> > +      }
> > +
> > +      if (this._addonActorPool) {
> > +        this.conn.removeActorPool(this._addonActorPool);
> 
> Why are we blowing away and re-creating `this._addonActorPool` on every
> "listAddons" request? Wouldn't it make more sense to re-use the same one and
> only add the addon actor to the pool if
> `!this._addonActorPool.has(addonActor)`?

That's not what listTabs does either, probably because that would never get rid
of actors to tabs/addons that no longer exist.

> 
> ::: toolkit/devtools/server/actors/webbrowser.js
> @@ +837,5 @@
> > +        let actor = this._actorByAddonId.get(addon.id);
> > +        if (!actor) {
> > +            actor = new BrowserAddonActor(this._connection, addon);
> > +            this._actorByAddonId.set(addon.id, actor);
> > +        }
> 
> nit: 2 spaces indent pls
> 
> @@ +855,5 @@
> > +    this._onListChanged = v;
> > +    if (this._onListChanged) {
> > +      AddonManager.addAddonListener(this);
> > +    } else {
> > +      AddonManager.removeAddonListener(this);
> 
> Cool
> 
> @@ +886,5 @@
> > +  get url() {
> > +    return this._addon.sourceURI ? this._addon.sourceURI.spec : undefined;
> > +  },
> > +
> > +  grip: function BAA_grip() {
> 
> s/grip/form/ please
> 
> Forms are how we represent a "thing" over the RDP, grips are how we
> represent js values over the RDP. (We don't get this right 100% of the time
> on the server already, but I'd like to make sure we don't worsen the
> problem).
> 
> I know it is confusing terminology, and personally I think we should rename
> all these grip and form methods to toJSON, but that is for a different bug,
> and I'm not sure Jim would agree or not.

Comment 12

[Eddy Bruel [:ejpbruel]]

Attachment: Implement the listAddons request

Comment 13

[Eddy Bruel [:ejpbruel]]

Attachment: Implement the listAddons request

Comment 14

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

> That's not what listTabs does either, probably because that would never
> get rid of actors to tabs/addons that no longer exist.

Ah ok, makes sense.

Comment 15

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

Comment on attachment 789537 [details] [diff] [review]
Implement the listAddons request

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

r+ with green try push

Just a note: its helpful for me as a reviewer if you include a brief list of things you changed from the last patch. A lot of the feedback I had were questions, not necessarily "you must do it this way" statements, because I wasn't sure if I was missing something or not (like re-creating actor pools). Because of that, it is nice to see what was stuff that you were like "oh yeah, good point; I should do that" and what was "no, it has to be this was cause of X". I know you commented about the actor pools, but just a short list (even just "updated based on feedback from the review" tells me that you decided the suggested changes were worth adding and there wasn't some unknown-to-me-reason). And now, in my quest to properly explain myself, it seems like this is a way bigger deal than it actually is.

::: toolkit/devtools/client/dbg-client.jsm
@@ +1054,3 @@
>  };
>  
> +eventSource(RootClient.prototype);

I don't think we need this anymore since we don't proxy the events anymore, right?

Comment 16

[Eddy Bruel [:ejpbruel]]

Attachment: Implement minimal addon actors

Comment 17

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #15)
> Comment on attachment 789537 [details] [diff] [review]
> Implement the listAddons request
> 
> Review of attachment 789537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with green try push
> 
> Just a note: its helpful for me as a reviewer if you include a brief list of
> things you changed from the last patch. A lot of the feedback I had were
> questions, not necessarily "you must do it this way" statements, because I
> wasn't sure if I was missing something or not (like re-creating actor
> pools). Because of that, it is nice to see what was stuff that you were like
> "oh yeah, good point; I should do that" and what was "no, it has to be this
> was cause of X". I know you commented about the actor pools, but just a
> short list (even just "updated based on feedback from the review" tells me
> that you decided the suggested changes were worth adding and there wasn't
> some unknown-to-me-reason). And now, in my quest to properly explain myself,
> it seems like this is a way bigger deal than it actually is.
>

Sure thing Nick!

By the way, the previous patch implements a minimal addon actor that you can attach to using the chrome debugger actor. Once Mike's metadata API lands, we can replace this chrome debugger actor with something more sophisticated, which only attaches to globals labeled as being part of a specific addon.

Comment 18

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

Comment on attachment 791265 [details] [diff] [review]
Implement minimal addon actors

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

r+ with green try push

::: browser/devtools/debugger/test/Makefile.in
@@ +133,5 @@
>  	browser_dbg_tab1.html \
>  	browser_dbg_tab2.html \
>  	browser_dbg_addon1.xpi \
>  	browser_dbg_addon2.xpi \
> +	browser_dbg_addon3.xpi \

It looks like the actual addon binary isn't in the patch?

How large are these addons? If they are somewhat size-able, maybe we can consider extending existing addons rather than adding even more. Up to you, I won't stop this from landing because of that.

::: browser/devtools/debugger/test/browser_dbg_addonactor.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Please add a summary of what this test is doing and testing to the top of this file.

::: browser/devtools/debugger/test/head.js
@@ +183,5 @@
>  }
>  
> +function get_addon_actor_for_url(aClient, aURL, aCallback) {
> +  aClient.listAddons(function(aResponse) {
> +    for each (let addon in aResponse.addons) {

Nit: can we use for/of instead of the deprecated/non-standard for each?

@@ +195,5 @@
> +
> +function attach_addon_actor_for_url(aClient, aURL, aCallback) {
> +  get_addon_actor_for_url(aClient, aURL, function(actor) {
> +    aClient.attachAddon(actor.actor, function(aResponse) {
> +      aCallback(actor, aResponse);

Nittiest of nits: can shorten to

  aClient.attachAddon(actor.actor, aCallback.bind(null, actor));

if you like that better.

Also, actor.actor is kinda funky on my eyes; maybe addon.actor?

Feel free to ignore this stuff if you don't agree.

Comment 19

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=31ef01e1bb61

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc9ca12061ea

Comment 21

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d4135bb531

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a3d4135bb531

Comment 23

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: Implement minimal addon actors

I've attached the rebased patch #3 as per Eddy's request.

Comment 24

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

Comment on attachment 797751 [details] [diff] [review]
Implement minimal addon actors

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

LGTM. Whiteboard [land-in-fx-team] once you have a green try push.

Please use 8 spaces of context in your future patches: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: browser/devtools/debugger/test/head.js
@@ +182,4 @@
>    });
>  }
>  
> +function get_addon_actor_for_url(aClient, aURL, aCallback) {

Aside: we really need to consolidate our head.js functions to either be snake- or camel-case. Need to wait for victor to finish his rewrites.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +940,5 @@
> +    return { type: "tabAttached", threadActor: this._threadActor.actorID };
> +  },
> +
> +  onDetach: function BAA_onDetach() {
> +    if (!this._attached) {

Shouldn't this be `this.attached` like it is above and elsewhere?

Comment 25

[Eddy Bruel [:ejpbruel]]

Wait, Mike already finished all this stuff didnt he?

Comment 26

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

If it is resolved, please mark it so ;)

Comment 27

[Eddy Bruel [:ejpbruel]]

Mike, this can be closed, right?

Comment 28

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: 0001-Implement-addon-actors.patch

Took a while, but here is a complete implementation of the addon actor.

I'm not quite sure if the tests are enough, though - they only test if the actor is debugging addon globals, but they say nothing about excluding non-addon stuff. I could check if a known non-addon chrome script is missing, but it seems kinda brittle. What do you think?

Comment 29

[Mike Hordecki [:mhordecki] (UTC+2)]

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=0c87be43704a

Comment 30

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

Comment on attachment 810953 [details] [diff] [review]
0001-Implement-addon-actors.patch

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

r+ with comments addressed.

Also, use 8 lines of context, it makes reviewing a whole lot easier: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: browser/devtools/debugger/test/browser_dbg_addonactor.js
@@ +31,5 @@
> +
> +}
> +
> +function attachAddonThread() {
> +  dump('Entering attachAddonThread\n');

If you think this logging is valuable outside of your own personal debugging, use dumpn instead of dump. Otherwise, remove this dump call.

@@ +42,5 @@
> +      .then(([aGrip, aResponse]) => {
> +        let deferred = promise.defer();
> +
> +        gClient.attachThread(aResponse.threadActor,
> +                             (aResponse, aThreadClient) => {

Style nit: funky indentation, put this on the same line as above.

@@ +80,5 @@
> +  let deferred = promise.defer();
> +
> +  gThreadClient.getSources(aResponse => {
> +      ok(aResponse.sources.some(
> +           (source) => source.url.indexOf(ADDON_MODULE_URL) != -1),

We only expect the source to show up once, right? And why are you using .indexOf() instead of ===? Or instead of .contains()?

Can we make this test a little tighter by doing:

  const matches = aResponse.sources.filter(s => s.url === ADDON_MODULE_URL);
  is(matches.length, 1);

If you really need to use .indexOf(), can you add a comment explaining why?

::: toolkit/devtools/client/dbg-client.jsm
@@ +479,5 @@
> +    let packet = {
> +      to: aAddonActor,
> +      type: "attach"
> +    };
> +    this.request(packet, (aResponse) => {

Drop parens if you want.

::: toolkit/devtools/server/actors/script.js
@@ +3581,5 @@
>  });
>  
> +/**
> + * Creates an actor for handling addon debugging. AddonDebuggerActor is a
> + * thin wrapper over ThreadActor, slightly changing some of its behavior.

Changing which behaviors? Either don't add the second half of the comment or fill it out with more info. Right now, I'm confused because it was apparently important enough to mention, but doesn't actually give me any info and leaves me wondering.

@@ +3584,5 @@
> + * Creates an actor for handling addon debugging. AddonDebuggerActor is a
> + * thin wrapper over ThreadActor, slightly changing some of its behavior.
> + *
> + * @param aConnection object
> + *        The DebuggerServerConnection with which this ChromeDebuggerActor

ChromeDebuggerActor?

@@ +3614,5 @@
> +  actorPrefix: "addonDebugger",
> +
> +  /**
> +   * Override the eligibility check for scripts and sources to make sure every
> +   * script and source with a URL is stored when debugging chrome.

"When debugging chrome"? "When debugging addons" would be better, no?

@@ +3620,5 @@
> +  _allowSource: function(aSourceURL) !!aSourceURL,
> +
> +   /**
> +   * An object that will be used by ThreadActors to tailor their behavior
> +   * depending on the debugging context being required (chrome or content).

chrome, addon, or content?

@@ +3627,5 @@
> +   */
> +  globalManager: {
> +    findGlobals: function ADA_findGlobals() {
> +      for (let global of this.dbg.findAllGlobals()) {
> +        if (this._checkGlobal(global))

Style nit: Unlike spidermonkey C++, we always use curlies in devtools JS code.

@@ +3666,5 @@
> +      metadata = Cu.getSandboxMetadata(aGlobal.unsafeDereference());
> +    } catch (e) {
> +    }
> +
> +    metadata = metadata || {};

Since you are setting metadata to {} when it doesn't exist here, can the metadata declaration simply be "let metadata;"?

Comment 31

[Mike Hordecki [:mhordecki] (UTC+2)]

Attachment: 0001-Implement-addon-actors.patch

Comment 32

[Paul Rouget [:paul]]

Apparently, this patch causes some issues with the app manager:

/GeckoConsole( 9217): [JavaScript Error: "Error loading: resource://gre/modules/devtools/server/actors/webbrowser.js: Error: You cannot use the AddonManager in child processes! - @resource://gre/modules/AddonManager.jsm:20
E/GeckoConsole( 9217):
E/GeckoConsole( 9217): " {file: "resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js" line: 66}]
E/GeckoConsole( 9217): [JavaScript Error: "Error: You cannot use the AddonManager in child processes!" {file: "resource://gre/modules/AddonManager.jsm" line: 20}]

Any idea what's going on?

Comment 33

[Paul Rouget [:paul]]

This happens when I'm debugging a device (keon).

Comment 34

[Alexandre Poirot [:ochameau]]

I opened bug 933273 and will submit a patch for that breakage.

Comment 35

[Eddy Bruel [:ejpbruel]]

(In reply to Mike Hordecki [:mhordecki] (UTC+2) from comment #31)
> Created attachment 811436 [details] [diff] [review]
> 0001-Implement-addon-actors.patch

(In reply to Nick Fitzgerald [:fitzgen] from comment #30)
> Comment on attachment 810953 [details] [diff] [review]
> 0001-Implement-addon-actors.patch
> 
> Review of attachment 810953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed.
> 
> Also, use 8 lines of context, it makes reviewing a whole lot easier:
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F
> 
> ::: browser/devtools/debugger/test/browser_dbg_addonactor.js
> @@ +31,5 @@
> > +
> > +}
> > +
> > +function attachAddonThread() {
> > +  dump('Entering attachAddonThread\n');
> 
> If you think this logging is valuable outside of your own personal
> debugging, use dumpn instead of dump. Otherwise, remove this dump call.
> 
> @@ +42,5 @@
> > +      .then(([aGrip, aResponse]) => {
> > +        let deferred = promise.defer();
> > +
> > +        gClient.attachThread(aResponse.threadActor,
> > +                             (aResponse, aThreadClient) => {
> 
> Style nit: funky indentation, put this on the same line as above.
> 
> @@ +80,5 @@
> > +  let deferred = promise.defer();
> > +
> > +  gThreadClient.getSources(aResponse => {
> > +      ok(aResponse.sources.some(
> > +           (source) => source.url.indexOf(ADDON_MODULE_URL) != -1),
> 
> We only expect the source to show up once, right? And why are you using
> .indexOf() instead of ===? Or instead of .contains()?
> 
> Can we make this test a little tighter by doing:
> 
>   const matches = aResponse.sources.filter(s => s.url === ADDON_MODULE_URL);
>   is(matches.length, 1);
> 
> If you really need to use .indexOf(), can you add a comment explaining why?
> 
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +479,5 @@
> > +    let packet = {
> > +      to: aAddonActor,
> > +      type: "attach"
> > +    };
> > +    this.request(packet, (aResponse) => {
> 
> Drop parens if you want.
> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +3581,5 @@
> >  });
> >  
> > +/**
> > + * Creates an actor for handling addon debugging. AddonDebuggerActor is a
> > + * thin wrapper over ThreadActor, slightly changing some of its behavior.
> 
> Changing which behaviors? Either don't add the second half of the comment or
> fill it out with more info. Right now, I'm confused because it was
> apparently important enough to mention, but doesn't actually give me any
> info and leaves me wondering.
> 
> @@ +3584,5 @@
> > + * Creates an actor for handling addon debugging. AddonDebuggerActor is a
> > + * thin wrapper over ThreadActor, slightly changing some of its behavior.
> > + *
> > + * @param aConnection object
> > + *        The DebuggerServerConnection with which this ChromeDebuggerActor
> 
> ChromeDebuggerActor?
> 
> @@ +3614,5 @@
> > +  actorPrefix: "addonDebugger",
> > +
> > +  /**
> > +   * Override the eligibility check for scripts and sources to make sure every
> > +   * script and source with a URL is stored when debugging chrome.
> 
> "When debugging chrome"? "When debugging addons" would be better, no?
> 
> @@ +3620,5 @@
> > +  _allowSource: function(aSourceURL) !!aSourceURL,
> > +
> > +   /**
> > +   * An object that will be used by ThreadActors to tailor their behavior
> > +   * depending on the debugging context being required (chrome or content).
> 
> chrome, addon, or content?
> 
> @@ +3627,5 @@
> > +   */
> > +  globalManager: {
> > +    findGlobals: function ADA_findGlobals() {
> > +      for (let global of this.dbg.findAllGlobals()) {
> > +        if (this._checkGlobal(global))
> 
> Style nit: Unlike spidermonkey C++, we always use curlies in devtools JS
> code.
> 
> @@ +3666,5 @@
> > +      metadata = Cu.getSandboxMetadata(aGlobal.unsafeDereference());
> > +    } catch (e) {
> > +    }
> > +
> > +    metadata = metadata || {};
> 
> Since you are setting metadata to {} when it doesn't exist here, can the
> metadata declaration simply be "let metadata;"?

This patch never landed. Review comments haven't been addressed, and as far as I can tell, addon3.xpi is missing from the patch. It will take me some time to piece things back together.

Comment 36

[Dave Townsend [:mossop]]

(In reply to Eddy Bruel [:ejpbruel] from comment #35)
> This patch never landed. Review comments haven't been addressed, and as far
> as I can tell, addon3.xpi is missing from the patch. It will take me some
> time to piece things back together.

addon3.xpi is in the patch file, bugzilla's diff view doesn't show binary files though.

Comment 37

[Eddy Bruel [:ejpbruel]]

(In reply to Dave Townsend (:Mossop) from comment #36)
> (In reply to Eddy Bruel [:ejpbruel] from comment #35)
> > This patch never landed. Review comments haven't been addressed, and as far
> > as I can tell, addon3.xpi is missing from the patch. It will take me some
> > time to piece things back together.
> 
> addon3.xpi is in the patch file, bugzilla's diff view doesn't show binary
> files though.

*sigh*

You know, I've learned not to expect too much from Bugzilla, but was it too much to ask to even just *list* the file in the diff view? I did not know about this particular 'feature', and it sounds like I wasted a full day trying to reconstitute a file that was there all along as a result. Major UX fail as far as I'm concerned.

On the plus side, that means the patch is as good as intact, and should be able to land with review comments addressed. The only thing that worries me a bit is how we handle exiting of the addon thread actor on the server side. Right now, we exit the thread actor when then onUninstalled event fires. This might not be the right thing to do, since add-ons can still run code even after they are disabled or uninstalled. It might be better to just remove that logic all together.

Comment 38

[Jeff Griffiths (:canuckistani) (:⚡︎)]

*bump* status?

Comment 39

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 0001-Bug-899052-Add-Addon-Thread-Actors.patch

Hey Victor, here's a patch regarding implement addon actors made awhile ago and bitrotted a bit. Running now, the tests pass and end with INFO TEST-END, but then followed by a "checking window state" and hangs there. Any idea what's causing this?
https://github.com/jsantell/gecko-dev/compare/899052-addon-thread-actors

Comment 40

[Victor Porof [:vporof][:vp]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #39)
> Created attachment 8368181 [details] [diff] [review]

I don't know why this is happening. It looks like the test succeeds and runs properly, but the suite is having a hard time cleaning up. I'll just push to try and see if we get anything from that.

Comment 41

[Victor Porof [:vporof][:vp]]

Try run with logging enabled: https://tbpl.mozilla.org/?tree=Try&rev=57feb49078f2

Comment 42

[Jordan Santell [:jsantell] [@jsantell]]

Regarding the crash in this test, with the help of Nick, we were able to get a stack trace of the crash... any idea here of what's going on?

Assertion failure: !k->compartment()->options_.invisibleToDebugger(), at /Users/jsantell/Dev/gecko-dev/js/src/vm/Debugger.h:86

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000

81	
82	    template<typename KeyInput, typename ValueInput>
83	    bool relookupOrAdd(AddPtr &p, const KeyInput &k, const ValueInput &v) {
84	        JS_ASSERT(v->compartment() == Base::compartment);
85	        JS_ASSERT(!k->compartment()->options_.mergeable());
86	        JS_ASSERT_IF(!InvisibleKeysOk, !k->compartment()->options_.invisibleToDebugger());
87	        JS_ASSERT(!Base::has(k));
88	        if (!incZoneCount(k->zone()))
89	            return false;
90	        bool ok = Base::relookupOrAdd(p, k, v);
(gdb) bt
#0  js::DebuggerWeakMap<js::EncapsulatedPtr<JSObject, unsigned long>, js::RelocatablePtr<JSObject>, false>::relookupOrAdd<JS::Rooted<JSObject*>, JSObject*> (this=0x12d92fa98, p=@0x7fff5fbedc20, k=@0x7fff5fbedc80, v=@0x7fff5fbedc10) at Debugger.h:86
#1  0x0000000106245031 in js::DependentAddPtr<js::DebuggerWeakMap<js::EncapsulatedPtr<JSObject, unsigned long>, js::RelocatablePtr<JSObject>, false> >::add<JS::Rooted<JSObject*>, JSObject*> (this=0x7fff5fbedc20, cx=0x10068cff0, table=@0x12d92fa98, key=@0x7fff5fbedc80, value=@0x7fff5fbedc10) at jshashutil.h:40
#2  0x00000001062056d0 in js::Debugger::wrapDebuggeeValue (this=0x12d92f800, cx=0x10068cff0, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbedd68}) at Debugger.cpp:732
#3  0x000000010620e71a in js::Debugger::findAllGlobals (cx=0x10068cff0, argc=0, vp=0x110329398) at Debugger.cpp:2761
#4  0x00000001062fa265 in js::CallJSNative (cx=0x10068cff0, native=0x10620e500 <js::Debugger::findAllGlobals(JSContext*, unsigned int, JS::Value*)>, args=@0x7fff5fbee340) at jscntxtinlines.h:230
#5  0x00000001062b886c in js::Invoke (cx=0x10068cff0, args={<JS::detail::CallArgsBase<0>> = {<JS::CallReceiver> = {<JS::detail::CallReceiverBase<0>> = {<JS::detail::UsedRvalBase<IncludeUsedRval>> = {usedRval_ = false}, argv_ = 0x1103293a8}, <No data fields>}, argc_ = 0}, <No data fields>}, construct=js::NO_CONSTRUCT) at Interpreter.cpp:476
#6  0x00000001062aecf0 in Interpret (cx=0x10068cff0, state=@0x7fff5fbf11c0) at Interpreter.cpp:2614
#7  0x00000001062a1b89 in js::RunScript (cx=0x10068cff0, state=@0x7fff5fbf11c0) at Interpreter.cpp:423
#8  0x00000001062b8989 in js::Invoke (cx=0x10068cff0, args={<JS::detail::CallArgsBase<0>> = {<JS::CallReceiver> = {<JS::detail::CallReceiverBase<0>> = {<JS::detail::UsedRvalBase<IncludeUsedRval>> = {usedRval_ = false}, argv_ = 0x7fff5fbf13a0}, <No data fields>}, argc_ = 0}, <No data fields>}, construct=js::NO_CONSTRUCT) at Interpreter.cpp:495
#9  0x0000000106039f99 in js::CallOrConstructBoundFunction (cx=0x10068cff0, argc=0, vp=0x1103292e0) at jsfun.cpp:1380
#10 0x00000001062fa265 in js::CallJSNative (cx=0x10068cff0, native=0x106039ac0 <js::CallOrConstructBoundFunction(JSContext*, unsigned int, JS::Value*)>, args=@0x7fff5fbf19e0) at jscntxtinlines.h:230
#11 0x00000001062b886c in js::Invoke (cx=0x10068cff0, args={<JS::detail::CallArgsBase<0>> = {<JS::CallReceiver> = {<JS::detail::CallReceiverBase<0>> = {<JS::detail::UsedRvalBase<IncludeUsedRval>> = {usedRval_ = false}, argv_ = 0x1103292f0}, <No data fields>}, argc_ = 0}, <No data fields>}, construct=js::NO_CONSTRUCT) at Interpreter.cpp:476
#12 0x00000001062aecf0 in Interpret (cx=0x10068cff0, state=@0x7fff5fbf4860) at Interpreter.cpp:2614
#13 0x00000001062a1b89 in js::RunScript (cx=0x10068cff0, state=@0x7fff5fbf4860) at Interpreter.cpp:423
#14 0x00000001062b8989 in js::Invoke (cx=0x10068cff0, args={<JS::detail::CallArgsBase<0>> = {<JS::CallReceiver> = {<JS::detail::CallReceiverBase<0>> = {<JS::detail::UsedRvalBase<IncludeUsedRval>> = {usedRval_ = false}, argv_ = 0x7fff5fbf4a40}, <No data fields>}, argc_ = 2}, <No data fields>}, construct=js::NO_CONSTRUCT) at Interpreter.cpp:495
#15 0x0000000106039f99 in js::CallOrConstructBoundFunction (cx=0x10068cff0, argc=2, vp=0x110329220) at jsfun.cpp:1380
#16 0x00000001062fa265 in js::CallJSNative (cx=0x10068cff0, native=0x106039ac0 <js::CallOrConstructBoundFunction(JSContext*, unsigned int, JS::Value*)>, args=@0x7fff5fbf5080) at jscntxtinlines.h:230
#17 0x00000001062b886c in js::Invoke (cx=0x10068cff0, args={<JS::detail::CallArgsBase<0>> = {<JS::CallReceiver> = {<JS::detail::CallReceiverBase<0>> = {<JS::detail::UsedRvalBase<IncludeUsedRval>> = {usedRval_ = false}, argv_ = 0x110329230}, <No data fields>}, argc_ = 2}, <No data fields>}, construct=js::NO_CONSTRUCT) at Interpreter.cpp:476
#18 0x00000001062aecf0 in Interpret (cx=0x10068cff0, state=@0x7fff5fbf7f00) at Interpreter.cpp:2614
#19 0x00000001062a1b89 in js::RunScript (cx=0x10068cff0, state=@0x7fff5fbf7f00) at Interpreter.cpp:423
#20 0x00000001062b8989 in js::Invoke (cx=0x10068cff0, args={<JS::detail::CallArgsBase<0>> = {<JS::CallReceiver> = {<JS::detail::CallReceiverBase<0>> = {<JS::detail::UsedRvalBase<IncludeUsedRval>> = {usedRval_ = false}, argv_ = 0x7fff5fbf84d0}, <No data fields>}, argc_ = 0}, <No data fields>}, construct=js::NO_CONSTRUCT) at Interpreter.cpp:495
#21 0x0000000106038a10 in js_fun_apply (cx=0x10068cff0, argc=2, vp=0x1103290a8) at jsfun.cpp:1108
#22 0x00000001062fa265 in js::CallJSNative (cx=0x10068cff0, native=0x106037f80 <js_fun_apply(JSContext*, unsigned int, JS::Value*)>, args=@0x7fff5fbf8aa0) at jscntxtinlines.h:230
#23 0x00000001062b886c in js::Invoke (cx=0x10068cff0, args={<JS::detail::CallArgsBase<0>> = {<JS::CallReceiver> = {<JS::detail::CallReceiverBase<0>> = {<JS::detail::UsedRvalBase<IncludeUsedRval>> = {usedRval_ = false}, argv_ = 0x1103290b8}, <No data fields>}, argc_ = 2}, <No data fields>}, construct=js::NO_CONSTRUCT) at Interpreter.cpp:476
#24 0x00000001062aecf0 in Interpret (cx=0x10068cff0, state=@0x7fff5fbfb920) at Interpreter.cpp:2614
#25 0x00000001062a1b89 in js::RunScript (cx=0x10068cff0, state=@0x7fff5fbfb920) at Interpreter.cpp:423
#26 0x00000001062b8989 in js::Invoke (cx=0x10068cff0, args={<JS::detail::CallArgsBase<0>> = {<JS::CallReceiver> = {<JS::detail::CallReceiverBase<0>> = {<JS::detail::UsedRvalBase<IncludeUsedRval>> = {usedRval_ = false}, argv_ = 0x7fff5fbfbb10}, <No data fields>}, argc_ = 0}, <No data fields>}, construct=js::NO_CONSTRUCT) at Interpreter.cpp:495
#27 0x00000001062b917d in js::Invoke (cx=0x10068cff0, thisv=@0x7fff5fbfbbe0, fval=@0x7fff5fbfc578, argc=0, argv=0x7fff5fbfc4b0, rval={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfc1e8}) at Interpreter.cpp:532
#28 0x0000000106015b4f in JS_CallFunctionValue (cx=0x10068cff0, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfc528}, fval={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5fbfc578}, args=@0x7fff5fbfc170, rval={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfc1e8}) at jsapi.cpp:5024
#29 0x000000010392f15d in nsXPCWrappedJSClass::CallMethod (this=0x118df21a0, wrapper=0x12dcc7b00, methodIndex=3, info_=0x10fb7a900, nativeParams=0x7fff5fbfc8a0) at XPCWrappedJSClass.cpp:1273
#30 0x0000000103928d59 in nsXPCWrappedJS::CallMethod (this=0x12dcc7b00, methodIndex=3, info=0x10fb7a900, params=0x7fff5fbfc8a0) at XPCWrappedJS.cpp:517
#31 0x00000001016be359 in PrepareAndDispatch (self=0x12d96adc0, methodIndex=3, args=0x7fff5fbfca00, gpregs=0x7fff5fbfc980, fpregs=0x7fff5fbfc9b0) at xptcstubs_x86_64_darwin.cpp:122
#32 0x00000001016bcdbb in SharedStub () at xptcstubs_x86_64_darwin.cpp:35
#33 0x00000001016a3009 in nsThread::ProcessNextEvent (this=0x100614e20, mayWait=false, result=0x7fff5fbfcbc3) at nsThread.cpp:643
#34 0x00000001015a45cb in NS_ProcessPendingEvents (thread=0x100614e20, timeout=20) at nsThreadUtils.cpp:210
#35 0x00000001037f1769 in nsBaseAppShell::NativeEventCallback (this=0x110323a60) at nsBaseAppShell.cpp:98
#36 0x000000010377d3fc in nsAppShell::ProcessGeckoEvents (aInfo=0x110323a60) at nsAppShell.mm:388
#37 0x00007fff8ed4cb31 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ ()
#38 0x00007fff8ed4c455 in __CFRunLoopDoSources0 ()
#39 0x00007fff8ed6f7f5 in __CFRunLoopRun ()
#40 0x00007fff8ed6f0e2 in CFRunLoopRunSpecific ()
#41 0x00007fff83f31eb4 in RunCurrentEventLoopInMode ()
#42 0x00007fff83f31b94 in ReceiveNextEventCommon ()
#43 0x00007fff83f31ae3 in BlockUntilNextEventMatchingListInMode ()
#44 0x00007fff894c5533 in _DPSNextEvent ()
#45 0x00007fff894c4df2 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#46 0x000000010377c357 in -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (self=0x10060af70, _cmd=0x7fff89cf3404, mask=18446744073709551615, expiration=0x422d63c37f00000d, mode=0x7fff7553d1c0, flag=1 '\001') at nsAppShell.mm:165
#47 0x00007fff894bc1a3 in -[NSApplication run] ()
#48 0x000000010377dee2 in nsAppShell::Run (this=0x110323a60) at nsAppShell.mm:742
#49 0x000000010543d14c in nsAppStartup::Run (this=0x1103b5150) at nsAppStartup.cpp:276
#50 0x00000001052f5ccc in XREMain::XRE_mainRun (this=0x7fff5fbfeae0) at /Users/jsantell/Dev/gecko-dev/toolkit/xre/nsAppRunner.cpp:4006
#51 0x00000001052f64d9 in XREMain::XRE_main (this=0x7fff5fbfeae0, argc=5, argv=0x7fff5fbff3e8, aAppData=0x7fff5fbfed78) at /Users/jsantell/Dev/gecko-dev/toolkit/xre/nsAppRunner.cpp:4073
#52 0x00000001052f694d in XRE_main (argc=5, argv=0x7fff5fbff3e8, aAppData=0x7fff5fbfed78, aFlags=0) at /Users/jsantell/Dev/gecko-dev/toolkit/xre/nsAppRunner.cpp:4283
#53 0x00000001000020e7 in do_main (argc=5, argv=0x7fff5fbff3e8, xreDirectory=0x10062fd40) at /Users/jsantell/Dev/gecko-dev/browser/app/nsBrowserApp.cpp:282
#54 0x0000000100001621 in main (argc=5, argv=0x7fff5fbff3e8) at /Users/jsantell/Dev/gecko-dev/browser/app/nsBrowserApp.cpp:643

Comment 43

[Eddy Bruel [:ejpbruel]]

Assertions in C++ trigger a NULL pointer exception when they fail. The assertion on line 86 is failing here, which means that the compartment you're trying to lookup or add has the invisibleToDebugger flag set to true, while InvisibleKeysOk is set to false. What's likely going on is that you're trying to add a global as debuggee that is supposed to be invisible to the debugger.

Do any globals that we use in the add-on SDK have the invisibleToDebugger flag set to true? Is it possible that we somehow try to debug some of these globals when we attach to an add-on thread actor? Afaik, invisibleToDebugger didn't exist when I originally wrote this patch, so I did not take this possibility into account.

Comment 44

[Shu-yu Guo [:shu]]

I didn't read the rest of the bug and I don't really know what this patch does, but invisibleToDebugger is true for compartments with values that aren't meant to escape to script.

For the JS engine, this is meant for compartments used in off-main-thread workloads in the JS engine which get merged back into parent compartments after finish.

For Gecko things, XPD and XBL globals have this flag set. Are you touching one of those?

Comment 45

[Shu-yu Guo [:shu]]

Actually now I see that findAllGlobals is in the stack. It's iterating all compartments, regardless of visibility to debugger. The iteration should skip invisibleToDebugger compartments like the XPD and XBL globals.

I'll defer to Jim, but try adding

  if (c->options().invisibleToDebugger())
      continue;

to the beginning of the body of this loop: http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp?from=findAllGlobals#2748

Comment 46

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #45)
> I'll defer to Jim, but try adding
> 
>   if (c->options().invisibleToDebugger())
>       continue;

Yes, by all means. This seems like a clearly correct change. Debugger::addAllGlobalsAsDebuggees does the same.

Comment 47

[Jordan Santell [:jsantell] [@jsantell]]

try https://tbpl.mozilla.org/?tree=Try&rev=81f5e1ff2740

Comment 48

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 899052-addon-thread-actors.patch

Somehow everything is passing -- Nick, adding you as r? since you reviewed the previous patches -- if this should go to someone else, let me know!

Comment 49

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

Comment on attachment 8387720 [details] [diff] [review]
899052-addon-thread-actors.patch

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

Nice job un-bitrotting this patch, diving into gdb and spidermonkey for what was a purely JS patch, and getting everything working!

This is an r+ for everything except the Debugger.cpp changes. I'm deferring to Shu for that since I'm not a reviewer of that code.

::: browser/devtools/debugger/test/head.js
@@ +664,5 @@
> +function attachAddonActorForUrl(aClient, aUrl) {
> +  let deferred = promise.defer();
> +
> +  getAddonActorForUrl(aClient, aUrl).then(aGrip => {
> +    info("ATTACHING ADODN ACTOR FOR URL");

All of these info calls: either remove them if you don't think we need the debug info in the future, or make them have normal capitalization and log the url they are for too.

::: toolkit/devtools/client/dbg-client.jsm
@@ +470,5 @@
>    },
>  
>    /**
> +   * Attach to an addon actor.
> +   * 

Nit: trailing space.

@@ +487,5 @@
> +      let addonClient;
> +      if (!aResponse.error) {
> +        addonClient = new AddonClient(this, aAddonActor);
> +        this._addonClients[aAddonActor] = addonClient;
> +        this.activeAddon = addonClient;

Will you add

  /**
   * blah blah blah
   */
  activeAddon: null,

so that

1. People reading the code know what it is.

2. The property is always present in the prototype in the same order. This helps us with js::Shape sharing.

@@ +1097,5 @@
> +    type: "detach"
> +  }, {
> +    after: function(aResponse) {
> +      if (this.activeAddon === this._client._addonClients[this.actor]) {
> +        delete this.activeAddon;

Won't somebody think of the shapes?! Set null instead of deleting.

Also, |this| is an |AddonClient| here, while the |activeAddon| property is on |DebuggerClient|. You want:

  this._client.activeAddon = null;

::: toolkit/devtools/server/actors/script.js
@@ +4575,5 @@
> + *        ID of the add-on this actor will debug. It will be used to
> + *        filter out globals marked for debugging.
> + */
> +
> +function AddonDebuggerActor(aConnect, aHooks, aAddonID) {

This should be named "AddonThreadActor".

@@ +4586,5 @@
> +update(AddonDebuggerActor.prototype, {
> +  constructor: AddonDebuggerActor,
> +
> +  // A constant prefix that will be used to form the actor ID by the server.
> +  actorPrefix: "addonDebugger",

"addonThread"

@@ +4645,5 @@
> +    } catch (e) {
> +    }
> +
> +    metadata = metadata || {};
> +    return metadata.addonID === this.addonID;

Could make it a shiny one liner and avoid the allocation with:

  return metadata && metadata.addonID === this.addonID;

::: toolkit/devtools/server/actors/webbrowser.js
@@ +1084,5 @@
> +      windowUtils.suspendTimeouts();
> +    }
> +  },
> +
> +  postNest: function() {

This should be |postNost| shouldn't it?











>:)

@@ +1099,5 @@
> +  addToParentPool: function(aActor) {
> +    this.conn.addActor(aActor);
> +  },
> +
> +  removeFromParentPool: function(aActor) {

addToParentPool and removeFromParentPool are no longer needed. Delete em!

Comment 50

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 899052-addon-thread-actors.patch

Correct fitzgen's nit's, readding shu as r? for the C++ code

Comment 51

[Shu-yu Guo [:shu]]

Comment on attachment 8387792 [details] [diff] [review]
899052-addon-thread-actors.patch

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

The Debugger.cpp parts LGTM.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

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

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6a1cc77c943