Closed Bug 1186940 Opened 4 years ago Closed 4 years ago

Leaking Debugger object on toolbox close

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → poirot.alex
Attachment #8637974 - Flags: review?(jryans)
Summary: Leaking Debuger object on toolbox close → Leaking Debugger object on toolbox close
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 "("
Attachment #8637974 - Flags: review?(jryans) → review+
Attached patch patch v2Splinter Review
Attachment #8637974 - Attachment is obsolete: true
Attachment #8639193 - Flags: review+
(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.
https://hg.mozilla.org/mozilla-central/rev/6a1fa063e520
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.