[remote-dbg-next] Fix the timing of enable/disable ADB

RESOLVED FIXED in Firefox 65

Status

P1
enhancement
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: daisuke, Assigned: jdescottes)

Tracking

(Blocks: 3 bugs)

unspecified
Firefox 65
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: old-remote-debugging-ng-m3)

Attachments

(17 attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
1.14 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 months ago
Currently, we enable/disable ADB when about:debugging page was opened/closed (bug 1405235).
However, if user is opening another about:debugging page or another application is using, we should not disable ADB.
The other hand, if another application made ADB disable, we should enable again.
So, we need to consider the timing of enable/disable ADB.
Priority: P3 → P2
(Assignee)

Comment 1

4 months ago
The status for this one changed a bit. Now when about:debugging starts, we attempt to start ADB if needed (when we start the scanner, if the ADB extension is installed). And when we stop about:debugging we just destroy the scanner but we don't try to shutdown ADB.

I think that for m1 the current behavior is ok, and we should not shutdown ADB automatically when closing about:debugging. ADB is a process, we should not assume about:debugging is the only consumer. However, maybe it could be interesting to let the user know that the ADB process is currently running and give them a way to stop it?
(Assignee)

Updated

4 months ago
Blocks: 1494542
No longer blocks: 1462211
(Assignee)

Comment 2

3 months ago
filter on remote-debugging-next-move-m3-to-m2
Blocks: 1494541
(Assignee)

Comment 3

3 months ago
filter on remote-debugging-next-move-m3-to-m2
(Assignee)

Comment 4

3 months ago
filter on remote-debugging-next-move-m3-to-m2
No longer blocks: 1494542
Whiteboard: old-remote-debugging-ng-m3
(Assignee)

Comment 5

3 months ago
Technically we should implement a mechanism to stop ADB if the process was started by firefox and if no client uses it anymore. 

I think we should also discuss if the status of ADB should be visible somewhere in the UI. It might be interesting for troubleshooting purposes. I think we should handle this in a separate bug.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Updated

3 months ago
Blocks: 1509313
(Assignee)

Comment 6

3 months ago
Created attachment 9027195 [details]
Bug 1492700 - Fix ADB stop() in case ADB start() was called several times;r=daisuke
(Assignee)

Comment 7

3 months ago
Created attachment 9027196 [details]
Bug 1492700 - Stop sending adb-addon updates from Devices.jsm;r=daisuke

Depends on D12758
(Assignee)

Comment 8

3 months ago
Created attachment 9027197 [details]
Bug 1492700 - Move Devices.jsm to devtools/shared/adb folder;r=daisuke

Depends on D12759
(Assignee)

Comment 9

3 months ago
Created attachment 9027198 [details]
Bug 1492700 - Convert Devices.jsm to adb-devices-registry module;r=daisuke

Depends on D12760
(Assignee)

Comment 10

3 months ago
Created attachment 9027199 [details]
Bug 1492700 - adb-device.js should export AdbDevice and not Device;r=daisuke

Depends on D12761
(Assignee)

Comment 11

3 months ago
Created attachment 9027200 [details]
Bug 1492700 - Stop exposing shell() on adb-device because it only forwards to ADB;r=daisuke

Depends on D12762
(Assignee)

Comment 12

3 months ago
Created attachment 9027201 [details]
Bug 1492700 - Fold FirefoxOnAndroidRuntime.detect in scanner.detectRuntimes;r=daisuke

Depends on D12763
(Assignee)

Comment 13

3 months ago
Created attachment 9027203 [details]
Bug 1492700 - Merge Runtime and FirefoxForAndroidRuntime into AdbRuntime;r=daisuke

Depends on D12764
(Assignee)

Comment 14

3 months ago
Created attachment 9027204 [details]
Bug 1492700 - Extract AdbRuntime out of adb-scanner.js;r=daisuke

Depends on D12765
(Assignee)

Comment 15

3 months ago
Created attachment 9027205 [details]
Bug 1492700 - Migrate AdbRuntime to a class;r=daisuke

Depends on D12766
(Assignee)

Comment 16

3 months ago
I am trying to cleanup a bit our ADB modules in the scope of this bug. The reason behind is that if we want to stop the ADB process when all the "devtools" consumers of ADB no longer want to get ADB updates, then it means we need a clear entry point for all the interaction with ADB. 

In practice, we need to have some global module that will keep track of the consumers, and that will stop the ADB process if the number of consumers is down to 0 _and_ if ADB was started by devtools.

At the moment, all the consumers of ADB are using the "addon-aware-adb-scanner". This means that we could quickly hack something on top of this module to achieve the feature of this bug. However some other modules are also relying on ADB: about:devtools-toolbox when connected to a remote USB runtime for instance. Those modules don't need to scan for ADB runtimes, they just to say that they "need" ADB.

So I would like to cleanup the adb folder so that we can have small public API:
- adb-addon.js 
- adb.js
Keywords: leave-open

Comment 17

3 months ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5f900b18101
Fix ADB stop() in case ADB start() was called several times;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/5199632a4ada
Stop sending adb-addon updates from Devices.jsm;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/7590eb03b4ff
Move Devices.jsm to devtools/shared/adb folder;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/363e8b2ecfab
Convert Devices.jsm to adb-devices-registry module;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/65825c93b47b
adb-device.js should export AdbDevice and not Device;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/d5e67b8fbde3
Stop exposing shell() on adb-device because it only forwards to ADB;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/5db9614af6a5
Fold FirefoxOnAndroidRuntime.detect in scanner.detectRuntimes;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/bbc5bcb72208
Merge Runtime and FirefoxForAndroidRuntime into AdbRuntime;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/21f0a93a65c2
Extract AdbRuntime out of adb-scanner.js;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/ad28397dfa43
Migrate AdbRuntime to a class;r=daisuke
(Assignee)

Comment 19

3 months ago
Created attachment 9028787 [details]
Bug 1492700 - Split adb.js in several files;r=daisuke
(Assignee)

Comment 20

3 months ago
Created attachment 9028788 [details]
Bug 1492700 - Migrate adb/commands/track-devices to a Class;r=daisuke

Depends on D13473
(Assignee)

Comment 21

3 months ago
Created attachment 9028789 [details]
Bug 1492700 - Remove unused observables in commands/track-devices;r=daisuke

Depends on D13474
(Assignee)

Comment 22

3 months ago
Created attachment 9028790 [details]
Bug 1492700 - Rename adb.js to adb-process.js and switch to class;r=daisuke

Depends on D13475
(Assignee)

Comment 23

3 months ago
Created attachment 9028791 [details]
Bug 1492700 - Introduce adb singleton to register adb consumers;r=daisuke

Depends on D13476
(Assignee)

Comment 24

3 months ago
Created attachment 9028792 [details]
Bug 1492700 - Add mochitest to check that ADB is stopped after closing about:debugging;r=daisuke

Depends on D13477
(Assignee)

Updated

3 months ago
Keywords: leave-open
(Assignee)

Updated

3 months ago
Blocks: 1511779

Comment 25

3 months ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d8c5a3fc14e
Split adb.js in several files;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/41384e20308d
Migrate adb/commands/track-devices to a Class;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/271eb6988c58
Remove unused observables in commands/track-devices;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/c804e90f0ce5
Rename adb.js to adb-process.js and switch to class;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/738cae90db60
Introduce adb singleton to register adb consumers;r=daisuke
https://hg.mozilla.org/integration/autoland/rev/ec57d6145de7
Add mochitest to check that ADB is stopped after closing about:debugging;r=daisuke
(Assignee)

Comment 26

3 months ago
Created attachment 9029817 [details] [diff] [review]
fix_eslint_failure.patch

Fix eslint failure.
Attachment #9029817 - Flags: review+

Comment 27

3 months ago
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2aa7b7e89e14
Fix eslint failure in test_adb.js;r=fix
You need to log in before you can comment on or make changes to this bug.