1899330 - Devices not showing up when connecting them loading about:debugging

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Steps to reproduce

1. Go to about:debugging
2. Plug your phone with a USB cable
3. Hit refresh devices for good measure

Expected results

The phone is displayed in the sidebar

Actual results

The phone is not displayed in the sidebar. It does once I refresh the about:debugging page though
The "Refresh devices" button should at least do the same thing we're doing when about:debugging loads

---

Comment 1

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

From what I can see, when reproducing the STR:

Plugin the device, there's a call to retrieve the name of the device https://searchfox.org/mozilla-central/rev/f60bb10a5fe6936f9e9f9e8a90d52c18a0ffd818/devtools/client/shared/remote-debugging/adb/adb-device.js#15,20-21,23

class AdbDevice {
...
  async initialize() {
    const model = await shell(this.id, "getprop ro.product.model");
...
  }

which fails in https://searchfox.org/mozilla-central/rev/f60bb10a5fe6936f9e9f9e8a90d52c18a0ffd818/devtools/client/shared/remote-debugging/adb/commands/shell.js#45-49

case "wait-transport":
  if (!client.checkResponse(data, OKAY)) {
    shutdown();
    return;
  }

now I'm not sure why the response is not okay here

Comment 2

[Julian Descottes [:jdescottes]]

Nicolas, I don't have a phone I can use for debugging at the moment, maybe we can pair on this next week?

Comment 3

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

sure. Let's keep the needinfo so we don't forget

Comment 4

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Turning on the logs with devtools.debugger.log, I'm seeing the following when plugging the phone after going to about:debugging

DBG-SERVER: trackDevices ondata
DBG-SERVER: length=27
DBG-SERVER: 001725131FDF6007DN  offline
DBG-SERVER: Len buffer: 27
DBG-SERVER: trackDevices ondata
DBG-SERVER: length=31
DBG-SERVER: 001b25131FDF6007DN  authorizing
DBG-SERVER: Len buffer: 31
DBG-SERVER: shell getprop ro.product.model on 25131FDF6007DN
DBG-SERVER: shell onopen
DBG-SERVER: runFSM start
DBG-SERVER: runFSM send-transport
DBG-SERVER: Created request: 001Dhost:transport:25131FDF6007DN
DBG-SERVER: len=33 30303144686f73743a7472616e73706f72743a32 001Dhost:transport:2
DBG-SERVER: shell ondata
DBG-SERVER: runFSM wait-transport
DBG-SERVER: Response: FAIL
DBG-SERVER: view[0] = 1279869254
DBG-SERVER: shell shutdown
DBG-SERVER: shell onclose
JavaScript error: , line 0: uncaught exception: BAD_RESPONSE
DBG-SERVER: trackDevices ondata
DBG-SERVER: length=53
DBG-SERVER: 001725131FDF6007DN  offline
001625131FDF6007DN      device
DBG-SERVER: Len buffer: 53

Comment 5

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

In https://searchfox.org/mozilla-release/rev/8d957d4113540621c868658e818dc2691f9e5e39/devtools/client/shared/remote-debugging/adb/commands/track-devices.js#104

// One line per device, each line being $DEVICE\t(offline|device)

I'm seeing an authorizing status when connecting my phone. In https://searchfox.org/mozilla-release/rev/8d957d4113540621c868658e818dc2691f9e5e39/devtools/client/shared/remote-debugging/adb/commands/track-devices.js#150,156-160

const isNewOnline = !!(newStatus && newStatus !== ADB_STATUS_OFFLINE);
...
if (isNewOnline) {
  this.emit("device-connected", deviceId);
} else {
  this.emit("device-disconnected", deviceId);
}

As soon as you get another status than offline, we'll consider the device as connected, but if this is authorizing, the query to retrieve the model info will fail, and we'll close the socket.

I found https://android.googlesource.com/platform/packages/modules/adb/+/HEAD/adb.cpp#120 , which does look like the list of possible states, but I can't tell what all the states mean, and when we can consider that a device is connected

std::string to_string(ConnectionState state) {
    switch (state) {
        case kCsOffline:
            return "offline";
        case kCsBootloader:
            return "bootloader";
        case kCsDevice:
            return "device";
        case kCsHost:
            return "host";
        case kCsRecovery:
            return "recovery";
        case kCsRescue:
            return "rescue";
        case kCsNoPerm:
            return UsbNoPermissionsShortHelpText();
        case kCsSideload:
            return "sideload";
        case kCsUnauthorized:
            return "unauthorized";
        case kCsAuthorizing:
            return "authorizing";
        case kCsConnecting:
            return "connecting";
        case kCsDetached:
            return "detached";
        default:
            return "unknown";
    }
}

Comment 6

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)

I found https://android.googlesource.com/platform/packages/modules/adb/+/HEAD/adb.cpp#120 , which does look like the list of possible states, but I can't tell what all the states mean, and when we can consider that a device is connected

