1448499 - Destroy device front when closing toolbox

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

While trying to release https://github.com/devtools-html/debugger.html/pull/5761 to mozilla-central, the following test failed :

devtools/client/framework/test/browser_toolbox_tool_remote_reopen.js

with Front for server1.conn69.deviceActor8 still held in pool

Comment 1

[Julian Descottes [:jdescottes]]

I am not certain what is the best way to destroy the device front here, but given the existing code in tollbox' destroy [1], I assume it is the responsibility of the toolbox to cleanup fronts when it gets destroyed. It would make sense to delete device fronts created specially for the toolbox target.

[1] https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/devtools/client/framework/toolbox.js#2672

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1448499 - Destroy deviceFront when destroying the toolbox;

Review commit: https://reviewboard.mozilla.org/r/230808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230808/

Comment 3

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

Hmm.  So the informal policy (I can't seem to find a document about it for fronts, only a similar mention for actors[1]) is that the module that created the front should also destroy it when no longer needed.  So, that's something I should have mentioned in the debugger PR, oops!

This test is explicitly trying to catch places where destroying was missed, which seems like exactly what happened here.

Is it possible to make a change like that on the debugger side to destroy the front when debugger closes or something simmilar?  Maybe we should switch to a different setup in general if this is too much of a hassle, but it feels a bit odd to me to carve out some special for just the device front in the toolbox like this.

[1]: http://docs.firefox-dev.tools/backend/actor-best-practices.html

Comment 4

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

Comment on attachment 8961969 [details]
Bug 1448499 - Destroy deviceFront when destroying the toolbox;

https://reviewboard.mozilla.org/r/230808/#review236690

As discussed in chat, let's go with this for now so we can land the worker fix.  We can follow up to rethink about these destroy paths.

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8961969 [details]
Bug 1448499 - Destroy deviceFront when destroying the toolbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/230808/diff/1-2/

Comment 6

[Julian Descottes [:jdescottes]]

Ah didn't realize you already r+'d this! thanks for the review, landing.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab3e4b65ae3a
Destroy deviceFront when destroying the toolbox;r=jryans

Comment 8

[Alexandre Poirot [:ochameau]]

Sorry to bring this old eternal bug 1222047 again, but the way to instanciate and destroy fronts is sub-optimal and everyone do its own recipe. Whereas the situation is much better on the server side. We could apply similar techniques to unify how things work around fronts.

Comment 9

[Julian Descottes [:jdescottes]]

I definitely agree, I had a lot of trouble figuring out "how" I was supposed to destroy this front. I think the solution that landed here is okay given the current constraints, but we should resume work on Bug 1222047.

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #9)
> I think the solution that landed here is okay given the current constraints,

Oh yes, I didn't meant to say this bug's patch was incorrect, it just do what we do everywhere.
I just wanted to highlight this yet another beneficial cleanup.

Comment 11

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

Yes, I also agree a standard approach to fronts seems helpful here as follow up work.

Comment 12

[Andrei Ciure[:aciure]]

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

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8961969 [details]
Bug 1448499 - Destroy deviceFront when destroying the toolbox;

This uplift request is the first part for a workaround to Bug 1443550.
The root issue is that starting with Firefox 59, you can no longer use the debugger to debug Fennec (see Bug 1443550 for details). 

The patch attached in Bug 1443550 will fix Fennec to allow the debugger to connect, but in the meantime we want to uplift a client-side workaround that will allow the debugger to connect even to Fennec 59. It is less risky than uplifting the Fennec change and with this, users will have a usable debugger for all versions of Fennec, starting with FF60.

The second part of this workaround will be a debugger update (v19.6)

Approval Request Comment
[Feature/Bug causing the regression]: Broken since FF59, although I don't have a specific commit to point to.
[User impact if declined]: Users cannot debug Fennec 59 and Fennec 60 unless they use Firefox 61. 
[Is this code covered by automated tests?]: Yes, devtools/client/framework/test/browser_toolbox_tool_remote_reopen.js
[Has the fix been verified in Nightly?]: No but this bug alone has no visible impact so except checking that the test mentioned above passes, there is not much to check.
[Needs manual test from QE? If yes, steps to reproduce]: no, covered by automated tests.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No, 
[Why is the change risky/not risky?]:DevTools only, we are just destroying an additional object when closing the toolbox
[String changes made/needed]: none

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: patch-uplift.patch

(needs rebase for beta, same comments apply)

Approval Request Comment
[Feature/Bug causing the regression]: Broken since FF59, although I don't have a specific commit to point to.
[User impact if declined]: Users cannot debug Fennec 59 and Fennec 60 unless they use Firefox 61. 
[Is this code covered by automated tests?]: Yes, devtools/client/framework/test/browser_toolbox_tool_remote_reopen.js
[Has the fix been verified in Nightly?]: No but this bug alone has no visible impact so except checking that the test mentioned above passes, there is not much to check.
[Needs manual test from QE? If yes, steps to reproduce]: no, covered by automated tests.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No, 
[Why is the change risky/not risky?]:DevTools only, we are just destroying an additional object when closing the toolbox
[String changes made/needed]: none

Comment 15

[Julian Descottes [:jdescottes]]

The second part of the uplift will be in Bug 1452203

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8965806 [details] [diff] [review]
patch-uplift.patch

Fix for broken Fennec debugging. Approved for 60.0b11.

Comment 17

[Julian Descottes [:jdescottes]]

Attachment: patch-uplift-rebased.patch

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8966209 [details] [diff] [review]
patch-uplift-rebased.patch

Same as previous, just rebased.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4ec5bcc4d729