1181100 - Dynamic actors fail when used on multiple tabs and closing the first one

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

See bug 1168872 comment 10.
This is surprising we didn't catched that before with tests or when using dynamic actors registered with Director actor...
But the following code is most likely wrong:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/child.js#23

We are storing a reference to the first frame script's message manager,
which is specific to the <xul:browser> we connected first.
When we end up closing the related tab and destroying the <browser>,
its message manager is no longer usable.
That's not an issue for the message manager used for ChildDebuggerTransport, as we destroy the transport as soon as we close the tab. But we keep using DebuggerServer.parentMessageManager, always.

Instead, we should use parentprocessmessagemanager/childprocessmessagemanager xpcoms.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3960e06548a

I first tried to use ppmm/cpmm (message manager that always work on both sides).
But there is the prefix argument being passed in setupParent...
So it looks like these message should still be tied to just one child/ChildDebuggerTransport.
Given that, DebuggerServerConnection looks like a good spot to put setupInParent/parentMessageManager/...
ChildDebuggerTransport would be even better, but that's not easily accessible from an actor.

This patch modifies storage actor accordingly,
but we would have to sync with Honza to update addons that are already using DebuggerServer.parentMessageManager.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Here is another scenario, which might be related to this bug:
1) Install TabActor example
https://github.com/firebug/devtools-extension-examples/tree/master/TabActor
(this example just registers a new tab actor)

2) Open browser and the Toolbox on current tab.

3) Open the Browser Console. You should see logs like:
- New toolbox ready, let's register a tab actor for it <url>.
- My tab actor is now registered
- My actor is now attached
- Response from the actor: Hello from a tab actor!

4) Open a new browser tab and the Toolbox on it
- New toolbox ready, let's register a tab actor for it. <url>
- My tab actor is already registered, attach to it

"My actor is now attached" message never appears -> BUG

This is because the "attach" is broken.

I am also seeing the following exception:
Console Service ERROR [JavaScript Error: "A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Wed Jul 08 2015 11:09:52 GMT+0200
Full Message: Protocol error (noSuchActor): No such actor for ID: server1.conn0.child4/mytabactor24"] [JavaScript Error: "A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

---

The patch attached to this bug doesn't solve the problem.

Alex: should I report this as separate bug?

Honza

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> Here is another scenario, which might be related to this bug:
> 1) Install TabActor example
> https://github.com/firebug/devtools-extension-examples/tree/master/TabActor
> (this example just registers a new tab actor)

That would be really great if we can reproduce that in a test.
I'm surprised this failure doesn't happen with existing tests that should do similar things?!

Also, we should really put helper method on client/target/something to do all the paperwork around actoractor. Addons shouldn't have to copy paste this extremelly complex piece of code!

Comment 4

[Alexandre Poirot [:ochameau]]

Otherwise, does this patch fixes bug 1168872 failures?
If yes, I would imagine the issue you have with the addon is about something else.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This patch fixes all the parent / child communication problems I had with the storage inspector.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Otherwise, does this patch fixes bug 1168872 failures?
> If yes, I would imagine the issue you have with the addon is about something
> else.
Yes, the test now passes on my machine. I am yet waiting for try-build at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e1e6464f1bf

Thanks!
Honza

Comment 7

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

[Tracking Requested - why for this release]:

