820524 - Debugger, Web Console and Profiler should share the debugger client even for local sessions

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

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

Remote connections get a shared debugger client residing in the RemoteTarget object, but local ones use their separate ones. We should fix that.

Comment 1

[Paul Rouget [:paul]]

How important this is? Can Firefox 20 ship without that?

Comment 2

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

I think so, yes.

Comment 3

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

Since this bug blocks P1 bug 821345, I'll call it P1 too.

Comment 4

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

This should include the profiler as well. AFAICS the profiler uses the remote protocol even for the local case, so in that respect it is more similar to the debugger than the console. That also means that we need to add the profiler in the remote toolbox as well.

CCing Anton to make sure I'm not hallucinating.

Comment 5

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

Bug 827083 shows that the remote web console is broken, and I'll need to base this patch on those fixes.

Comment 6

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

Attachment: WIP

The mechanism is in place and the debugger seems to work fine with it. I'll take a look at the console next.

Comment 7

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

Attachment: WIP 2

The previous version broke the recent change to the inspector that displays a warning when the debugger is paused. Fixed it with a slight rearrangement of the relationship between the TabTarget and its child RemoteTarget. All tests pass now. Web Console tomorrow.

Comment 8

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

Attachment: WIP 3

Web Console and Debugger now share the target even in the local case. I need to fix some web console tests that are now broken and do some cleanups.

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 702984 [details] [diff] [review]
WIP 3

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

