956185 - Add a 'adb only' debugging option

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

Currently enabling remote debugging turns on adb and the debugger server used by the devtools. The debugger server itself uses a bunch a js modules, and this adds up in memory usage.

For low memory devices, it's worthwhile to have adb access while not providing remote debugging of web apps.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: debug-adb-only.patch

I'll land it with the pref set to false, and turned on only for Tarako.

Comment 2

[Jason Smith [:jsmith]]

Are we making sure that you can still enable remote debugging of web apps with an additional pref flip in the settings app?

Comment 3

[[:fabrice] Fabrice Desré]

(In reply to Jason Smith [:jsmith] from comment #2)
> Are we making sure that you can still enable remote debugging of web apps
> with an additional pref flip in the settings app?

No. We could add that, but I'm not convinced that there is much value debugging web apps on these devices. This patch let us gather memory reports, get access to the adb shell etc.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> (In reply to Jason Smith [:jsmith] from comment #2)
> > Are we making sure that you can still enable remote debugging of web apps
> > with an additional pref flip in the settings app?
> 
> No. We could add that, but I'm not convinced that there is much value
> debugging web apps on these devices. This patch let us gather memory
> reports, get access to the adb shell etc.

Well, once you lower the memory usage, then app developers are going to deal with a new challenge of figuring out how to get their apps working on a device with even lower memory. This developer use case especially becomes important for apps that might currently have high memory use, but will end up being needed to be preinstalled on the Tarako device. As such, I'd expect that there will be a need for developer workflows to debug web apps on these devices, especially partner apps.

I think it would be better to keep the option open to allow the developer to choose to enable the debugger server separate from adb, just so we don't end up foot gunning ourselves in case we get developer complaints down the line. If we're concerned about the memory use of the debugger server here, then we could issue a warning when the developer decides to enable the debugger server.

Comment 5

[[:fabrice] Fabrice Desré]

That's a fair point, but I'll think a bit more about that. We already have too many checkboxes in the developer panel!

Comment 6

[Dave Hylands [:dhylands]]

Instead of a checkbox, we could make it be a dropdown list with a list of options:

none, adb only, adb and remote debugging (and also remote debugging only if that makes sense).

Comment 7

[Dave Hylands [:dhylands]]

Comment on attachment 8355384 [details] [diff] [review]
debug-adb-only.patch

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

r=me if you actually land it with the pref set to false.

::: b2g/app/b2g.js
@@ +880,5 @@
>  pref("apz.asyncscroll.throttle", 40);
> +
> +// Set this to true to only have adb active (but no devtools) when turning
> +// on remote debugging.
> +pref("devtools.b2g.adb_only", true);

Oops. This seems to disagree with your comment of landing it with the pref set to false.

::: b2g/chrome/content/shell.js
@@ +1070,3 @@
>    // Start the debugger server.
>    start: function debugger_start() {
> +    if (!this.enabled) {

It seems to me that we should log something here. Otherwise it might be extremely frustrating to figure out why the remote debugger isn't starting.

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

Fabrice, do we know how much memory saving we can have with this? Thanks

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Joe Cheng [:jcheng] from comment #8)
> Fabrice, do we know how much memory saving we can have with this? Thanks

If I recall correctly it was around 400k. I can't wait to have the memory usage reports on datazilla to track that!

Comment 10

[[:fabrice] Fabrice Desré]

Attachment: debug-adb-only.patch v2

Dave, I changed things a bit to use a select control in gaia. To be safe I'm using a new setting and turn on/off adb and the remote debugger according to the string value of this setting.

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: gaia PR

Kaze, I'm turning the "Remote debugging" checkbox into a 3-states select control, to let us have adb without devtools.

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 8357305 [details] [diff] [review]
debug-adb-only.patch v2

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

not quite ready, just found a bug.

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: debug-adb-only.patch v3

new gecko patch, reworked to be simpler. I kept the old setting around since people usually don't upgrade gecko and gaia at the same time... I'll remove it in a couple of weeks.

Comment 14

[Dave Hylands [:dhylands]]

Comment on attachment 8357447 [details] [diff] [review]
debug-adb-only.patch v3

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

Looks good to me.

Comment 15

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/508b79b63774

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/508b79b63774

Comment 17

[Paul Rouget [:paul]]

For future changes on devtools related code, can you please ask a devtools peer for feedback/review? At least to make sure we're aware of these changes (this patch breaks some upcoming feature we're working on).

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to Paul Rouget [:paul] from comment #17)
> For future changes on devtools related code, can you please ask a devtools
> peer for feedback/review? At least to make sure we're aware of these changes
> (this patch breaks some upcoming feature we're working on).

How so? I did not touch any devtools code, only b2g/

Comment 19

[Paul Rouget [:paul]]

(In reply to Fabrice Desré [:fabrice] from comment #18)
> (In reply to Paul Rouget [:paul] from comment #17)
> > For future changes on devtools related code, can you please ask a devtools
> > peer for feedback/review? At least to make sure we're aware of these changes
> > (this patch breaks some upcoming feature we're working on).
> 
> How so? I did not touch any devtools code, only b2g/

Right, nothing we own there. I'm just saying that if you plan to change code related to RemoteDebugger and the debugger pref or settings, I'd like to know about it. We have plans to unify the way the debugger is started across the different mozilla products (B2G,Fx,metro,thunderbird…). So for future changes, a f? (well, not a r?), or a ping on IRC would be nice :)

Comment 20

[[:fabrice] Fabrice Desré]

kaze, Travis is green now!

Comment 21

[Fabien Cazenave [:kaze]]

Comment on attachment 8357307 [details] [review]
gaia PR

Thanks Fabrice, r=me.

Comment 22

[[:fabrice] Fabrice Desré]

gaia master: https://github.com/mozilla-b2g/gaia/commit/df9c33fc3a3f217c2cf607baf86bef7f937dc614

Comment 23

[Casper van Donderen (Telenor)]

Is there still a way to get the remote debugger to start automatically by setting a property in the .userconfig file? It seems like only adb-only can be set in this way and not 'ADB and Devtools'

Comment 24

[Dave Hylands [:dhylands]]

Try this (in your .userconfig)

cat > gaia/build/custom-settings.json <<EOF
{"devtools.debugger.remote-mode": "adb-devtools"}
EOF

Comment 25

[[:fabrice] Fabrice Desré]

(In reply to Casper van Donderen from comment #23)
> Is there still a way to get the remote debugger to start automatically by
> setting a property in the .userconfig file? It seems like only adb-only can
> be set in this way and not 'ADB and Devtools'

What Dave said, or we could turn on adb-devtools by default for some configs. I don't have a strong opinion one way or the other.

Comment 26

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

Maybe DEVICE_DEBUG=1 should start both, instead of just ADB?

Comment 27

[Paul Rouget [:paul]]

bug 942756 will make sure that the server debugger is started based on the pref (not the setting).

But the default setting value should come from the pref.

Comment 28

[Casper van Donderen (Telenor)]

I cannot get that custom-settings.json to work. It does get added in the file, but the device still defaults to adb-only (using in combination with PRODUCTION=1 & DEVICE_DEBUG=1)

Comment 29

[Dave Hylands [:dhylands]]

It looks like gaia/build/settings.js only sets adb-only when config.REMOTE_DEBUGGER is set, and DEVICE_DEBUG also only sets adb-only.

Personally, I think that this is a bug, and that settings.js should use adb-devtools

There was also 2 mistakes in my override. It should look like:

cat > gaia/build/config/custom-settings.json <<EOF
{"debugger.remote-mode": "adb-devtools"}
EOF

The 2 changes are that the path should be: gaia/build/config/custom-settings.json and the setting should be debugger.remote-mode (without the devtools. at the front).

I confirmed that with the above, my unagi in VARIANT=user builds defaults to adb-devtools.

Comment 30

[Joe Cheng [:jcheng] (please needinfo)]

remove [tarako] in whiteboard and use 1.3T?

Comment 31

[Joe Cheng [:jcheng] (please needinfo)]

triage: save ~400KB for tarako. 1.3T+

Comment 32

[[:fabrice] Fabrice Desré]

gecko patch on 1.3t: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/71d2b5f66cf1

Tim, can you uplift the gaia part to 1.3t? thanks! (it's https://github.com/mozilla-b2g/gaia/commit/df9c33fc3a3f217c2cf607baf86bef7f937dc614)

Comment 33

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.3t: 0f84d0b0b9bfea1c463037f05954d0b4685b1f89

Conflict resolved by removing build tests... we don't have that in v1.3 or v1.3t.

Fabrice, could you verify?

Comment 34

[[:fabrice] Fabrice Desré]

Tim, thanks, that looks ok on my device!

Comment 35

[Montoya Clemmons (MClemmons)]

Verified ADB only option in remote debugging section on latest 1.3T build. 

Environmental Variables:
Device: Tarako 1.3T MOZ
BuildID: 20140429014002
Gaia: b5adc5a943d3abbd6ab070a47c847f2c24891cc5
Gecko: e9890f5d4709
Version: 28.1
Firmware Version: sp6821a_gonk4.0_user.pac