1041752 - Actor `destroy` not called with e10s

Status: RESOLVED INVALID

(DevTools :: General, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

While some tests will still pass if `destroy` is not called for some actors (shader, audio), it still causes issues when things are required to be cleaned up for tests. The `destroy` function is not called.

AFAICT, the issue happens after DebuggerServer.onClosed is called and the server emits a "closed" event. From this point, the e10s-ified version attempts to handle the closing in `connectToChild`, throwing errors and warnings in several places.

Only removing the message manager for e10s once seems to fix a lot of the breaking tests I'm having, but would like to get more eyes, because this area of code I'm very unfamiliar with. Or if there are better suggestions.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1041752-actor-destroy-e10s.patch

Comment 2

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

Can you explain the problem in more detail?  Do you have a TBPL run with logs of the issue?

In your patch, I see you're protecting against removing the observer more than once.  I guess you saw an error related to doing it a second time...?

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Pushing to try, should run in e10s mode: https://tbpl.mozilla.org/?tree=Try&rev=6c1db253c3bd

In the mean time, here's a dump of at the end of all tests: http://pastebin.com/t7kDPwm4
And here's just running browser_se_bfcache.js test, which is passing successfully, but has a lot of errors and nightmares at the end: http://pastebin.com/FbXi55jk

Not saying that this will fix all the issues I'm having, but will atleast reduce the surface space, because getting immediate errors thrown when unbinding the same handler twice from an observer, although I don't quite understand everything that's happening in protocol and transport.

Comment 4

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

I don't feel like I have enough to context here to make a decisive recommendation... but from the errors, it seems like the message manager is already disconnected (so the |onMessageManagerDisconnect| block of |connectToChild|) already ran, and then later DS emits the "closed" event.

It looks like we currently handle the reverse ordering (DS emits "closed", then message manager disconnects) by having the DS "closed" handler remove the observer.

Perhaps |onMessageManagerDisconnect| should remove the "closed" event listener, so we don't enter the "closed" listener after the message manager is gone?  At the same time, the "closed" listener, while doing mostly the same work, is the only one to send "debug:disconnect", to tell the child process to close its side of the connection[1].  But maybe this work is not needed if the manager has already disconnected?  I am not sure.

[1]: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/child.js#46

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Moving the `debug:disconnect` event also to the other closer seemed to do the trick for my specific issue -- spent some time trying to get the protocol tests running in e10s, which will need to be done, but not for this ticket. Handling this fix in bug 1034295 to ensure tests are with it (in shader editor)

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1041752-actor-destroy-e10s.patch

Try push: https://tbpl.mozilla.org/?tree=Try&rev=87b2ba146ac8

Comment 7

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

Comment on attachment 8461649 [details] [diff] [review]
1041752-actor-destroy-e10s.patch

Review of attachment 8461649 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/main.js
@@ +603,5 @@
>            childTransport = null;
>            aConnection.cancelForwarding(prefix);
> +
> +          // ... and notify the child process to clean the tab actors.
> +          mm.sendAsyncMessage("debug:disconnect");

Hmm, I guess I am surprised that this works...

Once you've received "message-manager-disconnect", the manager should be gone by the next turn of the event loop, so sending a new message doesn't seem like it should work.  But, I suppose it does seem to work for you?

Let's wait for the Try results.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

*shrug* it works, but ya definitely wait for try tests

Comment 9

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #8)
> *shrug* it works, but ya definitely wait for try tests

Do the platforms you've run here actually use e10s on try (and thus the message manager)?

I am not up to date of the state of this.

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

No -- e10s is only running on Holly (not sure how to run try tests on there or what it is)

Comment 11

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #10)
> No -- e10s is only running on Holly (not sure how to run try tests on there
> or what it is)

Hmm, I guess that means you'd just push to Holly instead of try, and then it shows up on TBPL for that branch[1].

In any event, it seems like the regular Try push found some issues as well.

[1]: https://tbpl.mozilla.org/?tree=Holly

Comment 12

[Joe Walker [:jwalker]]

(In reply to J. Ryan Stinnett [:jryans] from comment #9)
> (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #8)
> > *shrug* it works, but ya definitely wait for try tests
> 
> Do the platforms you've run here actually use e10s on try (and thus the
> message manager)?
> 
> I am not up to date of the state of this.

My understanding is, holly == try with e10s turned on.
So push at will.