Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Detect package name when using abstract socket address #43

Merged
merged 1 commit into from
Aug 3, 2018

Conversation

makotokato
Copy link
Member

Follow up https://bugzilla.mozilla.org/show_bug.cgi?id=1462019. When using abstract socket address, its name is @/firefox-debugger-socket. So to recongnize package name in WebIDE, adbhelper should support this socket name.

scanner.js Outdated
@@ -169,7 +169,10 @@ FirefoxOnAndroidRuntime.prototype = Object.create(Runtime.prototype);

Object.defineProperty(FirefoxOnAndroidRuntime.prototype, "name", {
get() {
let packageName = this._socketPath.split("/")[3];
// If using abstract socket address, it is "@org.mozilla.firefox/..."
let packageName = this._socketPath.startsWith("@") ?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 'const' otherwise lint tools will complain when this change gets merged into devtools.

Copy link
Contributor

@juliandescottes juliandescottes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I just have a few questions related to https://bugzilla.mozilla.org/show_bug.cgi?id=1462019

After https://bugzilla.mozilla.org/show_bug.cgi?id=1462019, do we still need to handle the split("/")[3] ? Was this just related to using MOZ_ANDROID_DATA_DIR ? If this is the case could we add a comment to say that this._socketPath.split("/")[3] is only useful for Firefox 62 or older? If not, please ignore this comment :)

Second question is about old runtimes. Are we sure there is no risk of having a MOZ_ANDROID_DATA_DIR starting with @?

@makotokato
Copy link
Member Author

@juliandescottes

do we still need to handle the split("/")[3] ?

  • Until Fennec 62 only supports path based UNIX domain socket (/data/data/<package name>/firefox-debugger-socket)
  • Fennec 63+ supports both path based and abstract socket (@<package name>/firefox-debugger-socket). you can change it by devtools.debugger.unix-domain-socket
  • GeckoView 63+ only supports abstract socket because socket name is hard-coded (@<package name>/firefox-debugger-socket)

So If WebIDE supports both version, we still need to support old style name.

@juliandescottes
Copy link
Contributor

@makotokato Thanks for the feedback, maybe this could be added as a comment above let packageName = this._socketPath.startsWith("@") ? ?

@makotokato makotokato force-pushed the detect-package branch 2 times, most recently from b55db39 to 48e165f Compare August 2, 2018 06:39
@juliandescottes
Copy link
Contributor

@makotokato thanks for updating the comment! Any thoughts about my second question:

Second question is about old runtimes. Are we sure there is no risk of having a MOZ_ANDROID_DATA_DIR starting with @?

Happy to validate the review after this.

@makotokato
Copy link
Member Author

Second question is about old runtimes. Are we sure there is no risk of having a MOZ_ANDROID_DATA_DIR starting with @?

C:\Development>adb shell cat /proc/net/unix | findstr firefox
0000000000000000: 00000002 00000000 00010000 0001 01 1349065 @org.mozilla.fennec
_aurora/firefox-debugger-socket
0000000000000000: 00000002 00000000 00010000 0001 01 1827399 /data/data/org.mozi
lla.firefox_beta/firefox-debugger-socket

When using abstract socket address, /proc/net/unix has '@' prefix. if using path based socket, it is full path (/data/data/<package name>/...). So when using old runtime, no '@' prefix for socket because it is file path.

@juliandescottes
Copy link
Contributor

ok thanks! merging

@juliandescottes juliandescottes merged commit d37963c into mozilla:master Aug 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants