740551 - ThreadActor should automatically add appropriate debuggee globals

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Jim Blandy :jimb]

For chrome debugging, rather than selecting the debuggee by choosing a tab actor, it makes more sense to simply have a single thread actor for Firefox's main thread. Then, when the thread is paused, the protocol should let the client enumerate all globals (other than the debugger itself), and select/deselect them as debuggees.

Draft documentation here: https://github.com/jimblandy/DebuggerDocs/commit/c533d261c5e3d02b1ac57c6a65f822385054b827

Comment 1

[Jim Blandy :jimb]

After

Comment 2

[Jim Blandy :jimb]

(ahem) After talking with Dave Camp in Toronto, I don't think we actually need the protocol to provide this facility at all; debuggee management can be done entirely server-side.

Having a selected set of debuggee globals is important for content debugging, because such debugging clearly shouldn't extent to chrome or unrelated tabs by default. It's also important for chrome debugging, to avoid infelicities arising from the debugger trying to debug itself.

However, I'd drifted into the assumption that it was an attractive feature for chrome debugger users to select which parts of the system they wanted to enable debugging in. Dave argued (and I now agree) that it isn't, really. You want to be able to set breakpoints in and step into any function you like, without pre-arrangement. You want the backtrace to show all the frames.

It seems like it's pretty well-understood, then, what the set of debuggee globals for a given kind of debugger should be: a content debugger should watch for iframes and navigation; a chrome debugger should watch everything but itself (but who debugs the ... never mind); a b2g app debugger should debug the top iframe for the app; and so on.

In each case, the set of interesting debuggees is determined by the nature of the debugger (content; chrome; b2g). The process of identifying and adding debuggees doesn't need to involve the protocol.

Instead, I'd like ThreadActor to take a 'globalManager' parameter: an object with the interface:

