Closed
Bug 1049835
Opened 10 years ago
Closed 10 years ago
Monitor breaks disconnecting
Categories
(DevTools Graveyard :: WebIDE, defect)
DevTools Graveyard
WebIDE
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 34
People
(Reporter: jryans, Assigned: janx)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Good catch! Apparently an active link to the device's monitor actors won't allow `AppManager.connection.status` to go from "disconnecting" to "disconnected".
Assignee: nobody → janx
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Blocks: enable-webide
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
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...
Attachment #8473035 -
Flags: review?(jryans)
Comment 4•10 years ago
|
||
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
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Attachment #8473035 -
Attachment is obsolete: true
Attachment #8473035 -
Flags: review?(jryans)
Assignee | ||
Updated•10 years ago
|
Attachment #8473045 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 7•10 years ago
|
||
Comment 8•10 years ago
|
||
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!
Attachment #8473045 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 9•10 years ago
|
||
(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 :)
Reporter | ||
Comment 10•10 years ago
|
||
Someone should rework this |detach| code... It should not be dependent on calling the passed function to continue. Filed bug 1053950.
Assignee | ||
Comment 11•10 years ago
|
||
Wow that's a lot of intermittents in mochitest devtools. Something's wrong, retriggering.
Assignee | ||
Comment 12•10 years ago
|
||
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
Attachment #8473045 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
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.
Attachment #8474530 -
Flags: review?(poirot.alex)
Comment 14•10 years ago
|
||
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.
Attachment #8474530 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
Comment 16•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Comment 17•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
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).
Flags: needinfo?(janx)
Comment 20•10 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•5 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•