1493104 - [remote-dbg-next] adb-scanner should monitor ADB extension status

Status: RESOLVED FIXED

(DevTools :: about:debugging, enhancement, P1)

Description

[Julian Descottes [:jdescottes]]

Today, consumers of devtools/shared/adb/adb-scanner have to be careful about the status of the ADB extension before enabling/disabling the scanner. 

It would be nice if the scanner (or another wrapper) could monitor the status of the extension so that consumers of this code can safely enable/disable the scanner without having to check if the ADB extension is installed.

Such a module could be shared by both WebIDE and the new about:debugging.

(I am blocking m1 for this one because I think cleaning this up will make our lives easier even for early testing of USB debugging, but if we are really short on time we can move out to m2).

Comment 1

[Julian Descottes [:jdescottes]]

I am tempted to simply refactor the ADB scanner rather than doing small fixes for WebIDE/about:debugging for Bug 1496415 and Bug 1496108.

We have two issues today:

Shared singleton:
A single adb-scanner is shared by about-debugging and WebIDE. When WebIDE calls disable() on the scanner, it also disables the scanner for about:debugging (well actually it keeps scanning because the enable/disable logic is buggy as soon as you have more than one consumer of the scanner.

I propose to make ADBScanner a class that both WebIDE and about:debugging can instanciate, so that each one deals with its own scanner. It means we will create more listeners to Devices.jsm and each instance will attempt to start ADB. Normally our ADB module should work if the ADB process is already running, so I assume it is fine to start it twice. If this proves to be an issue. The alternate solution will be to keep a single shared instance of the scanner that would keep track of its consumers.


ADB addon dependency
the adb-scanner can only be enabled() after the addon is installed, because it needs to start ADB() which needs the binaries to be available. This logic is not clearly exposed. The closest thing we have to an implementation is in the addons UI module for WebIDE: devtools/client/webide/content/addons.js. But it is 1/ buggy, 2/ tied to the webIDE addons UI ...

Option 1 is to duplicate the enable/disable logic in WebIDE and about:debugging. The logic is fairly simple:

  if addon installed -> adbScanner.enable()
  on addon "update"
    if addon installed -> adbScanner.enable()
    else -> adbScanner.disable()

It simply requires WebIDE and about:debugging modules to be aware of the addon and of the relationship between the addon and the scanner.

Option 2 is to make this transparent in the scanner. The scanner (or an AddonAwareADBScanner decorator...) would listen to the addon updates and lazily call enable/disable when the addon update changes.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1493104 - Convert ADBScanner from a singleton to a class;r=ladybenko,daisuke

This allows to cleanly disable the scanner when you have several users.
Without this if you start both WebIDE and about:debugging, the first listeners
set in enable() can never be removed.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1493104 - Listen to ADB extension updates in about:debugging usb-runtime helper;r=ladybenko,daisuke

Depends on D7872. This fixes the bug where about:debugging thinks the addon
is uninstalled the first time it starts.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1493104 - Add adbScanner to WebIDE scanners if status is already installed;r=ladybenko,daisuke

Depends on D7873.
This fixes the following scenario:
- install ADB
- connect a Device
- start about:debugging
=> device shows up in sidebar
- start WebIDE
=> device should show up in WebIDE

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1493104 - Add ADBScanner decorator to listen to addon status udpates;r=ladybenko,daisuke

Depends on D7875
I would like to make the adb addon dependency transparent to the scanner consumers

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1493104 - Add unit test for AddonAwareADBScanner;r=ladybenko,daisuke

Depends on D7876

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa20c2c68c42
Convert ADBScanner from a singleton to a class;r=ladybenko,daisuke
https://hg.mozilla.org/integration/autoland/rev/f7ecf841eb8f
Listen to ADB extension updates in about:debugging usb-runtime helper;r=ladybenko,daisuke
https://hg.mozilla.org/integration/autoland/rev/cb9f55dd4257
Add adbScanner to WebIDE  scanners if status is already installed;r=ladybenko,daisuke
https://hg.mozilla.org/integration/autoland/rev/8df2a288391c
Add ADBScanner decorator to listen to addon status udpates;r=ladybenko,daisuke
https://hg.mozilla.org/integration/autoland/rev/a6cbba097c8a
Add unit test for AddonAwareADBScanner;r=ladybenko,daisuke

Comment 8

[Cristina Coroiu [:ccoroiu]]

Backed out 5 changesets (bug 1493104) for ESlint failure at builds/worker/checkouts/gecko/devtools/client/webide/content/addons.js

Backout: https://hg.mozilla.org/integration/autoland/rev/c24bf71170530c4eadb3f8c05c42f6c08e95e036

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=204482598&revision=a6cbba097c8a8300e51d3675b8e098756bc37a63

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=204482598&repo=autoland&lineNumber=253

[vcs 2018-10-10T11:40:53.280Z] TinderboxPrint:<a href=https://hg.mozilla.org/integration/autoland/rev/a6cbba097c8a8300e51d3675b8e098756bc37a63 title='Built from autoland revision a6cbba097c8a8300e51d3675b8e098756bc37a63'>a6cbba097c8a8300e51d3675b8e098756bc37a63</a>
[task 2018-10-10T11:40:53.280Z] executing ['bash', '-cx', 'cd /builds/worker/checkouts/gecko/ && cp -r /build/node_modules_eslint node_modules && ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules && ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules && ./mach lint -l eslint -f treeherder --quiet\n']
[task 2018-10-10T11:40:53.283Z] + cd /builds/worker/checkouts/gecko/
[task 2018-10-10T11:40:53.283Z] + cp -r /build/node_modules_eslint node_modules
[task 2018-10-10T11:40:54.772Z] + ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules
[task 2018-10-10T11:40:54.773Z] + ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules
[task 2018-10-10T11:40:54.774Z] + ./mach lint -l eslint -f treeherder --quiet
[task 2018-10-10T11:40:55.589Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python2.7
[task 2018-10-10T11:40:55.589Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python
[task 2018-10-10T11:40:57.258Z] Installing setuptools, pip, wheel...done.
[task 2018-10-10T11:40:58.484Z] running build_ext
[task 2018-10-10T11:40:58.484Z] building 'psutil._psutil_linux' extension
[task 2018-10-10T11:40:58.484Z] creating build
[task 2018-10-10T11:40:58.484Z] creating build/temp.linux-x86_64-2.7
[task 2018-10-10T11:40:58.485Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2018-10-10T11:40:58.485Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-10-10T11:40:58.485Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-10-10T11:40:58.485Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2018-10-10T11:40:58.485Z] creating build/lib.linux-x86_64-2.7
[task 2018-10-10T11:40:58.485Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2018-10-10T11:40:58.485Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2018-10-10T11:40:58.485Z] building 'psutil._psutil_posix' extension
[task 2018-10-10T11:40:58.485Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-10-10T11:40:58.485Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-10-10T11:40:58.485Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2018-10-10T11:40:58.485Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-10-10T11:40:58.485Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-10-10T11:40:58.485Z] 
[task 2018-10-10T11:40:58.485Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-10-10T11:45:53.023Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/webide/content/addons.js:13:1 | 'ADB_ADDON_STATES' is defined but never used. (no-unused-vars)
[taskcluster 2018-10-10 11:45:53.359Z] === Task Finished ===
[taskcluster 2018-10-10 11:45:53.360Z] Unsuccessful task run with exit code: 1 completed in 522.3 seconds

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d25a11674ba
Convert ADBScanner from a singleton to a class;r=ladybenko,daisuke
https://hg.mozilla.org/integration/mozilla-inbound/rev/a56d01a74964
Listen to ADB extension updates in about:debugging usb-runtime helper;r=ladybenko,daisuke
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed3ff9c48f5
Add adbScanner to WebIDE  scanners if status is already installed;r=ladybenko,daisuke
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba67a0aadb68
Add ADBScanner decorator to listen to addon status udpates;r=ladybenko,daisuke
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa920249ca47
Add unit test for AddonAwareADBScanner;r=ladybenko,daisuke

Comment 12

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/6d25a11674ba
https://hg.mozilla.org/mozilla-central/rev/a56d01a74964
https://hg.mozilla.org/mozilla-central/rev/5ed3ff9c48f5
https://hg.mozilla.org/mozilla-central/rev/ba67a0aadb68
https://hg.mozilla.org/mozilla-central/rev/fa920249ca47