App actor should be cleaned up on disconnection

RESOLVED FIXED in Firefox 31

Status

DevTools
WebIDE
RESOLVED FIXED
5 years ago
a month ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
Firefox 31
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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

5 years ago
Summary: App actor should be cleaned up on disctionnection → App actor should be cleaned up on disconnection
(Assignee)

Comment 2

4 years ago
Created attachment 8389073 [details] [diff] [review]
disconnect
Attachment #8388647 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Assignee: nobody → poirot.alex
(Assignee)

Comment 3

4 years ago
Created attachment 8396605 [details] [diff] [review]
patch, with test

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

4 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

4 years ago
Created attachment 8397136 [details] [diff] [review]
patch, with more concrete test

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+
> > +  // Instanciate a miniman server
> 
> Nit: miniman -> minimal

Also: Instanciate -> Instantiate!
(Assignee)

Comment 9

4 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

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/0b5eeb29646b
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0b5eeb29646b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.