Closed
Bug 966991
Opened 11 years ago
Closed 11 years ago
App actor should be cleaned up on disconnection
Categories
(DevTools Graveyard :: WebIDE, defect)
DevTools Graveyard
WebIDE
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: ochameau, Assigned: ochameau)
Details
Attachments
(1 file, 4 obsolete files)
|
(deleted),
patch
|
Details | Diff | Splinter Review |
Follow up of bug 962541 discussions.
So far, we are caching app actors in a map in the app actor.
But on client disconnection, we loose this map and let these actor instances alive in the child process. On next connection, we instanciate new set of actors.
We should clean these actors on client disconnection.
|
||
Updated•11 years ago
|
Summary: App actor should be cleaned up on disctionnection → App actor should be cleaned up on disconnection
|
Assignee | |
Comment 1•11 years ago
|
||
|
|
Assignee | |
Comment 2•11 years ago
|
||
Attachment #8388647 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•11 years ago
|
Assignee: nobody → poirot.alex
|
|
Assignee | |
Comment 3•11 years ago
|
||
I even have a test! That doesn't really cover the important part,
but it allows to at least test multiple connection to the same iframe.
I imagine that would cover bug 962541.
If you have any idea on how to somehow ensure that Console actor's disconnect method
is called, I'm all up for it!
Attachment #8389073 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•11 years ago
|
Attachment #8396605 -
Flags: review?(jryans)
Comment on attachment 8396605 [details] [diff] [review]
patch, with test
Review of attachment 8396605 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good overall! Not sure I helped with the test, but maybe there is a way to check for disconnect...
::: toolkit/devtools/server/child.js
@@ +45,5 @@
> + let onDisconnect = DevToolsUtils.makeInfallible(function (msg) {
> + removeMessageListener("debug:disconnect", onDisconnect);
> +
> + // Call DebuggerServerConnection.onClosed to destroy all child actors
> + conn.onClosed();
Use |conn.close()|, which eventually calls onClosed via the transport.
Probably good to null |conn| after this?
::: toolkit/devtools/server/main.js
@@ +56,5 @@
> throw e;
> }
> }
>
> +let events = require("sdk/event/core");
Do you need |this.events = events;| on b2g?
@@ +602,5 @@
> + mm.sendAsyncMessage("debug:disconnect");
> + }
> + Services.obs.removeObserver(onMessageManagerDisconnect, "message-manager-disconnect");
> + };
> + events.once(aConnection, "closed", onConnectionClosed);
Seems like you could just inline the callback, instead of saving to a variable first, but either way really.
::: toolkit/devtools/server/tests/mochitest/test_connectToChild.html
@@ +1,4 @@
> +<SDOCTYPv HTM.>
> +<html>
> +<!--
> +Bug 895360 - [app manager] Device meta data actor
Update this.
@@ +51,5 @@
> + ok(actor.consoleActor, "Got a webconsole actor for the first connection");
> + let firstConsoleActor = actor.consoleActor;
> + client.close(function () {
> + //TODO: ensure that tab actors are destroyed.
> + // i.e. their disconnect method is called
Hmm, for the test can you wrap the DebuggerServer's cleanup / disconnect path before it runs?
> let _cleanup = DebuggerServer.cleanup;
> DebuggerServer.cleanup = function() {
> this.reallyCleanNow = true;
> _cleanup();
> };
Not sure if it will affect the server used by the iframe though...
I guess even if you do that you'd need to get a message up to the parent to say it actually happened.
Attachment #8396605 -
Flags: review?(jryans) → review+
|
|
Assignee | |
Comment 5•11 years ago
|
||
Ok, here is a more decent test for tab actor cleanup.
What do you think, is it hacky enough without being too much?!
Attachment #8396605 -
Attachment is obsolete: true
Comment on attachment 8397136 [details] [diff] [review]
patch, with more concrete test
Review of attachment 8397136 [details] [diff] [review]:
-----------------------------------------------------------------
Seems like a good way to approach the test, given the tools we have!
::: toolkit/devtools/server/main.js
@@ +56,5 @@
> throw e;
> }
> }
>
> +let events = require("sdk/event/core");
Again, be sure to test this on b2g... (Maybe you did already!)
::: toolkit/devtools/server/tests/mochitest/test_connectToChild.html
@@ +64,5 @@
> + };
> + DebuggerServer.addTabActor(TestActor, "testActor");
> + }, false);
> +
> + // Instanciate a miniman server
Nit: miniman -> minimal
Attachment #8397136 -
Flags: review+
|
||
Comment 7•11 years ago
|
||
> > + // Instanciate a miniman server
>
> Nit: miniman -> minimal
Also: Instanciate -> Instantiate!
|
|
Assignee | |
Comment 8•11 years ago
|
||
Attachment #8397136 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 9•11 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #6)
> Comment on attachment 8397136 [details] [diff] [review]
> patch, with more concrete test
>
> Review of attachment 8397136 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Seems like a good way to approach the test, given the tools we have!
>
> ::: toolkit/devtools/server/main.js
> @@ +56,5 @@
> > throw e;
> > }
> > }
> >
> > +let events = require("sdk/event/core");
>
> Again, be sure to test this on b2g... (Maybe you did already!)
>
I forgot to comment about that... We have to bind symbols to `this` on b2g only if the symbols are used by an actor. Here events is only meant to be used in main.js. (Some actors are using events, but define it explicitely in their header)
|
|
Assignee | |
Updated•11 years ago
|
Keywords: checkin-needed
|
||
Comment 10•11 years ago
|
||
|
||
Comment 11•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
|
|
||
Updated•5 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•