842667 - DeviceManagerADB gives unhelpful error when unable to connect over TCP

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Mark Côté [:mcote]]

Found this while testing the fix for bug 800082.  If you give it the wrong port, instead of a helpful "unable to connect"-style message, you get this:

>>> dm = DeviceManagerADB(host='192.168.1.158', port=12345)
unable to execute 'cp' on device; consider installing busybox from Android Market

busybox is clearly not the problem here.

Comment 1

[Mark Côté [:mcote]]

Hm even more strangely, if you instantiate a DeviceManagerADB object with the correct port, and then one without, you don't always get an error, although the device is indeed not found:

>>> dm = DeviceManagerADB(host='192.168.1.158', port=5555)
>>> dm = DeviceManagerADB(host='192.168.1.158', port=12345)
>>> dm.listFiles('/mnt')
['error: device not found']

It's like there is some saved class or module state or something.

Comment 2

[Mark Côté [:mcote]]

Probably useful: instructions for enabling adb over TCP: http://stackoverflow.com/questions/2604727/how-can-i-connect-to-android-with-adb-over-tcp

Comment 3

[Mihnea Dobrescu-Balaur (:mihneadb)]

How about this? http://pastebin.mozilla.org/2160878 :) Something is strange, indeed. My phone is listening on 5555, not 12345.

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #3)
> How about this? http://pastebin.mozilla.org/2160878 :) Something is strange,
> indeed. My phone is listening on 5555, not 12345.

I did manage to duplicate the behavior after restarting the adb server. Still, strange.

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: fix

Found it

Comment 6

[Mark Côté [:mcote]]

Comment on attachment 715977 [details] [diff] [review]
fix

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

This looks good, although I would drop the bit about it maybe being the wrong port.  Could be that the device is down or adbd isn't running or other things; no point in just choosing one problem. :)

However this doesn't fix the issue I mentioned in comment #1.  Could you look into that too?  It's 100% reproducible for me.  Feel free to push this fix now, though, as it's an improvement.

Comment 7

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: fix

I removed the "wrong port" part.

Also, regarding the issue you mentioned in comment 1, it is because you did not call the _disconnectRemoteADB method which gets called in __del__[1]. So I guess this is not a bug.

[1] https://github.com/mozilla/mozbase/blob/master/mozdevice/mozdevice/devicemanagerADB.py#L82

Comment 8

[Mark Côté [:mcote]]

Cool, although sorry I didn't look at your patch carefully enough--could you move the if ret: raise DMError(...) check below the try/except block?  Better to wrap only the potential source of subprocess.CalledProcessError, which is the _checkCmd() call.  No need to rereview it.

I see what you mean about _disconnectRemoteADB() not being called, but I would still consider it a bug, if only because requiring del to be called first is really unintuitive.  There must be a way to work around this.  But I can file another bug for it and close this one if you don't want to tackle that part.

Comment 9

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Mark Côté ( :mcote ) from comment #8)
> Cool, although sorry I didn't look at your patch carefully enough--could you
> move the if ret: raise DMError(...) check below the try/except block? 
> Better to wrap only the potential source of subprocess.CalledProcessError,
> which is the _checkCmd() call.  No need to rereview it.
> 

Changed and pushed: https://github.com/mozilla/mozbase/commit/8a5f18fd7279294532cdcf8fa50a2b3ee762c247

> I see what you mean about _disconnectRemoteADB() not being called, but I
> would still consider it a bug, if only because requiring del to be called
> first is really unintuitive.  There must be a way to work around this.  But
> I can file another bug for it and close this one if you don't want to tackle
> that part.

Please open a new bug and I'll try to come up with a solution when I find some more time.

Comment 10

[Mark Côté [:mcote]]

Okay, filed as bug 843378.

Comment 11

[Mark Côté [:mcote]]

Comment on attachment 716238 [details] [diff] [review]
fix

Sorry realized I never officially r+ed this.