916777 - Use a UNIX domain socket for debugger server connections on Android

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Firefox OS uses a UNIX domain socket for the debugger server after bug 832000. We should do the same for Android, too.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Use a UNIX domain socket for debugger server connections on Android

This should work, but my computer is busy building other patches right now to test.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

I couldn't get a successful Fennec build all day to test. I'll try again on Monday.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Use a UNIX domain socket for debugger server connections on Android v2

Made the pref "live", but I can't get it to work on my Galaxy S II. In logcat I see:

I/Gecko   (16509): WARNING: failed to bind socket: file /Users/past/src/fx-team/netwerk/base/src/nsServerSocket.cpp, line 352
E/GeckoConsole(16509): Remote debugger didn't start: 2147746065

The error seems to be NS_ERROR_NOT_AVAILABLE, which I assume is caused by insufficient permissions in the Fennec process. I tried using /data/local/tmp/debugger-socket as the path, since my non-root adb shell seems to have access there, but no dice.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 808556 [details] [diff] [review]
Use a UNIX domain socket for debugger server connections on Android v2

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

Jim from bug 892114 comment 23 I understand that you didn't manage to verify that UNIX domain sockets work on Android, is that correct?

Mark, do you happen to know if there is a standard path on Android that Fennec can use for creating a UNIX domain socket?

The changes in this patch are intended to make remote protocol connections on Firefox for Android more secure and identical to the way the remote debugger in Firefox OS now works. Having the same path for the socket would also dimplify our remote debugging documentation, but we could certainly live without that if we had to.

Comment 5

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #4)
> Comment on attachment 808556 [details] [diff] [review]
> Use a UNIX domain socket for debugger server connections on Android v2
> 
> Review of attachment 808556 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Jim from bug 892114 comment 23 I understand that you didn't manage to verify
> that UNIX domain sockets work on Android, is that correct?

Yes, that's right. If anyone can tell us how to find a filesystem that can handle Unix domain sockets in an xpcshell test, then we can fix that.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 808556 [details] [diff] [review]
Use a UNIX domain socket for debugger server connections on Android v2

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

It looks great!

Would "unix-domain-path" be better than "unix-domain-socket"? "unix-domain-socket-path"?

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jim Blandy :jimb from comment #6)
> Would "unix-domain-path" be better than "unix-domain-socket"?
> "unix-domain-socket-path"?

Probably, but this is the name used already in Firefox OS and AIUI the documentation, blog posts, etc. are already written and published, so it may be confusing to change it now.

Comment 8

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #7)
> Probably, but this is the name used already in Firefox OS and AIUI the
> documentation, blog posts, etc. are already written and published, so it may
> be confusing to change it now.

Ah, I didn't realize that the time for bikeshedding was long past. Carry on...

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] from comment #4)
> Mark, do you happen to know if there is a standard path on Android that
> Fennec can use for creating a UNIX domain socket?