::: browser/devtools/webconsole/webconsole.js
@@ +391,5 @@
> +    } else {
> +      // Local debugging needs to convert the TabTarget to a RemoteTarget.
> +      this.owner.target.remote().then(function success(aRemoteTarget) {
> +        this.owner.target = aRemoteTarget;
> +        this.proxy = new WebConsoleConnectionProxy(this, this.owner.target);

To avoid breakage do not overwrite this.owner.target with the remote target object. Tests assume they work with a TabTarget and they access the .tab property - in the local tab scenario this actually makes sense. This is how getHudByWindow() works.

Also, target.tab is needed for the clear console button - this allows the HUDService.jsm to clear the errors count in the developer toolbar for the current tab.

I believe you can give the remoteTarget object only to the WCCP constructor. HUDService works with the TabTarget and WCCP works with the RemoteTarget for that tab.

Comment 10

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

Comment on attachment 702984 [details] [diff] [review]
WIP 3

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

::: browser/devtools/debugger/debugger-controller.js
@@ +95,5 @@
>  
>    /**
>     * Prepares the hostname and port number for a remote debugger connection
>     * and handles connection retries and timeouts.
> +   * XXX: remove all that

Link to bug 823577 so we don't forget :)

@@ -174,5 @@
> -    // Content or chrome debugging can connect directly to the debuggee.
> -    // TODO: convert this to use a TabTarget.
> -    let transport = window._isChromeDebugger
> -      ? debuggerSocketConnect(Prefs.remoteHost, Prefs.remotePort)
> -      : DebuggerServer.connectPipe();

Doesn't this break the actual chrome debugging (starting the browser debugger via the menuitem, not the connect screen)? The above call to this._startChromeDebugging(client, dbg, callback); doesn't spawn a new process.

@@ +378,5 @@
>     */
>    _update: function TS__update(aEvent) {
>      DebuggerView.Toolbar.toggleResumeButtonState(this.activeThread.state);
>  
> +    // Notify the RemoteTarget and any parent TabTarget of his.

s/his/this

Comment 11

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

Attachment: WIP 4

Thanks for the comments guys, they were very helpful!
I've fixed some of them (not the Browser Debugger, yet), but I still have some broken tests and use cases (opening the toolbox twice for instance). More to come.

Comment 12

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

Attachment: WIP 5

Fixed chrome debugging, a web console initialization bug and aligned the tabNavigated notification better with the updated spec. Still trying to fix web console reopening, which is broken with this patch.

Comment 13

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

Attachment: WIP 6

All manual tests pass now, but I still need to fix 4 console mochitests, and figure out what caused the regression of debugger mochitests.

Comment 14

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

Attachment: WIP 7

Fixed the debugger regression and run all the devtools tests. 6 of them are still failing (web console, gcli & framework). I'm not sure if I'll work on those before investigating the alternative API we discussed today.

Comment 15

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

Attachment: WIP 8

Fixed the gcli test, 5 more to go.

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 705056 [details] [diff] [review]
WIP 8

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

Just a couple of comments for issues I found while working with this patch.

::: browser/devtools/framework/Toolbox.jsm
@@ +645,5 @@
>    destroy: function TBOX_destroy() {
>      // If several things call destroy then we give them all the same
>      // destruction promise so we're sure to destroy only once
>      if (this._destroyer) {
> +      return this._destroyer.promise;

this._destroyer is already a promise.

@@ +674,5 @@
>  
>      this._destroyer = Promise.all(outstanding);
>      this._destroyer.then(function() {
>        this.emit("destroyed");
> +      this._destroyer.resolve(null);

this._destroyer is not a defer object.

resolve() is not a function

Comment 17

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

Attachment: WIP 9

Fixed the bug found by Mihai (thanks!) and another minor one with remote JS debugging.

Comment 18

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

Attachment: Patch to incorporate RemoteTarget into TabTarget

This is the alternative API that we were discussing last week. Applying this patch on top of the other, we get to keep only the TabTarget abstraction, which now incorporates the functionality of the RemoteTarget. Now the local case can call makeRemote to augment the functionality of the TabTarget (which is a nice thing), but a remote debugging case now has to provide a "dummy" object to initialize the TabTarget before eventually calling makeRemote (which is not so nice).

I don't feel particularly strong about the relative merits of each of these API approaches, so I'd like feedback from those that do. There are a few tests that still fail with both approaches, but all the manual testing I have done assures me that both patches are almost there.

Comment 19

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

Comment on attachment 706040 [details] [diff] [review]
WIP 9

Adding feedback flags to make sure this doesn't slip through the cracks.

Comment 20

[Joe Walker [:jwalker]]

Comment on attachment 707276 [details] [diff] [review]
Patch to incorporate RemoteTarget into TabTarget

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

What if we didn't get rid of RemoteTarget, but instead, had TabTarget do everything that RemoteTarget does? The idea being that we can treat every target as if it was remote, but there are some targets which have a special extra 'tab' property.

Over time the requirement for a TabTarget should go away.

Comment 21

[Mihai Sucan [:msucan]]

(In reply to Joe Walker [:jwalker] from comment #20)
> Comment on attachment 707276 [details] [diff] [review]
> Patch to incorporate RemoteTarget into TabTarget
> 
> Review of attachment 707276 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What if we didn't get rid of RemoteTarget, but instead, had TabTarget do
> everything that RemoteTarget does? The idea being that we can treat every
> target as if it was remote, but there are some targets which have a special
> extra 'tab' property.
> 
> Over time the requirement for a TabTarget should go away.

Why not have only a RemoteTarget, with an optional |tab| property? Could be simpler...

Comment 22

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

(In reply to Joe Walker [:jwalker] from comment #20)
> What if we didn't get rid of RemoteTarget, but instead, had TabTarget do
> everything that RemoteTarget does? The idea being that we can treat every
> target as if it was remote, but there are some targets which have a special
> extra 'tab' property.
> 
> Over time the requirement for a TabTarget should go away.

I could try that. It seemed less clean, but maybe that's what the interim state should look like.

(In reply to Mihai Sucan [:msucan] from comment #21)
> Why not have only a RemoteTarget, with an optional |tab| property? Could be
> simpler...

That's pretty much what the combination of the two patches does, with the small difference that it calls the combined object TabTarget.

Comment 23

[Joe Walker [:jwalker]]

(In reply to Panos Astithas [:past] from comment #22)
> (In reply to Joe Walker [:jwalker] from comment #20)
> > What if we didn't get rid of RemoteTarget, but instead, had TabTarget do
> > everything that RemoteTarget does? The idea being that we can treat every
> > target as if it was remote, but there are some targets which have a special
> > extra 'tab' property.
> > 
> > Over time the requirement for a TabTarget should go away.
> 
> I could try that. It seemed less clean, but maybe that's what the interim
> state should look like.
> 
> (In reply to Mihai Sucan [:msucan] from comment #21)
> > Why not have only a RemoteTarget, with an optional |tab| property? Could be
> > simpler...
> 
> That's pretty much what the combination of the two patches does, with the
> small difference that it calls the combined object TabTarget.

Ah - at first reading, I missed that makeRemote returns a promise of |this| because I saw code like this:

  target.makeRemote().then(function(newTarget) { newTarget.foo(); });

Perhaps we could just make the promise resolve to null, so instead:

  target.makeRemote().then(function() { target.foo(); });

Then perhaps the next step is to push the makeRemote into the construction process, so targets come pre-remoted, rather than being remotable.

Thanks.

Comment 24

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

Attachment: WIP 11

Made the change suggested in comment 23 and also made similar changes to the profiler. I still have test failures and leaks though, so this isn't quite ready for review, yet.

Comment 25

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

Attachment: WIP 12

Test failures fixed, but leaks remain. Trying to understand more of the framework tests that are the main culprit.

Comment 26

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

Attachment: WIP 13

Fixed leaks in regular usage and some tests, but not all tests.

Comment 27

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

Attachment: Patch v14

More fixes, but I still can't get the leaks to disappear from tests. The current status is that most devtools subdirectories pass cleanly, without leaks, except for framework, styleinspector, markupview and styleeditor IIRC. The problem is that the tools I've touched in this patch and the tools I'm more familiar with are clean, so I've been spending my time reading the inspector code, while trying to make sense of the leaks.

I could definitely use some help here, so I'm asking for review even though I still have some dump() calls in there, and I know this can't land as is due to the test leaks. Maybe a fresh pair of eyes can see something obvious I'm not seeing, or point out problems in how the changes interact with the styling tools that I'm not aware of.

My favorite (not!) test is browser_toolbox_hosts.js that leaks 4 windows during the last call to showToolbox. To my astonishment, when I explicitly forced GC before finishing the test, it still complained about a leak, even though the mentioned window IDs were clearly marked in the log as scavenged before the test finished. Before starting to debug our test harness, I thought I'd ask for some help.

Comment 28

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

Attachment: Patch v15

Come to think of it, I didn't have any good reason to bother you with my debugging dump()s, so I took them out.

Comment 29

[Joe Walker [:jwalker]]

Comment on attachment 715966 [details] [diff] [review]
Patch v15

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

Sorry it wook me a while.

::: browser/devtools/framework/Toolbox.jsm
@@ +428,4 @@
>      if (this._currentToolId == id) {
> +      // Return the existing panel in order to have a consistent return value.
> +      deferred.resolve(this._toolPanels.get(id));
> +      return deferred.promise;

Or:

  return Promise.resolve(this._toolPanels.get(id));

@@ +650,5 @@
>      if (this._destroyer) {
>        return this._destroyer;
>      }
>  
> +    this._destroyer = Promise.defer().promise;

I think you mean:
  this._destroyer = Promise.resolve();

The former will return an unresolvable promise?

::: browser/devtools/framework/test/browser_new_activation_workflow.js
@@ +51,5 @@
>  
>  function testToggle() {
>    toolbox.once("destroyed", function() {
> +    // Give the debugger server a chance to shut down properly.
> +    executeSoon(function() {

I worry about us adding these. Do we know that the destruction happens for real in the event loop after then event is dispatched, or are we just getting lucky that the scheduler is doing the destroy before the executeSoon?

Comment 30

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

Attachment: Patch v16

Made all the requested changes and took out all other instances of executeSoon that I had added. The test leaks are still there, though. Two new data points are:
- the tests that leak are for tools that are not remotable
- the leaked windows (dozens of them) are either about:blank or chrome://browser/content/devtools/csshtmltree.xul

In the meantime I'd like Anton, Mihai and Victor to vet the profiler, web console and debugger changes respectively.

Comment 31

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

Comment on attachment 717035 [details] [diff] [review]
Patch v16

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

Debugger changes are exquisite.

::: browser/devtools/debugger/debugger-controller.js
@@ +173,4 @@
>  
>        if (this._target.chrome) {
>          let dbg = this._target.form.chromeDebugger;
>          this._startChromeDebugging(client, dbg, callback);

This is still a bit fishy, but the context of this being here has so many ramifications, I'll pretend everything's fine.

Comment 32

[Mihai Sucan [:msucan]]

Comment on attachment 717035 [details] [diff] [review]
Patch v16

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

Patch looks good. Thank you for the fixes!

I found an error that I would like fixed:

TEST-START | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_621644_jsterm_dollar.js
console.error:
ReferenceError
  - prototype ReferenceError
    - columnNumber = 8
    - fileName = chrome://mochitests/content/browser/browser/devtools/webconsole/test/head.js
    - lineNumber = 169
  - prototype Error
  - prototype Object

I see the same error in browser_webconsole_null_and_undefined_output.js. Can you please fix it? This a new error with this patch.

More comments below. r+ with these changes.

::: browser/devtools/framework/Toolbox.jsm
@@ +649,5 @@
>      if (this._destroyer) {
>        return this._destroyer;
>      }
>  
> +    this._destroyer = Promise.resolve();

Do you need this? A few lines below you override this._destroyer with a Promise.all(outstanding).

::: browser/devtools/webconsole/WebConsolePanel.jsm
@@ +8,5 @@
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");

Please make this a lazy module getter to avoid loading promise.js when it's not needed.

@@ +81,5 @@
>      this._destroyer.then(function() {
>        this.emit("destroyed");
>      }.bind(this));
>  
> +    return this._destroyer.promise;

Is this change needed/correct? hud.destroy() returns a promise, not a deferred object, IIANM.

Comment 33

[Anton Kovalyov (:anton)]

Comment on attachment 717035 [details] [diff] [review]
Patch v16

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

I had a couple of stylistic comments and one question, but overall code looks good. That said, when I ran tests locally (debug build) they failed with memory leak reports:

 0:15.83 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 2997707 bytes during test execution
 0:15.83 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 1 instance of AsyncStatement with size 88 bytes
 0:15.83 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 1682 instances of AtomImpl with size 40 bytes each (67280 bytes total)
 0:15.83 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 48 bytes
 0:15.83 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 1 instance of CalculateFrecencyFunction with size 24 bytes
 0:15.83 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 4 instances of CallbackObject with size 32 bytes each (128 bytes total)

I remember debugging these exact leaks so this sounds like a regression. I'll try to find reasons these failures happened previously—hopefully it'll help with the debugging.

::: browser/devtools/profiler/ProfilerController.jsm
@@ -128,5 @@
>    /**
>     * Cleanup.
>     */
>    destroy: function PCn_destroy() {
> -    this.client.close(function () {

Is this.client.close unnecessary?

::: browser/devtools/profiler/ProfilerPanel.jsm
@@ +265,3 @@
>  
> +    return promise
> +      .then(function(aTarget) {

A tiny nit: we don't use aVar style in this file (and in the profiler/ code in general) so I'd change that to `target` for consistency.

@@ +286,5 @@
>        }.bind(this))
> +      .then(null, function onError(aReason) {
> +        let msg = "ProfilerPanel open failed. " +
> +                  aReason.error + ": " + aReason.message;
> +        dump(msg + "\n");

Do we really need `dump` here? I think `Cu.reportError` is enough.

Comment 34

[Anton Kovalyov (:anton)]

This is the patch where I fixed these leaks: https://bugzilla.mozilla.org/attachment.cgi?id=710381&action=edit I fixed them by cleaning up on Debugger:Shutdown. Since you haven't touched the code at all I assume there's some other property that needs to be cleaned up and that property is preventing the debugger instance from being cleared. I might be wrong tho.

Comment 35

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

Attachment: Patch v17

(In reply to Mihai Sucan [:msucan] from comment #32)
> I found an error that I would like fixed:
> 
> TEST-START |
> chrome://mochitests/content/browser/browser/devtools/webconsole/test/
> browser_webconsole_bug_621644_jsterm_dollar.js
> console.error:
> ReferenceError
>   - prototype ReferenceError
>     - columnNumber = 8
>     - fileName =
> chrome://mochitests/content/browser/browser/devtools/webconsole/test/head.js
>     - lineNumber = 169
>   - prototype Error
>   - prototype Object
> 
> I see the same error in browser_webconsole_null_and_undefined_output.js. Can
> you please fix it? This a new error with this patch.

I don't see this error locally and I also haven't touched that file. Is there anything different in your setup? Do you have any other patches in your queue?

> ::: browser/devtools/framework/Toolbox.jsm
> @@ +649,5 @@
> >      if (this._destroyer) {
> >        return this._destroyer;
> >      }
> >  
> > +    this._destroyer = Promise.resolve();
> 
> Do you need this? A few lines below you override this._destroyer with a
> Promise.all(outstanding).

The reason for this is to guard against re-entrance from the call to target.destroy(). Since we are using the _destroyer property as a sentinel for re-entrance, it has to be initialized as soon as the method starts. This is always a good pattern to follow, but the addition of the call to target.destroy() makes it now indispensable.

> ::: browser/devtools/webconsole/WebConsolePanel.jsm
> @@ +8,5 @@
> >  
> >  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> >  
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
> 
> Please make this a lazy module getter to avoid loading promise.js when it's
> not needed.

OK, but since tool panel definitions will be loaded after the toolbox, the promise module will already be in the cache, so this might well be extra overhead instead.

> @@ +81,5 @@
> >      this._destroyer.then(function() {
> >        this.emit("destroyed");
> >      }.bind(this));
> >  
> > +    return this._destroyer.promise;
> 
> Is this change needed/correct? hud.destroy() returns a promise, not a
> deferred object, IIANM.

Good catch!

(In reply to Anton Kovalyov (:anton) from comment #33)
> I had a couple of stylistic comments and one question, but overall code
> looks good. That said, when I ran tests locally (debug build) they failed
> with memory leak reports:

Like I said in comment 30, this patch is known to leak and I am not going to land it before fixing those leaks. I only meant to solicit feedback on the changes themselves, and maybe even a hint or two on what might be triggering them :-)

> ::: browser/devtools/profiler/ProfilerController.jsm
> @@ -128,5 @@
> >    /**
> >     * Cleanup.
> >     */
> >    destroy: function PCn_destroy() {
> > -    this.client.close(function () {
> 
> Is this.client.close unnecessary?

Yes, one of the main changes in this patch is to move responsibility for client shutdown from the individual tools to the target. Otherwise the tools will race to close it causing errors. The same change was made to both console and debugger.

> ::: browser/devtools/profiler/ProfilerPanel.jsm
> @@ +265,3 @@
> >  
> > +    return promise
> > +      .then(function(aTarget) {
> 
> A tiny nit: we don't use aVar style in this file (and in the profiler/ code
> in general) so I'd change that to `target` for consistency.

Oh, I didn't see that. Note that the rest of devtools and Firefox frontend use this style, but personally I don't care either way.

> @@ +286,5 @@
> >        }.bind(this))
> > +      .then(null, function onError(aReason) {
> > +        let msg = "ProfilerPanel open failed. " +
> > +                  aReason.error + ": " + aReason.message;
> > +        dump(msg + "\n");
> 
> Do we really need `dump` here? I think `Cu.reportError` is enough.

Note that you need to have the Error Console open in order to see errors from Cu.reportError, so in the debugger we use both: reportError for end users to file bugs and dump for us to observe the errors while debugging. That being said, I can understand your workflow may be different, so I took it out. I also changed aReason to reason per your previous comment.

Comment 36

[Anton Kovalyov (:anton)]

Comment on attachment 718425 [details] [diff] [review]
Patch v17

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

LGTM.

Comment 37

[Mihai Sucan [:msucan]]

Thanks for the updated patch!


(In reply to Panos Astithas [:past] from comment #35)
> Created attachment 718425 [details] [diff] [review]
> Patch v17
> 
> (In reply to Mihai Sucan [:msucan] from comment #32)
> > I found an error that I would like fixed:
> > 
> > TEST-START |
> > chrome://mochitests/content/browser/browser/devtools/webconsole/test/
> > browser_webconsole_bug_621644_jsterm_dollar.js
> > console.error:
> > ReferenceError
> >   - prototype ReferenceError
> >     - columnNumber = 8
> >     - fileName =
> > chrome://mochitests/content/browser/browser/devtools/webconsole/test/head.js
> >     - lineNumber = 169
> >   - prototype Error
> >   - prototype Object
> > 
> > I see the same error in browser_webconsole_null_and_undefined_output.js. Can
> > you please fix it? This a new error with this patch.
> 
> I don't see this error locally and I also haven't touched that file. Is
> there anything different in your setup? Do you have any other patches in
> your queue?

Just pulled from fx-team and applied only the latest patch from this bug. Same error. I'm testing on Ubuntu.

Also do note that the patch doesn't apply cleanly, there's a simple failure in Toolbox.jsm. I had to fix that in order to get the patch to apply.

Do you have other patches in your queue?


> > ::: browser/devtools/framework/Toolbox.jsm
> > @@ +649,5 @@
> > >      if (this._destroyer) {
> > >        return this._destroyer;
> > >      }
> > >  
> > > +    this._destroyer = Promise.resolve();
> > 
> > Do you need this? A few lines below you override this._destroyer with a
> > Promise.all(outstanding).
> 
> The reason for this is to guard against re-entrance from the call to
> target.destroy(). Since we are using the _destroyer property as a sentinel
> for re-entrance, it has to be initialized as soon as the method starts. This
> is always a good pattern to follow, but the addition of the call to
> target.destroy() makes it now indispensable.

I see your point, but I didn't refer to this aspect of the code. Sorry, maybe I was confusing.

Copy from the patch:

   destroy: function TBOX_destroy() {
     // If several things call destroy then we give them all the same
     // destruction promise so we're sure to destroy only once
     if (this._destroyer) {
       return this._destroyer;
     }
 
+    this._destroyer = Promise.resolve();
     let outstanding = [];
     .....
     this._destroyer = Promise.all(outstanding);

this._destroyer is always the promise returned by Promise.all(). Promise.resolve() is redundant. Anyhow, this is going to change with the rebase - TBOX_destroy() no longer has |this._destroy = Promise.all(outstanding)|, it's slightly changed.

I expect that I'm either missing a patch or I wrongly rebased this rejected hunk. See the rebased patch here:

https://github.com/mihaisucan/mozilla-patch-queue/blob/master/patches-webconsole/bug-820524

As I understood your changes: you changed the order of destroying various objects - you moved the target destroy call after panels destroy, and you added a call to |target.off("close", this.destroy)|, for all targets, not just remote targets.


> > ::: browser/devtools/webconsole/WebConsolePanel.jsm
> > @@ +8,5 @@
> > >  
> > >  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> > >  
> > >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > > +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
> > 
> > Please make this a lazy module getter to avoid loading promise.js when it's
> > not needed.
> 
> OK, but since tool panel definitions will be loaded after the toolbox, the
> promise module will already be in the cache, so this might well be extra
> overhead instead.

Good point. I just suggested the change because I prefer to be conservative about when we do Cu.imports(). The overhead caused by a lazy getter is insignificant compared to a module load when it might not needed.

Comment 38

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

Attachment: Patch v18

Rebased and fixed some breakage that occurred due to bug 734664. Also made target.destroy wait for the debugger server to finish, and fixed a listener removal in markup view. Still getting leaks in inspector tests though.

(In reply to Mihai Sucan [:msucan] from comment #37)
> (In reply to Panos Astithas [:past] from comment #35)
> > I don't see this error locally and I also haven't touched that file. Is
> > there anything different in your setup? Do you have any other patches in
> > your queue?
> 
> Just pulled from fx-team and applied only the latest patch from this bug.
> Same error. I'm testing on Ubuntu.
> 
> Also do note that the patch doesn't apply cleanly, there's a simple failure
> in Toolbox.jsm. I had to fix that in order to get the patch to apply.
> 
> Do you have other patches in your queue?

As discussed in IRC I couldn't reproduce this, but I think it's just a strict warning that doesn't break tests.

> > The reason for this is to guard against re-entrance from the call to
> > target.destroy(). Since we are using the _destroyer property as a sentinel
> > for re-entrance, it has to be initialized as soon as the method starts. This
> > is always a good pattern to follow, but the addition of the call to
> > target.destroy() makes it now indispensable.
> 
> I see your point, but I didn't refer to this aspect of the code. Sorry,
> maybe I was confusing.
> 
> Copy from the patch:
> 
>    destroy: function TBOX_destroy() {
>      // If several things call destroy then we give them all the same
>      // destruction promise so we're sure to destroy only once
>      if (this._destroyer) {
>        return this._destroyer;
>      }
>  
> +    this._destroyer = Promise.resolve();
>      let outstanding = [];
>      .....
>      this._destroyer = Promise.all(outstanding);
> 
> this._destroyer is always the promise returned by Promise.all().
> Promise.resolve() is redundant. Anyhow, this is going to change with the
> rebase - TBOX_destroy() no longer has |this._destroy =
> Promise.all(outstanding)|, it's slightly changed.

The purpose of setting _destroyer to a throw-away value was precisely to avoid re-entrancy from the calls before Promise.all. But, as you also discovered, the patch was bitrotten by bug 734664, which made a similar change. After that, I had to remove the line in question in this version of the patch.

Comment 39

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

Attachment: Patch v19

Baby steps: destroying the panels after the ToolboxHost fixed some of the leaks.

Comment 40

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

Attachment: Patch v20

Finally fixed all leaks locally! \o/
In retrospect the fix is rather obvious: when target.destroy() is called before toolbox.destroy(), it should notify the latter and wait patiently while the toolbox orchestrates an orderly shutdown.

Another change I reverted from a previous iteration of the patch is to wait for the debugger server to shutdown, because it was causing leaks in scenarios where entire windows were abruptly torn down (we had a couple of tests that exercised that path).

Try run: https://tbpl.mozilla.org/?tree=Try&rev=8e4222337d4b

Joe, can you take another look to make sure I didn't miss anything?

Comment 41

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

I tried to make it easier for you to see what has changed since your last review, but unfortunately interdiff barfs on these patches:

https://bugzilla.mozilla.org/attachment.cgi?oldid=715966&action=interdiff&newid=720059&headers=1

Comment 42

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

Try is green, the oranges are unrelated bugs.

Comment 43

[Joe Walker [:jwalker]]

Comment on attachment 719524 [details] [diff] [review]
Patch v19

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

::: browser/devtools/framework/Target.jsm
@@ +411,3 @@
>      }
>  
> +    return Promise.all([dbgServerDestroyer.promise, this._destroyer.promise]);

So there is a potential problem here in that the first person to call target.destroy will get notified when the target is destroyed and when the debug server is destroyed, but the second person will be notified slightly earlier, with no delay while the debug server is destroyed.

Perhaps at the end we should say:

this.destroyer = Promise.all([dbgServerDestroyer.promise, this._destroyer.promise]);
return this.destroyer.promise;

(Now clearly that's broken because we have confusion between deferreds and promises, so I suggest keeping this.destroyer being the destroy promise, and just having the deferred as a local variable.)

Also where did you get Promise.all from? I'm concerned that either I'm out of date, or that we're propagating a monster hack.

Comment 44

[Joe Walker [:jwalker]]

Wait - the above was old code. Looking again at v20.

Comment 45

[Mihai Sucan [:msucan]]

I am running all of the tests from browser/devtools/framework/test. I see the following errors:

- browser_new_activation_workflow.js:
  http://www.pastebin.mozilla.org/2198828
- and browser_toolbox_window_title_changes.js
  http://www.pastebin.mozilla.org/2198829

Are these errors known? When I run the same tests without this patch I only get the following error: http://www.pastebin.mozilla.org/2198831

Comment 46

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

(In reply to Mihai Sucan [:msucan] from comment #45)
> I am running all of the tests from browser/devtools/framework/test. I see
> the following errors:
> 
> - browser_new_activation_workflow.js:
>   http://www.pastebin.mozilla.org/2198828
> - and browser_toolbox_window_title_changes.js
>   http://www.pastebin.mozilla.org/2198829
> 
> Are these errors known? When I run the same tests without this patch I only
> get the following error: http://www.pastebin.mozilla.org/2198831

If the tests pass, then yes, I have been spotting similar errors in our tests occasionally. The problem is that the tests are written in a way that doesn't correspond to how a user will use the tools, e.g. by switching to another tool or abruptly destroying the toolbox while loading.

I do believe that we should have better tests, but this is not something that should be done in this bug IMO, nor am I the right person to do it, for the tests that exercise tools I am not that familiar with.

Comment 47

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #46)
> (In reply to Mihai Sucan [:msucan] from comment #45)
> > I am running all of the tests from browser/devtools/framework/test. I see
> > the following errors:
> > 
> > - browser_new_activation_workflow.js:
> >   http://www.pastebin.mozilla.org/2198828
> > - and browser_toolbox_window_title_changes.js
> >   http://www.pastebin.mozilla.org/2198829
> > 
> > Are these errors known? When I run the same tests without this patch I only
> > get the following error: http://www.pastebin.mozilla.org/2198831
> 
> If the tests pass, then yes, I have been spotting similar errors in our
> tests occasionally. The problem is that the tests are written in a way that
> doesn't correspond to how a user will use the tools, e.g. by switching to
> another tool or abruptly destroying the toolbox while loading.

They all pass, I was only checking that this is expected.

> I do believe that we should have better tests, but this is not something
> that should be done in this bug IMO, nor am I the right person to do it, for
> the tests that exercise tools I am not that familiar with.

Agreed. We shouldn't have such errors showing up randomly in our tests.

Comment 48

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

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

Comment 49

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

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