- findGlobals(): a method to produce a list of all applicable globals, in categories, with
  labels.
  - A content ThreadActor's globalManager will return the list of windows, iframes, and so
    on associated with the tab.
  - A chrome ThreadActor's globalManager will return the list of every single global on
    the system they know about (other than the debugger server's own global).

- onNewGlobal: a hook the ThreadActor can set to provide a function for the global manager
  to call when new globals of possible interest are loaded.
  - A content ThreadActor's globalManager will watch for new iframes. (I don't think
    there's any other way to introduce a new global into content.)
  - a chrome ThreadActor needs to hook into SpiderMonkey somehow to get notification of
    new globals.

Eventually, perhaps we could fold the tab navigation into the globalManager as well,
instead of making the client explicitly attach to each new page.

Comment 3

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

Attachment: Patch v1

This works, but I haven't had a chance to incorporate findAllGlobals yet.

Comment 4

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

Attachment: Patch v2

New patch, this time with findAllGlobals, but for some reason I don't seem to get all the scripts I would expect.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=652075e626d8

Comment 5

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

Previous try run was missing some dependencies. New try: https://tbpl.mozilla.org/?tree=Try&rev=fdc564ad9f11

Comment 6

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

Attachment: Patch v3

Fixed broken xpcshell tests. Try: https://tbpl.mozilla.org/?tree=Try&rev=4496a4b5b817

Comment 7

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

Attachment: Patch v4

Rebased and added a few more fixes.

Comment 8

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

Attachment: Patch v5

More fixes. I still need to do a couple of cleanups, but this is a good time to ask for feedback. The two main issues at this point is that (a) some times breakpoints in chrome code are not triggered (which I'm investigating) and (b) it takes 2.5 minutes in my laptop to add all the globals as debuggees and 4.5 minutes to remove them, during which the browser is beachballing. I haven't profiled it, but I remember something about SpiderMonkey doing something expensive every time we call addDebuggee? Unfortunately my memory is fuzzy.

Comment 9

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

I got a profiling trace while the browser debugger is attaching to the debuggee:

http://people.mozilla.com/~bgirard/cleopatra/?report=bca38181bdd284291b22789c951e612504d77007

No add-ons besides the profiler, one window with a single tab displaying about:home. Started Browser Debugger after Firefox started.
fx-team tip plus this patch and its dependency.

Comment 10

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

Surprisingly, 1/3 of the time was spent in GC and the profiler and 2/3 were spent in onNewGlobal -> addDebuggee -> CheckCompartmentCallback. I expected that the findGlobals loop would be the culprit, but it looks like we get new globals at a rapid rate afterwards.

Comment 11

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

Attachment: Patch v6

ChromeDebuggerActor wasn't properly overriding the ThreadActor's properties, but I have fixed it now. Also, when attaching to the tab we used to initialize the Debugger object, even though the client hadn't attached to the thread actor yet. Fixed that too, so that the web console and others can attach to tab actors without taking the performance hit.

Comment 12

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

Attachment: Patch v7

Got the initial delay when attaching to the chrome debugger down to a few seconds from 1.5 minutes. Also got the delay when detaching down from 4.5 minutes to 1.5', but I haven't really looked into that yet. Also added a newGlobal notification that will be used when we get hostAnnotations to present the globals to the user and made a number of cleanups that I wanted to do for a long time. I just need to write some tests before asking for a formal review.

Comment 13

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

Attachment: Patch v8

I just remembered that it would be appropriate to flip the chrome-enabled killswitch in this patch, since chrome debugging should be functional after this. All that should be required in order to see the 'Browser Debugger' menu item now is the devtools.chrome.enabled pref set to true (same as for chrome-enabled scratchpad).

I also profiled the chrome debugger detachment and it looks like we're spending time in GC land (CheckCompartmentCallback):

http://people.mozilla.com/~bgirard/cleopatra/?report=95860fdbbf88407f63666e86b6262538d4a525de

Comment 14

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

Attachment: Implement Debugger.removeAllDebuggees

Based on the profiling I did I think that a new Debugger.removeAllDebuggees() method would make chrome debugger detaching quite faster. This is my attempt at implementing this and I must have missed something obvious, because I get an assertion:

Assertion failure: addr % Cell::CellSize == 0, at js/src/gc/Heap.h:846

Halp!

Comment 15

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

Attachment: Implement Debugger.removeAllDebuggees (take 2)

jorendorff explained that if I want to modify the hash table while enumerating I need to pass the enumerator to removeDebuggeeGlobal. Indeed this gets rid of the assertion, but unfortunately it doesn't make detachment faster. I'm going to enable stackwalking to see if I can get any more information from the profiler.

This new method however seems useful, since that is precisely what a debugger would want to do in the teardown path. Even if SpiderMonkey can optimize the JS loop to the speed of the C++ one, I'd argue that conceptually this is a small win for the API.

Another idea suggested by Jason is to not remove the debuggees when detaching, but just disable the debugger. This would be lightning fast, but AFAICT the debuggee would remain without the JIT optimizations that the Debugger precludes for the lifetime of the process. Probably leaking some memory, too. Is that better UX? Do we expect users of chrome debugging to not use the debuggee instance for regular browsing afterwards?

Comment 16

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

Attachment: Patch v9

Added a test that exercises the new protocol paths, without touching the parts that are shared with the thread actor implementation and are tested by lots of other tests. Also fixed a bug where starting the content debugger before chrome debugging would not display the connection permission dialog for the latter.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=3249a5dc6038

Comment 17

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

Attachment: Implement Debugger.removeAllDebuggees (take 3)

The test relied on a specific order for the results of getDebuggees, which was not guaranteed. Fixed that.

Comment 18

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

Attachment: Patch v10

Made the test more robust by ensuring that a newGlobal event will be fired while the client is attached to the chrome debugger. New try:

https://tbpl.mozilla.org/?tree=Try&rev=366a50fb77a5

Comment 19

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

Attachment: Patch v11

Non-optimized test runs were still failing, but this version fixes them. Proof (crosses fingers):
https://tbpl.mozilla.org/?tree=Try&rev=58fa3126a6a9

Comment 20

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

Attachment: Disable debugging instead of removing debuggees

This is the alternative that jorendorff suggested. Detaching the chrome debugger is instantaneous, but we don't give back the memory and the globals are jitted somewhat (how much?) slower. Also, the next test that runs after the chrome-debugging test fails when it hits a |debugger| keyword with: Assertion failure: !InNoGCScope(), at js/src/gc/Root.h:767

To try it out, apply it on top of the onGlobals and removeAllDebuggees patches.

Comment 21

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

Attachment: Patch v12

Rebased.

Comment 22

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

>>  // Enable the Debugger
>>  pref("devtools.debugger.enabled", true);
>> -pref("devtools.debugger.chrome-enabled", false);
>> +pref("devtools.debugger.chrome-enabled", true);

This pref is redundant now. We should probably remove it completely and rely only on devtools.chrome.enabled to display the menuitem entry. But this is very low priority.

Comment 23

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

(In reply to Victor Porof [:vp] from comment #22)
> >>  // Enable the Debugger
> >>  pref("devtools.debugger.enabled", true);
> >> -pref("devtools.debugger.chrome-enabled", false);
> >> +pref("devtools.debugger.chrome-enabled", true);
> 
> This pref is redundant now. We should probably remove it completely and rely
> only on devtools.chrome.enabled to display the menuitem entry. But this is
> very low priority.

That's true, it is redundant, I left it only as a safety net in case we discover we need to killswitch the feature in aurora or beta.

Comment 24

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

Attachment: Patch v13

Hidden the new globals list that will remain unused until we get Debugger.Object.prototype.hostAnnotations.

Comment 25

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 676510 [details] [diff] [review]
Patch v13

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

this is nice stuff.

I'm really mostly concerned with whether or not we should enable this for everyone by default (I don't think we do, but could use a product and/or UX call for backup) and a couple of comment nits. I think with this preffed off we can land it, so R+ with that.

::: browser/app/profile/firefox.js
@@ +1026,5 @@
>  pref("devtools.responsiveUI.enabled", true);
>  
>  // Enable the Debugger
>  pref("devtools.debugger.enabled", true);
> +pref("devtools.debugger.chrome-enabled", true);

Do we want to do this by default? It's not something most web developers will want, though it is incredibly beautiful and everyone should have a chance to see it.

::: browser/devtools/debugger/debugger-controller.js
@@ +813,5 @@
> +  _onNewGlobal: function SS__onNewGlobal(aNotification, aPacket) {
> +    // TODO: after we get the global list in bug 707302, we should start
> +    // updating it here using aPacket.hostAnnotations from bug 801084.
> +  },
> +

yes!

::: browser/devtools/debugger/test/browser_dbg_chrome-debugging.js
@@ +47,5 @@
> +}
> +
> +function onNewScript(aEvent, aScript)
> +{
> +  if (aScript.url.startsWith("chrome:")) {

do we care about about: or resource: uris here? Are they likely to make this fail?

::: toolkit/devtools/debugger/server/dbg-browser-actors.js
@@ +240,5 @@
> +   * Prepare to enter a nested event loop by disabling debuggee events.
> +   */
> +  preNest: function BRA_preNest() {
> +    let top = windowMediator.getMostRecentWindow("navigator:browser");
> +    let browser = top.gBrowser.selectedBrowser;

Would be kind of nice if this file could live up in /browser/devtools/debugger somewhere.

Do any of our other consumers use dbg-browser-actors.js?

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +81,5 @@
>    addDebuggee: function TA_addDebuggee(aGlobal) {
> +    try {
> +      this.dbg.addDebuggee(aGlobal);
> +    } catch (e) {
> +      // Ignore attempts to add the debugger's compartment as a debuggee.

not even worth a dumpn, eh?

@@ +88,3 @@
>  
> +  /**
> +   * Initialize the Debugger object using the inspector XPCOM component to turn

which XPCOM component?

@@ +202,4 @@
>      this.dbg.enabled = true;
>      try {
>        // Put ourselves in the paused state.
>        // XXX: We need to put the debuggee in a paused state too.

Is there a bug for this XXX comment or can it go now?

@@ +2183,5 @@
> +        hostAnnotations: aGlobal.hostAnnotations
> +      });
> +    }
> +  }
> +});

nice.

@@ +2205,5 @@
> +    if (desc) {
> +      Object.defineProperty(aTarget, key, desc);
> +    }
> +  }
> +}

check out victor's create() method here:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/VariablesView.jsm#1182

I'm thinking that would be useful sugaring for these cases. We should avoid creating too many of them, though don't want to refactor things for this.

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +322,5 @@
>  
>    /**
>     * Remove the connection from the debugging server.
>     */
> +  _connectionClosed: function DS_connectionClosed(aConnection) {

how did those get the DH_ prefix in the first place?

@@ +341,5 @@
> +   *        actorPrefix property of the constructor prototype is used.
> +   */
> +  addTabActor: function DS_addTabActor(aFunction, aName) {
> +    let name = aName ? aName : aFunction.prototype.actorPrefix;
> +    if (["title", "url", "actor"].indexOf(name) != -1) {

maybe add a comment for this check indicating that we prevent certain types of extensions. If this is going to be public API.

@@ +358,5 @@
> +   * @param aFunction function
> +   *        The constructor function for this request type.
> +   */
> +  removeTabActor: function DS_removeTabActor(aFunction) {
> +    for (let name in DebuggerServer.tabActorFactories) {

do we need to prevent the removal of certain types too?

Comment 26

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #25)
> ::: browser/app/profile/firefox.js
> @@ +1026,5 @@
> >  pref("devtools.responsiveUI.enabled", true);
> >  
> >  // Enable the Debugger
> >  pref("devtools.debugger.enabled", true);
> > +pref("devtools.debugger.chrome-enabled", true);
> 
> Do we want to do this by default? It's not something most web developers
> will want, though it is incredibly beautiful and everyone should have a
> chance to see it.

In order to get the Browser Debugger menu item one has to flip the preferences devtools.debugger.remote-enabled and devtools.chrome.enabled from their default values. Does that sound enough, or do you think we need the extra devtools.debugger.chrome-enabled pref as well?

> ::: browser/devtools/debugger/test/browser_dbg_chrome-debugging.js
> @@ +47,5 @@
> > +}
> > +
> > +function onNewScript(aEvent, aScript)
> > +{
> > +  if (aScript.url.startsWith("chrome:")) {
> 
> do we care about about: or resource: uris here? Are they likely to make this
> fail?

I'm using a chrome: URL here as proof that scripts that are hidden when debugging content are present when debugging chrome. While the test is running this handler is called many times with all kinds of scripts, so other protocols won't be a problem here.

You could argue for using about: instead of chrome: here, since they are both hidden when debugging content, but in my testing about: URLs were rather rare (about:blank basically) when the test was running, whereas I got a large number of chrome: URLs (with browser.xul and tabbrowser.xul being the most common).

> ::: toolkit/devtools/debugger/server/dbg-browser-actors.js
> @@ +240,5 @@
> > +   * Prepare to enter a nested event loop by disabling debuggee events.
> > +   */
> > +  preNest: function BRA_preNest() {
> > +    let top = windowMediator.getMostRecentWindow("navigator:browser");
> > +    let browser = top.gBrowser.selectedBrowser;
> 
> Would be kind of nice if this file could live up in
> /browser/devtools/debugger somewhere.
> Do any of our other consumers use dbg-browser-actors.js?
> 

This is something I was planning to do, but it didn't seem too important lately. When I implemented browser actor overrides for Fennec and B2G, I thought that it would make sense to split the few Firefox-specific bits into browser/, but then everyone else that wanted to port the debugger to another product (Thunderbird, Instantbird, etc.) would have to fill in the missing bits, instead of overriding broken methods. The tradeoff is better semantics vs. discoverability and it doesn't seem to be much of a difference, but it doesn't look like we gain much either.

> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +81,5 @@
> >    addDebuggee: function TA_addDebuggee(aGlobal) {
> > +    try {
> > +      this.dbg.addDebuggee(aGlobal);
> > +    } catch (e) {
> > +      // Ignore attempts to add the debugger's compartment as a debuggee.
> 
> not even worth a dumpn, eh?

I can do that.

> @@ +88,3 @@
> >  
> > +  /**
> > +   * Initialize the Debugger object using the inspector XPCOM component to turn
> 
> which XPCOM component?

Bah, this whole comment is obsolete. I should have deleted it instead of copying it along with the code.

> @@ +202,4 @@
> >      this.dbg.enabled = true;
> >      try {
> >        // Put ourselves in the paused state.
> >        // XXX: We need to put the debuggee in a paused state too.
> 
> Is there a bug for this XXX comment or can it go now?

Yes, I think it can go away.

> @@ +2205,5 @@
> > +    if (desc) {
> > +      Object.defineProperty(aTarget, key, desc);
> > +    }
> > +  }
> > +}
> 
> check out victor's create() method here:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/
> VariablesView.jsm#1182
> 
> I'm thinking that would be useful sugaring for these cases. We should avoid
> creating too many of them, though don't want to refactor things for this.

Sure. In this patch I'm only moving Nick's function to the bottom, where we generally place helper functions.

> ::: toolkit/devtools/debugger/server/dbg-server.js
> @@ +322,5 @@
> >  
> >    /**
> >     * Remove the connection from the debugging server.
> >     */
> > +  _connectionClosed: function DS_connectionClosed(aConnection) {
> 
> how did those get the DH_ prefix in the first place?

Dunno. Maybe Dave had originally called it the DebuggerHandler?

> @@ +341,5 @@
> > +   *        actorPrefix property of the constructor prototype is used.
> > +   */
> > +  addTabActor: function DS_addTabActor(aFunction, aName) {
> > +    let name = aName ? aName : aFunction.prototype.actorPrefix;
> > +    if (["title", "url", "actor"].indexOf(name) != -1) {
> 
> maybe add a comment for this check indicating that we prevent certain types
> of extensions. If this is going to be public API.

Done.

> @@ +358,5 @@
> > +   * @param aFunction function
> > +   *        The constructor function for this request type.
> > +   */
> > +  removeTabActor: function DS_removeTabActor(aFunction) {
> > +    for (let name in DebuggerServer.tabActorFactories) {
> 
> do we need to prevent the removal of certain types too?

We don't need to, since we make sure never to add a name that clashes with an existing protocol packet property. But even if somehow such names were added to the factory map, removing them can only affect the protocol in a positive way :-)

Comment 27

[Rob Campbell [:rc] (:robcee)]

(In reply to Panos Astithas [:past] from comment #26)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #25)
> > ::: browser/app/profile/firefox.js
> > @@ +1026,5 @@
> > >  pref("devtools.responsiveUI.enabled", true);
> > >  
> > >  // Enable the Debugger
> > >  pref("devtools.debugger.enabled", true);
> > > +pref("devtools.debugger.chrome-enabled", true);
> > 
> > Do we want to do this by default? It's not something most web developers
> > will want, though it is incredibly beautiful and everyone should have a
> > chance to see it.
> 
> In order to get the Browser Debugger menu item one has to flip the
> preferences devtools.debugger.remote-enabled and devtools.chrome.enabled
> from their default values. Does that sound enough, or do you think we need
> the extra devtools.debugger.chrome-enabled pref as well?

No, if you're already checking both .remote-enabled and .chrome.enabled I don't think we need an extra pref for this.

> > ::: browser/devtools/debugger/test/browser_dbg_chrome-debugging.js
> > @@ +47,5 @@
> > > +}
> > > +
> > > +function onNewScript(aEvent, aScript)
> > > +{
> > > +  if (aScript.url.startsWith("chrome:")) {
> > 
> > do we care about about: or resource: uris here? Are they likely to make this
> > fail?
> 
> I'm using a chrome: URL here as proof that scripts that are hidden when
> debugging content are present when debugging chrome. While the test is
> running this handler is called many times with all kinds of scripts, so
> other protocols won't be a problem here.
> 
> You could argue for using about: instead of chrome: here, since they are
> both hidden when debugging content, but in my testing about: URLs were
> rather rare (about:blank basically) when the test was running, whereas I got
> a large number of chrome: URLs (with browser.xul and tabbrowser.xul being
> the most common).

OK, whatever works. Just making sure we're not going to fail surprisingly.

> > ::: toolkit/devtools/debugger/server/dbg-browser-actors.js
> > @@ +240,5 @@
> > > +   * Prepare to enter a nested event loop by disabling debuggee events.
> > > +   */
> > > +  preNest: function BRA_preNest() {
> > > +    let top = windowMediator.getMostRecentWindow("navigator:browser");
> > > +    let browser = top.gBrowser.selectedBrowser;
> > 
> > Would be kind of nice if this file could live up in
> > /browser/devtools/debugger somewhere.
> > Do any of our other consumers use dbg-browser-actors.js?
> 
> This is something I was planning to do, but it didn't seem too important
> lately. When I implemented browser actor overrides for Fennec and B2G, I
> thought that it would make sense to split the few Firefox-specific bits into
> browser/, but then everyone else that wanted to port the debugger to another
> product (Thunderbird, Instantbird, etc.) would have to fill in the missing
> bits, instead of overriding broken methods. The tradeoff is better semantics
> vs. discoverability and it doesn't seem to be much of a difference, but it
> doesn't look like we gain much either.

Yeah, like I said, I'm fine with it where it is and if it serves as documentation for other products then it's useful to keep it where it is.

> > ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> > @@ +81,5 @@
> > >    addDebuggee: function TA_addDebuggee(aGlobal) {
> > > +    try {
> > > +      this.dbg.addDebuggee(aGlobal);
> > > +    } catch (e) {
> > > +      // Ignore attempts to add the debugger's compartment as a debuggee.
> > 
> > not even worth a dumpn, eh?
> 
> I can do that.

OK!

> > @@ +88,3 @@
> > >  
> > > +  /**
> > > +   * Initialize the Debugger object using the inspector XPCOM component to turn
> > 
> > which XPCOM component?
> 
> Bah, this whole comment is obsolete. I should have deleted it instead of
> copying it along with the code.

That's what I thought. FWIW, it's been wrong for awhile.

> > @@ +202,4 @@
> > >      this.dbg.enabled = true;
> > >      try {
> > >        // Put ourselves in the paused state.
> > >        // XXX: We need to put the debuggee in a paused state too.
> > 
> > Is there a bug for this XXX comment or can it go now?
> 
> Yes, I think it can go away.

ditto.

> > @@ +2205,5 @@
> > > +    if (desc) {
> > > +      Object.defineProperty(aTarget, key, desc);
> > > +    }
> > > +  }
> > > +}
> > 
> > check out victor's create() method here:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/
> > VariablesView.jsm#1182
> > 
> > I'm thinking that would be useful sugaring for these cases. We should avoid
> > creating too many of them, though don't want to refactor things for this.
> 
> Sure. In this patch I'm only moving Nick's function to the bottom, where we
> generally place helper functions.

right. Down the road, some nice'n'tidy syntactic sugar for extending classes would be good to have.

> > ::: toolkit/devtools/debugger/server/dbg-server.js
> > @@ +322,5 @@
> > >  
> > >    /**
> > >     * Remove the connection from the debugging server.
> > >     */
> > > +  _connectionClosed: function DS_connectionClosed(aConnection) {
> > 
> > how did those get the DH_ prefix in the first place?
> 
> Dunno. Maybe Dave had originally called it the DebuggerHandler?

Maybe. Lost to history.

> > @@ +341,5 @@
> > > +   *        actorPrefix property of the constructor prototype is used.
> > > +   */
> > > +  addTabActor: function DS_addTabActor(aFunction, aName) {
> > > +    let name = aName ? aName : aFunction.prototype.actorPrefix;
> > > +    if (["title", "url", "actor"].indexOf(name) != -1) {
> > 
> > maybe add a comment for this check indicating that we prevent certain types
> > of extensions. If this is going to be public API.
> 
> Done.

Thanks!

> > @@ +358,5 @@
> > > +   * @param aFunction function
> > > +   *        The constructor function for this request type.
> > > +   */
> > > +  removeTabActor: function DS_removeTabActor(aFunction) {
> > > +    for (let name in DebuggerServer.tabActorFactories) {
> > 
> > do we need to prevent the removal of certain types too?
> 
> We don't need to, since we make sure never to add a name that clashes with
> an existing protocol packet property. But even if somehow such names were
> added to the factory map, removing them can only affect the protocol in a
> positive way :-)

I'm just thinking that if a malicious user wants to mess about with core actors in our Debugger, they could cause some problems by removing the defaults.

I guess if someone really wanted to get around any guards we put in they could and I don't know what that would get them. Probably a broken debugger.

Thanks for the updates!

Comment 28

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

Attachment: Patch v14 [landed]

Addressed review comments.

Comment 29

[Rob Campbell [:rc] (:robcee)]

https://tbpl.mozilla.org/?tree=Try&rev=7341aec346ec

Comment 30

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

Comment on attachment 675197 [details] [diff] [review]
Disable debugging instead of removing debuggees

As Rob pointed out today, optimized builds do not display any beachballing or delays at all, so this must be extra work to keep debugging data around. Therefore the workaround is not necessary, but we may still want the removeAllDebuggees patch for API goodness.

Comment 31

[Rob Campbell [:rc] (:robcee)]

(In reply to Panos Astithas [:past] from comment #28)
> Created attachment 677039 [details] [diff] [review]
> Patch v14
> 
> Addressed review comments.

except for the part where we create a new pref and switch it on by default. Shouldn't be anything added to firefox.js.

Comment 32

[Rob Campbell [:rc] (:robcee)]

this is the time when I reply to myself in bugs.

You were right, Panos, we *do* need the extra pref in case we need to kill-switch the feature. Users shouldn't have to tweak this pref themselves, but will have to turn on remote-debugging and chrome.enabled.

whew.

Comment 33

[Jim Blandy :jimb]

Comment on attachment 675097 [details] [diff] [review]
Implement Debugger.removeAllDebuggees (take 3)

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

This looks great. I'll attach an expanded version of the test. Please look it over and include it if it makes sense; otherwise, let's work out what would be better before landing.

We did once run into problems with a legacy XPCOM interface where we ended up doing a full GC for each compartment we took out of debug mode, yielding quadratic behavior. But I believe the new removeAllDebuggees avoids that problem; only the affected compartments should get marked to participate in the GC.

::: js/src/jit-test/tests/debug/Debugger-debuggees-19.js
@@ +9,5 @@
> +var a1 = dbg.getDebuggees();
> +assertEq(a1.length, 2);
> +assertEq(dbg.removeAllDebuggees(), undefined);
> +var a2 = dbg.getDebuggees();
> +assertEq(a2.length, 0);

I'd like the test to actually check that the globals become debuggees and then become non-debuggees. I'll attach a revision.

Comment 34

[Jim Blandy :jimb]

Attachment: Expanded regression test for removeAllDebuggees

Here's the expanded regression test. I'm going to r? jorendorff on this, since it's a decent bit larger than the original.

Comment 35

[Jim Blandy :jimb]

To be clear: I think we need Jason's r on the test before we can land the patch.

Comment 36

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 677039 [details] [diff] [review]
Patch v14 [landed]

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

Comment 37

[Jason Orendorff [:jorendorff]]

Comment on attachment 677623 [details]
Expanded regression test for removeAllDebuggees

OK.

Comment 38

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

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

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/426b858da27f
https://hg.mozilla.org/mozilla-central/rev/c0b9df7f6230

Comment 40

[Jim Blandy :jimb]

This is tangential, but a disabled Debugger instance should have no effect on performance --- but they do. I've filed bug 809563 about this.