Ping?

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Panos Astithas [:past] from comment #9)
> (In reply to Panos Astithas [:past] from comment #4)
> > Mark, do you happen to know if there is a standard path on Android that
> > Fennec can use for creating a UNIX domain socket?
> 
> Ping?

Ugh. Sorry for the delay. Also, sorry I don't know. I'll need-info Brad Lassey to look into this. It's a little too low level for me.

We might need to know what bionic supports.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

It looks like LocalServerSocket lets you pass an arbitrary path, so I'd say that there isn't a standard path on Android.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #11)
> It looks like LocalServerSocket lets you pass an arbitrary path, so I'd say
> that there isn't a standard path on Android.

So can't we find a path that will work for us from a permission POV? I would think that paths inside the profile wouldn't work due to the crazy path name, but perhaps we could register a path on the abstract namespace like Chrome seems to be doing? Chrome uses "adb forward tcp:9222 localabstract:chrome_devtools_remote" to make a remote connection. Does that require the app to request extra security permissions?

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Panos Astithas [:past] from comment #12)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #11)
> > It looks like LocalServerSocket lets you pass an arbitrary path, so I'd say
> > that there isn't a standard path on Android.
> 
> So can't we find a path that will work for us from a permission POV? I would
> think that paths inside the profile wouldn't work due to the crazy path
> name, but perhaps we could register a path on the abstract namespace like
> Chrome seems to be doing? Chrome uses "adb forward tcp:9222
> localabstract:chrome_devtools_remote" to make a remote connection. Does that
> require the app to request extra security permissions?

I don't see why paths in the profile wouldn't work. Also, I think we have all the permissions necessary to use an arbitrary namespace. So either option should work.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

The problem with profile paths is that we point users to documentation that instructs them to use adb forward to connect to the device. Since the profile path is different from one instance to another, how could we provide user-friendly instructions?

If an abstract namespace is an option, do you know how we can make Fennec register a name for our use? Or is it just a matter of using a localabstract:firefox_devtools_remote path when opening the socket?

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Panos Astithas [:past] from comment #14)
> The problem with profile paths is that we point users to documentation that
> instructs them to use adb forward to connect to the device. Since the
> profile path is different from one instance to another, how could we provide
> user-friendly instructions?
If user friendly is what you're looking for, something like /data/data/<packagename>/firefox_devtools_remote would seem to be a good option (where <packagename> is org.mozilla.fennec for nightly)
> 
> If an abstract namespace is an option, do you know how we can make Fennec
> register a name for our use? Or is it just a matter of using a
> localabstract:firefox_devtools_remote path when opening the socket?

I'm unaware of a way to register it for exclusive use, so I think you're in the latter case.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 808556 [details] [diff] [review]
Use a UNIX domain socket for debugger server connections on Android v2

I don't know why I didn't do this already, but I can give feedback on this approach without really knowing the answer to the unix socket question.

The changes look OK to me. As long as the path stuff works, the prefs and JS look OK.

Do we want to support the "port" approach at all? It looks like you are removing it from the JS.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

I hope to get back to this patch in the next few weeks. We still need the TCP socket method for Windows desktop at least, since UNIX domain sockets aren't supported there AFAIK.

Comment 18

[Paul Rouget [:paul]]

Can we rename `/data/local/debugger-socket` to `/data/local/firefoxandroid-debugger-socket` ?

For WebIDE, we want to be able to identify what's behind this socket before connecting to it. Then we can have a pretty "Connect to Firefox for Android" button.

Any reason to not land that now?

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #18)
> Can we rename `/data/local/debugger-socket` to
> `/data/local/firefoxandroid-debugger-socket` ?

Or just `/data/local/firefox-debugger-socket`, but either is fine

Comment 20

[Paul Rouget [:paul]]

The reason why we need a different file path than Firefox OS is because we want to be able to identify what kind of target is behind the socket.

We want to be able to list connected devices. Via ADB, we can get the name of a device ("Nexus 4", "Flame", ...) and if we can also list the unix sockets. So we can then come up with a list like that:

* Flame: Firefox OS
* Nexus 4: Firefox Android
* Nexus 4: Chrome (we are working on supporting Chrome too)

We can't just initiate the connection to discover the nature of the target as initiating a connection triggers a security prompt.

Also, it's what Chrome does (scanning sockets (and processus) to know what targets are available).

Comment 21

[Paul Rouget [:paul]]

Attachment: rebased, new path

Mark, I haven't tested this patch, I don't have a fennec build environment setup on this computer. Can I ask you to test that for me?

> adb forward tcp:6000 localfilesystem:/data/local/firefoxandroid-debugger-socket

Comment 22

[Paul Rouget [:paul]]

Comment on attachment 8482514 [details] [diff] [review]
rebased, new path

Panos mentioned that creating the file in /data/local/ requires root privileges and that we need to find a better patch or use an abstract name.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Use a UNIX domain socket for debugger server connections on Android v3

OK, the path blassey mentioned in comment 15 works. Looks like using the abstract namespace will require some platform work, so I'm leaving it as a followup.

The MDN page will need an update after this for the adb forward command, but I'm hoping this will enable support for Android from Web IDE, which will let us completely hide adb commands from the user.

Paul, I left the firefoxandroid-prefixed file name in the patch, but you should be able to tell Android from Firefox OS apart by the path prefix. I think it would be simpler to use debugger-socket everywhere if you are OK with it.

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

For testing this patch I've used the following adb forward command:

adb forward tcp:6000 localfilesystem:/data/data/org.mozilla.fennec_past/firefoxandroid-debugger-socket

Comment 25

[Paul Rouget [:paul]]

Comment on attachment 8482667 [details] [diff] [review]
Use a UNIX domain socket for debugger server connections on Android v3

Perfect! That means we can support multiple Firefox running.

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8482667 [details] [diff] [review]
Use a UNIX domain socket for debugger server connections on Android v3

>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

>+// Absolute path to the devtool unix domain socket file used
>+// to communicate with a usb cable via adb forward.
>+pref("devtools.debugger.unix-domain-socket", "/data/data/@ANDROID_PACKAGE_NAME@/firefoxandroid-debugger-socket");

Looking at this more, I kinda like using "firefox-debugger-socket" which is like chrome uses on Android. Would you guys mind changing it? It's just my OCD kicking in, so if you have a good reason for "firefoxandroid-debugger-socket" I won't push back.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-      case "devtools.debugger.remote-port":
>+      case "devtools.debugger.unix-domain-socket":

I am worried about dropping support for "port" because WebApps running on Android still use ports:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/WebappRT.js#142

Can we use both?

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

CC'ing Myk so he is aware of the potential WebApps impact

Comment 28

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mark Finkle (:mfinkle) from comment #26)
> Looking at this more, I kinda like using "firefox-debugger-socket" which is
> like chrome uses on Android. Would you guys mind changing it? It's just my
> OCD kicking in, so if you have a good reason for
> "firefoxandroid-debugger-socket" I won't push back.

Since Paul agreed, I was actually going to change it to debuger-socket, like Firefox OS, for simplicity. I don't have any better reason than that though.

> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >-      case "devtools.debugger.remote-port":
> >+      case "devtools.debugger.unix-domain-socket":
> 
> I am worried about dropping support for "port" because WebApps running on
> Android still use ports:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> WebappRT.js#142
> 
> Can we use both?

We can, but why not change WebappRT to use a domain socket as well? That way WebIDE could observe those apps in the devices and offer a separate option to debug them. It could even use the package name in the socket path as the name of the target app.

Comment 29

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

(In reply to Panos Astithas [:past] from comment #28)
> > >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> > 
> > >-      case "devtools.debugger.remote-port":
> > >+      case "devtools.debugger.unix-domain-socket":
> > 
> > I am worried about dropping support for "port" because WebApps running on
> > Android still use ports:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> > WebappRT.js#142
> > 
> > Can we use both?
> 
> We can, but why not change WebappRT to use a domain socket as well? That way
> WebIDE could observe those apps in the devices and offer a separate option
> to debug them. It could even use the package name in the socket path as the
> name of the target app.

It may be best to work out the WebappRT change separately (assuming that the existing port setup for WebappRT still works with this patch), as I know Paul would like to land the Firefox (non-apps) change here quickly.

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Use a UNIX domain socket for debugger server connections on Android v4

OK, to get something in sooner rather than later, here is an updated patch that uses the firefox-debugger-socket name and leaves the option to use a port number intact. Myk, does this look OK for webapprt?

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8482828 [details] [diff] [review]
Use a UNIX domain socket for debugger server connections on Android v4

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-      let port = this._getPort();
>-      DebuggerServer.openListener(port);
>-      dump("Remote debugger listening on port " + port);
>+      let pathOrPort = this._getPath();
>+      if (!pathOrPort)
>+        pathOrPort = this._getPort();
>+      DebuggerServer.openListener(pathOrPort);
>+      dump("Remote debugger listening at path " + pathOrPort);

Since the WebApp will inherit the default profile, _getPath() will always return something. Also, WebAppRT.js will set a user pref for the "port" preference.

We need to either clear the "path" preference in WebAppRT.js or we need to see if the "port" preference is a "user" set pref, using nsIPrefBranch.prefHasUserValue(...)

Does that make sense?

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

I'm confused: if webapprt would inherit the path change without any further changes, why are we worried about breaking it by dropping the port number?

Is it about ensuring that a new desktop Firefox remains compatible with old webapprt APKs? But in that case, the old APK wouldn't have the path pref, would it?

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Panos Astithas [:past] from comment #32)
> I'm confused: if webapprt would inherit the path change without any further
> changes, why are we worried about breaking it by dropping the port number?
> 
> Is it about ensuring that a new desktop Firefox remains compatible with old
> webapprt APKs? But in that case, the old APK wouldn't have the path pref,
> would it?

No, the problem is that each webapp is running in it's own process with it's own profile. If you have two or more webapps running at the same time, you'd have a conflict. The code in WebAppRT.js finds a free port and uses it, instead of just useing the default of 6000, which is what Firefox was using anyway.

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Use a UNIX domain socket for debugger server connections on Android v5

Thanks, I understand now. I chose to clear the path in webapprt code, because the does-port-override-path-or-vice-versa algorithm becomes simpler that way: "non empty path overrides any port". It also remains clear that domain sockets are the new thing and TCP ports the old thing.

Checking for a user-set pref would make the algorithm less straightforward, apart from introducing user-overriding as an important factor in this decision: "user-overriden port overrides a non-empty path". It could also raise questions about who does the overriding (not the end user!). But I won't insist if you want it done the other way around.

Comment 35

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8483498 [details] [diff] [review]
Use a UNIX domain socket for debugger server connections on Android v5

This should work. Let's give it a go!

Comment 36

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/42bdf2b7f26f

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42bdf2b7f26f

Comment 38

[Panos Astithas (he/him) [:past] (please ni?)]

Updated the adb forwarding docs:
https://developer.mozilla.org/docs/Tools/Remote_Debugging/Firefox_for_Android#Connecting