If possible, we should uplift this to 41 (where storage tool first supported e10s, bug 1049888).

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> (In reply to Alexandre Poirot [:ochameau] from comment #4)
> > Otherwise, does this patch fixes bug 1168872 failures?
> > If yes, I would imagine the issue you have with the addon is about something
> > else.
> Yes, the test now passes on my machine. I am yet waiting for try-build at:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e1e6464f1bf

So, it looks like there is still a problem on Linux x64 Opt

Honza

Comment 9

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #8)
>
> So, it looks like there is still a problem on Linux x64 Opt
> 

Looks like an intermittent. Is it really related to this patch?
It may be just something else specific to bug 1168872.

Here is a try with just this patch:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2376329562c

Also, what do you think about this change?
It will break the old addons, while ensuring the new ones works fine regarding e10s...
The compatibility breakage is that instead of doing DebuggerServer.setupInParent, you will have to do conn.setupInParent.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> (In reply to Jan Honza Odvarko [:Honza] from comment #8)
> >
> > So, it looks like there is still a problem on Linux x64 Opt
> > 
> 
> Looks like an intermittent. Is it really related to this patch?
> It may be just something else specific to bug 1168872.
Yeah, I need to do more analyses.

> 
> Here is a try with just this patch:
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2376329562c
> 
> Also, what do you think about this change?
> It will break the old addons, while ensuring the new ones works fine
> regarding e10s...
> The compatibility breakage is that instead of doing
> DebuggerServer.setupInParent, you will have to do conn.setupInParent.

I am not too worried in this case. First, I don't know about any extension that would use the API (and there is none on AMO) and second it's simple to feature-detect the API and support both.

Honza

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> Also, what do you think about this change?
> It will break the old addons, while ensuring the new ones works fine
> regarding e10s...
> The compatibility breakage is that instead of doing
> DebuggerServer.setupInParent, you will have to do conn.setupInParent.


I am using DebuggerServer.setupInParent and it works fine with this patch.

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I have push updated patch to try
v6: https://bugzilla.mozilla.org/attachment.cgi?id=8634027

And the server side logging test passes on all platforms now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52dfce7cbb68

There is one (I guess) unrelated leaking test:
browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js


Honza

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

New patch, with a test!!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eeda53edac3

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b3161fc4563

I had to bump memory-footprint test as instanciating the child process message manager increase the memory usage by 100-200k.
Hopefully, that wouldn't bump it on b2g as I'm expected it to be already used/loaded.

Otherwise, I also had to ensure calling DebuggerServer.destroy in xpcshell tests to ensure removing the message listener.
If we don't do so, register-actor-actor test doesn't shutdown and timeout at the end as if the process doesn't want to close by itself.

Comment 15

[Alexandre Poirot [:ochameau]]

Most failure are fixed except a crash that I don't reproduce locally (yet):
browser/devtools/markupview/test/browser_markupview_update-on-navigtion.js | application crashed [@ mozilla::::RunWatchdog]

Comment 16

[Alexandre Poirot [:ochameau]]

Yet another try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ddc127b9fa0
The ppmm listener seems to be hanging the process during shutdown.

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

Here is a good one!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17a90edb5dae

Finally, I stopped using ppmm/cpmm as it introduces too many issues in shutdown (again!).
Keeping the list of active mm works fine...

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8638060 [details] [diff] [review]
patch v4

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

Looks very clean to me... awesome job.

r+ assuming that try is green.

Comment 19

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

Comment on attachment 8638060 [details] [diff] [review]
patch v4

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

::: toolkit/devtools/server/docs/actor-e10s-handling.md
@@ +95,4 @@
>  
>  In the parent process:
>  
>  * The DebuggerServer receives the `DebuggerServer.setupInParent` request,

This is now on the connection, not the server.

@@ +98,5 @@
>  * The DebuggerServer receives the `DebuggerServer.setupInParent` request,
>  * tries to load the required module,
>  * tries to call the `module[setupParent]` function with the frame message manager and the prefix as parameters `{ mm, prefix }`,
>  * the `setupParent` function then uses the mm to subscribe the messagemanager events,
> +* the `setupParent` function also uses the DebuggerServer object to subscribe *once* to the `"disconnected-from-child:PREFIX"` event to unsubscribe from messagemanager events.

Looks line line 86 should be changed to replace CHILDID with PREFIX.

::: toolkit/devtools/server/main.js
@@ +914,2 @@
>     */
> +  _childMessageManagers : new Set(),

Nit: no space before :

(Use ESLint) :)

@@ +1313,5 @@
>  
>    _transport: null,
>    get transport() { return this._transport },
>  
> +  /*

Nit: /** to match the usual comment style

@@ +1315,5 @@
>    get transport() { return this._transport },
>  
> +  /*
> +   * Message manager used to communicate with the parent process,
> +   * set by child.js. Is only defined for connections instanciated

Nit: instanciated -> instantiated

::: toolkit/devtools/server/tests/mochitest/test_setupInParentChild.html
@@ +54,5 @@
> +  let conn = transport._serverConnection;
> +  let client = new DebuggerClient(transport);
> +
> +  // Wait for a response from setupInChild
> +  const ppmm = Cc["@mozilla.org/parentprocessmessagemanager;1"].

Nit: I believe most DevTools code places the dot on the next line.  I think ESLint would warn for this with our current rules.

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Comment 21

[Alexandre Poirot [:ochameau]]

Honza, This patch is now ready to land.
Should we coordinate while landing this patch?
I imagine you need to tweak some stuff in your addons code in order to keept setupInParent from working. Or should I just land this?

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked. Adding a bit more of the impact that Alex explained to me. Thanks Alex!

This bug introduces issues on e10s, so it is limited to Nightly or people manually enabling e10s and having such devtools addon.
Devtools addons break when opening them on a first tab, closing this tab, and reopening devtools (with an addon) on another tab, all that with e10s turned on.

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #21)
> Honza, This patch is now ready to land.
> Should we coordinate while landing this patch?
> I imagine you need to tweak some stuff in your addons code in order to keept
> setupInParent from working. Or should I just land this?

If I understand correctly the change is that "setupInParent" and "parentMessageManager" API are not available in DebuggerServer anymore, but in DebuggerConnection object, correct?

If yes, you can land it, I'll take care about the addons.

Honza

Comment 24

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> If I understand correctly the change is that "setupInParent" and
> "parentMessageManager" API are not available in DebuggerServer anymore, but
> in DebuggerConnection object, correct?

Yes, exactly.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f6f67168e242

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Alex, could you please verify the fix? If it looks good, I will request Honza to request uplift to Aurora if the patch is deemed stable and safe. Thanks!

Comment 28

[Alexandre Poirot [:ochameau]]

Here is a try run on aurora:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd03c19a611a

Comment 29

[Alexandre Poirot [:ochameau]]

Comment on attachment 8639207 [details] [diff] [review]
patch v5

Approval Request Comment
[Feature/regressing bug #]:
  Storage inspector with e10s was fixed during bug 1049888, but had this bug since then. Some addons using this same e10s codepath can be also broken. That isn't a regression but more a always broken behavior.
[User impact if declined]:
  Storage inspector and may be some addons can be broken if you open devtools on a first tab, then close this tab and open then in another one.
[Describe test coverage new/current, TreeHerder]:
  Is covered by a dedicated test.
  Landed on master for quite a bit now.
  Honza started using this codepath from another place in bug 1168872.
  Has try run on aurora:
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd03c19a611a
[Risks and why]:
  Just to fix that bug also on FF41 where we enabled storage inspector with e10s. Codepaths specific to devtools.
[String/UUID change made/needed]:
  no

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8639207 [details] [diff] [review]
patch v5

Alexandre, 41 is in the beta channel. Aurora is 42 now.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8639207 [details] [diff] [review]
patch v5

The patch has been on Nightly for over 2 weeks, seems safe to uplift to Beta. The patch also adds automated tests which is always a good things.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d4bf2f467ce9