Closed Bug 1448499 Opened 2 years ago Closed 2 years ago

Destroy device front when closing toolbox

Categories

(DevTools :: Framework, enhancement, P3)

enhancement

Tracking

(firefox60 fixed, firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(2 files, 1 obsolete file)

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
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
Attachment #8961969 - Flags: feedback?(jryans)
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
Flags: needinfo?(jdescottes)
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.
Attachment #8961969 - Flags: review+
Flags: needinfo?(jdescottes)
Attachment #8961969 - Flags: feedback?(jryans) → feedback+
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Ah didn't realize you already r+'d this! thanks for the review, landing.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab3e4b65ae3a
Destroy deviceFront when destroying the toolbox;r=jryans
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.
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.
(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.
Yes, I also agree a standard approach to fronts seems helpful here as follow up work.
https://hg.mozilla.org/mozilla-central/rev/ab3e4b65ae3a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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
Attachment #8961969 - Flags: approval-mozilla-beta?
Attached patch patch-uplift.patch (obsolete) — Splinter Review
(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
Attachment #8965806 - Flags: approval-mozilla-beta?
Attachment #8961969 - Flags: approval-mozilla-beta?
Blocks: 1452203
The second part of the uplift will be in Bug 1452203
Attachment #8965806 - Attachment is patch: true
Comment on attachment 8965806 [details] [diff] [review]
patch-uplift.patch

Fix for broken Fennec debugging. Approved for 60.0b11.
Attachment #8965806 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8965806 - Attachment is obsolete: true
Comment on attachment 8966209 [details] [diff] [review]
patch-uplift-rebased.patch

Same as previous, just rebased.
Attachment #8966209 - Flags: approval-mozilla-beta?
Attachment #8966209 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.