All users were logged out of Bugzilla on October 13th, 2018

[mozdevice] Port forward will fail if the socket is already bound.

RESOLVED FIXED in mozilla36

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: zcampbell, Assigned: davehunt)

Tracking

unspecified
mozilla36
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
While testing this with combinations of b2g open, I noticed that this can fail silently if the port is already bound by desktop b2g.
STR: 
1. Start a desktopb2g instance with marionette port 2828 open
2. Also connect a device with marionette port 2828 open
3. Use mozdevice 'forward' method to port forward tcp:2828 tcp:2828

mozdevice forward returns 1 regardless of the success of the command.

Repeating this STR manually creates a "error: cannot bind to socket" 

http://hg.mozilla.org/mozilla-central/file/62990ec7ad78/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py#l158
(Reporter)

Updated

4 years ago
Blocks: 1038868
(In reply to Zac C (:zac) from comment #0)
> While testing this with combinations of b2g open, I noticed that this can
> fail silently if the port is already bound by desktop b2g.
> STR: 
> 1. Start a desktopb2g instance with marionette port 2828 open
> 2. Also connect a device with marionette port 2828 open
> 3. Use mozdevice 'forward' method to port forward tcp:2828 tcp:2828
> 
> mozdevice forward returns 1 regardless of the success of the command.

This is bad information provided by me. The forward method does in fact return 0 if the command is successful. See http://hg.mozilla.org/mozilla-central/file/62990ec7ad78/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py#l158 for the method we're talking about.

So there's two ways we can resolve this. One would be to modify the place we're calling forward to check that it returns 0, which would be here: http://hg.mozilla.org/mozilla-central/file/62990ec7ad78/testing/mozbase/mozrunner/mozrunner/devices/base.py#l188

The other option would be for the forward method itself to throw an exception if it's unsuccessful. This would be my personal preference but would break anything that currently relies on failed forwards being silently ignored.

Will: How do you think we should proceed here?
Flags: needinfo?(wlachance)
+1 to throwing an exception. I don't like using return codes for checking success because it is all too easy for consumers to ignore.
Yup, very much in agreement with you and :ahal, throwing an exception is the way to go. If that breaks stuff, we should fix it.
Flags: needinfo?(wlachance)
Created attachment 8520049 [details] [diff] [review]
Raise exception if we fail to forward socket connections. v1.0
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Attachment #8520049 - Flags: review?(wlachance)
Comment on attachment 8520049 [details] [diff] [review]
Raise exception if we fail to forward socket connections. v1.0

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

wfm
Attachment #8520049 - Flags: review?(wlachance) → review+
https://hg.mozilla.org/mozilla-central/rev/a986055f7db2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.