1049835 - Monitor breaks disconnecting

Status: VERIFIED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

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

STR:

1. Connect to a USB runtime
2. Open the Monitor
3. Runtime menu -> Disconnect

ER:

We should disconnect immediately (which we do if you do not open the Monitor).

AR:

Disconnect shows the busy indicator and eventually times out.

Comment 1

[Jan Keromnes [:janx]]

Good catch! Apparently an active link to the device's monitor actors won't allow `AppManager.connection.status` to go from "disconnecting" to "disconnected".

Comment 2

[Jan Keromnes [:janx]]

Attachment: MonitorClient.detach should accept a callback.

Comment 3

[Jan Keromnes [:janx]]

Comment on attachment 8473035 [details] [diff] [review]
MonitorClient.detach should accept a callback.

In actor clients, `detach` is pointless, but we secretly expect it to call back:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#423

This made me sad...

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8473035 [details] [diff] [review]
MonitorClient.detach should accept a callback.

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

::: toolkit/devtools/client/dbg-client.jsm
@@ +1012,4 @@
>                        "have an `emit` function.");
>      }
>      if (typeof(client.detach) != "function") {
> +      // Note: We also need `detach` to accept a callback.

I don't remember strict mode rules, but I think we can at least assert detach expect an argument: client.detach.length == 1

Comment 5

[Jan Keromnes [:janx]]

Note: In the STR, step 2 is not enough to reproduce the bug.

On the device, you need to have either the certified apps debugging pref on, or a non-certified app open (e.g. Marketplace). That will cause a monitor actor client to be registered, and will reproduce the bug if the attached fix was not applied.

Comment 6

[Jan Keromnes [:janx]]

Attachment: Don't require actor clients to have a detach method.

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Comment on attachment 8473035 [details] [diff] [review]
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +1012,4 @@
> >                        "have an `emit` function.");
> >      }
> >      if (typeof(client.detach) != "function") {
> > +      // Note: We also need `detach` to accept a callback.
> 
> I don't remember strict mode rules, but I think we can at least assert
> detach expect an argument: client.detach.length == 1

OK fine, let's end the detach madness completely then.

Comment 7

[Jan Keromnes [:janx]]

https://tbpl.mozilla.org/?tree=Try&rev=1da3dd8467c3

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8473045 [details] [diff] [review]
Don't require actor clients to have a detach method.

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

::: toolkit/devtools/client/dbg-client.jsm
@@ +424,5 @@
> +        if (client.detach.length > 0) {
> +          client.detach(detachClients);
> +          return;
> +        }
> +        client.detach();

So function.length doesn't print any message in strict mode?

I imagine they all expect a callback argument,
so instead of adding a preemptive feature,
I would just throw an error!

Comment 9

[Jan Keromnes [:janx]]

(In reply to Alexandre Poirot [:ochameau] from comment #8)
> So function.length doesn't print any message in strict mode?

So it seems. :)

> I imagine they all expect a callback argument,
> so instead of adding a preemptive feature,
> I would just throw an error!

Then I guess this line would throw! http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/test_monitor_actor.js#30 and I already know who I can blame when the test burns :)

Comment 10

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

Someone should rework this |detach| code...  It should not be dependent on calling the passed function to continue.  Filed bug 1053950.

Comment 11

[Jan Keromnes [:janx]]

Wow that's a lot of intermittents in mochitest devtools. Something's wrong, retriggering.

Comment 12

[Jan Keromnes [:janx]]

Attachment: Don't require actor clients to have a detach method.

Giving up on Function.length, not exactly sure why everything exploded but I don't trust it anymore.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0306d9aef218

Comment 13

[Jan Keromnes [:janx]]

Comment on attachment 8474530 [details] [diff] [review]
Don't require actor clients to have a detach method.

Alex, I don't think it's necessary to throw an error, but I'll give you another chance to complain about my patch.

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8474530 [details] [diff] [review]
Don't require actor clients to have a detach method.

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

Still ok, but it makes bug 1053950 slightly more important.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/023bfd07bd0b

Comment 17

[Petruta Horea [:phorea], Desktop QA]

I couldn't reproduce the initial issue using a 34 Nightly build (under Win 7 and Mac OS X 10.9.5.) and a Flame device with 1.4 version after following the steps from comment 0 and comment 5. Are there extra steps to do?

Comment 18

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

Jan, can you help Petruta?

Comment 19

[Jan Keromnes [:janx]]

Sure!

Petruta, you should probably test with a device that's at least on 2.1.

This bug is for a client that controls a devtools actor on the device, implemented in bug 1025804, which I think didn't make Firefox OS 2.0 (I say that because I can't find the commit in the mozilla-b2g/b2g32_v2_0 branch, however it's in mozilla-b2g/b2g34_v2_1).

Comment 20

[Petruta Horea [:phorea], Desktop QA]

Thanks Jan! I finally could look into this, sorry for delay.

I reproduced the issue after flashing the Flame device with Firefox OS 2.1.

The issue no longer occurs with Firefox 34 beta 9, win 7 64-bit.