Closed Bug 1181100 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(firefox40 unaffected, firefox41+ fixed, firefox42+ fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox40 --- unaffected
firefox41 + fixed
firefox42 + fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch patch v1 (obsolete) — Splinter Review
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.
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
(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!
Otherwise, does this patch fixes bug 1168872 failures?
If yes, I would imagine the issue you have with the addon is about something else.
This patch fixes all the parent / child communication problems I had with the storage inspector.
(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
[Tracking Requested - why for this release]:

If possible, we should uplift this to 41 (where storage tool first supported e10s, bug 1049888).
(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
(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.
(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
(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.
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
Attached patch patch v2 (obsolete) — Splinter Review
New patch, with a test!!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eeda53edac3
Attachment #8630446 - Attachment is obsolete: true
Attachment #8636033 - Flags: review?(mratcliffe)
Attachment #8636033 - Flags: review?(jryans)
Assignee: nobody → poirot.alex
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #8636033 - Attachment is obsolete: true
Attachment #8636033 - Flags: review?(mratcliffe)
Attachment #8636033 - Flags: review?(jryans)
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]
Yet another try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ddc127b9fa0
The ppmm listener seems to be hanging the process during shutdown.
Attached patch patch v4 (obsolete) — Splinter Review
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...
Attachment #8636522 - Attachment is obsolete: true
Attachment #8638060 - Flags: review?(mratcliffe)
Attachment #8638060 - Flags: review?(jryans)
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.
Attachment #8638060 - Flags: review?(mratcliffe) → review+
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.
Attachment #8638060 - Flags: review?(jryans) → review+
Attached patch patch v5Splinter Review
Attachment #8638060 - Attachment is obsolete: true
Attachment #8639207 - Flags: review+
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?
Flags: needinfo?(odvarko)
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.
(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
Flags: needinfo?(odvarko) → needinfo?(poirot.alex)
(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.
Flags: needinfo?(poirot.alex)
https://hg.mozilla.org/mozilla-central/rev/f6f67168e242
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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!
Flags: needinfo?(poirot.alex)
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
Flags: needinfo?(poirot.alex)
Attachment #8639207 - Flags: approval-mozilla-aurora?
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]:
Attachment #8639207 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
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.
Attachment #8639207 - 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.