1186940 - Leaking Debugger object on toolbox close

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

While looking at bug 1181122, I figured out they we were leaking all Debugger objects... If we close the toolbox, actors are meant to be destroyed. Some are correctly collected, others aren't (see bug 1186937 for ex). But even if the actor is destroyed, the Debugger objects created via makeDebugger() aren't garbage collected.
From what I've seen, some references are kept alive from makeDebugger arrow function. This is really weird. There might be something broken at CC level or something where a cycle is keeping things alive.
I'm saying that because I'm just breaking a cycle, I don't see anything that hold this whole cycle alive.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc1ea5b25961

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8637974 [details] [diff] [review]
patch v1

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

Seems like it would be better to know why this caused a leak...  but let's at least get a fix in for sure!

::: toolkit/devtools/server/actors/utils/make-debugger.js
@@ +67,5 @@
>    EventEmitter.decorate(dbg);
>  
>    dbg.uncaughtExceptionHook = reportDebuggerHookException;
>  
> +  dbg.onNewGlobalObject = function (global) {

Nit: Current DevTools style disallows[1] a space between "function" and "("

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/.eslintrc#305

@@ +74,4 @@
>      }
>    };
>  
> +  dbg.addDebuggees = function () {

Nit: Current DevTools style disallows[1] a space between "function" and "("

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Comment 4

[Pulsebot]

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

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) (on PTO July 27 - 29) from comment #2)
> Comment on attachment 8637974 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8637974 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems like it would be better to know why this caused a leak...  but let's
> at least get a fix in for sure!

I thought it was some platform leak, due to Debugger implementation in C++,
but I'm not sure as I tried to reproduce this leak with a simple test case without any success (no leak):

  let d = new Debugger();
  let w = Cu.getWeakReference(d);
  d.onNewGlobalObject = () => {return d;};
  d.addDebuggees = () => {return d;};
  d.addDebuggees(window);
  d = null;
  Cu.forceGC();
  !!w.get(); // false -> `d` is freed

The issue is that I didn't got clear enough information out of CCAnalyzer to figure out exactly why it was leaking. I've just seen that the arrow function was holding `dbg` variable in its scope (which makes sense). But somehow we we most likely leaking addDebuggees somewhere else that I haven't been able to see in CCAnalyzer data.

See my comment in bug 1175871 to see how I'm figuring out these leaks.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a1fa063e520