std::string to_string(ConnectionState state) {
    switch (state) {
        case kCsOffline:
            return "offline";
        case kCsBootloader:
            return "bootloader";
        case kCsDevice:
            return "device";
        case kCsHost:
            return "host";
        case kCsRecovery:
            return "recovery";
        case kCsRescue:
            return "rescue";
        case kCsNoPerm:
            return UsbNoPermissionsShortHelpText();
        case kCsSideload:
            return "sideload";
        case kCsUnauthorized:
            return "unauthorized";
        case kCsAuthorizing:
            return "authorizing";
        case kCsConnecting:
            return "connecting";
        case kCsDetached:
            return "detached";
        default:
            return "unknown";
    }
}

Julian, you have more experience than I do working with adb, do you recall having to handle those different states?

For what it's worth, with the following diff, my phone is detected when I plug it in after about:debugging is loaded, and unplugging/plugging back does seems to be handled fine in about:debugging:

diff --git a/devtools/client/shared/remote-debugging/adb/commands/track-devices.js b/devtools/client/shared/remote-debugging/adb/commands/track-devices.js
--- a/devtools/client/shared/remote-debugging/adb/commands/track-devices.js
+++ b/devtools/client/shared/remote-debugging/adb/commands/track-devices.js
@@ -18,6 +18,7 @@ const {
 const client = require("resource://devtools/client/shared/remote-debugging/adb/adb-client.js");
 
 const ADB_STATUS_OFFLINE = "offline";
+const ADB_STATUS_DEVICE = "device";
 const OKAY = 0x59414b4f;
 
 // Start tracking devices connecting and disconnecting from the host.
@@ -144,10 +145,8 @@ class TrackDevicesCommand extends EventE
   }
 
   _fireConnectionEventIfNeeded(deviceId, currentStatus, newStatus) {
-    const isCurrentOnline = !!(
-      currentStatus && currentStatus !== ADB_STATUS_OFFLINE
-    );
-    const isNewOnline = !!(newStatus && newStatus !== ADB_STATUS_OFFLINE);
+    const isCurrentOnline = currentStatus === ADB_STATUS_DEVICE;
+    const isNewOnline = newStatus === ADB_STATUS_DEVICE;
 
     if (isCurrentOnline === isNewOnline) {
       return;

Comment 7

[Julian Descottes [:jdescottes]]

Honestly most of this code was lifted from WebIDE, we only did cleanups when working on about:debugging.

I found some doc about the states at https://android.googlesource.com/platform/packages/modules/adb/+/HEAD/adb.h#108

    kCsConnecting = 0,  // Haven't received a response from the device yet.
    kCsAuthorizing,     // Authorizing with keys from ADB_VENDOR_KEYS.
    kCsUnauthorized,    // ADB_VENDOR_KEYS exhausted, fell back to user prompt.
    kCsNoPerm,          // Insufficient permissions to communicate with the device.
    kCsDetached,        // USB device that's detached from the adb server.
    kCsOffline,
    // After CNXN packet, the ConnectionState describes not a state but the type of service
    // on the other end of the transport.
    kCsBootloader,  // Device running fastboot OS (fastboot) or userspace fastboot (fastbootd).
    kCsDevice,      // Device running Android OS (adbd).
    kCsHost,        // What a device sees from its end of a Transport (adb host).
    kCsRecovery,    // Device with bootloader loaded but no ROM OS loaded (adbd).
    kCsSideload,    // Device running Android OS Sideload mode (minadbd sideload mode).
    kCsRescue,      // Device running Android OS Rescue mode (minadbd rescue mode).

Based on this, I think all the states corresponding to a device which is ready for communication are kCsBootloader, kCsDevice, kCsRecovery, kCsSideload and kCsRescue. I imagine that "device" is the main one here, but it would be nice to just include the others. Other than this I think your proposal makes sense, it's hard to know why this would have regressed though.

Edit: ah well there is even a ConnectionStateIsOnline https://android.googlesource.com/platform/packages/modules/adb/+/HEAD/adb.h#127 which matches what I listed above + "host" (but I don't think we care about this one)

Comment 8

[Julian Descottes [:jdescottes]]

I tried the patch and it doesn't seem to work for me. My phone rarely gets to the "device" state, and seems to switch mostly between authorizing and offline. If I mess with the USB connection settings (changing from "charging only" to "file transfer" for instance) it will sometimes reach "device", but that seems very random.

Looking a bit, it seems that my issue is that I get multiline track-devices packets, but our unpackPacket only reads one line. I will share a patch combining both your suggestion and this.

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1899330 - [devtools] Improve ADB track-devices to check correct online states

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1899330 - [devtools] Fix adb unpackPacket helper to handle multiple lines

Depends on D221739

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e6f55b09bea
[devtools] Improve ADB track-devices to check correct online states r=nchevobbe,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/4a074016c96b
[devtools] Fix adb unpackPacket helper to handle multiple lines r=nchevobbe,devtools-reviewers

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4e6f55b09bea
https://hg.mozilla.org/mozilla-central/rev/4a074016c96b