836770 - Don't change persist.sys.usb.config if it already had the correct value (ADB)

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Dave Hylands [:dhylands]]

It looks like we need to check the current setting of persist.sys.usb.config and not try to set it if it already has the correct value.

Othwerwise, it will restart the adb server. If you're trying to use run-gdb.sh then restarting adb bumps it out of the gdb session.

Nominating tef? since this blocks a tef+

Comment 1

[Dave Hylands [:dhylands]]

Attachment: Don't set persist.sys.usb.config if it doesn't change.

Don't set persist.sys.usb.config unless it actually changes.

Otherwise, if adb was enabled before, and is enabled after, it will stop and start adb.

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 708645 [details] [diff] [review]
Don't set persist.sys.usb.config if it doesn't change.

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

::: b2g/chrome/content/settings.js
@@ +209,5 @@
>    try {
>      let current = libcutils.property_get("persist.sys.usb.config");
>      let prefix = current.replace(/,adb/, "");
> +    let new_config = prefix + (value ? ",adb" : "");
> +    if (new_config != current) {

nit: s/new_config/newConfig

Comment 3

[Mike Hommey [:glandium]]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> Comment on attachment 708645 [details] [diff] [review]
> Don't set persist.sys.usb.config if it doesn't change.
> 
> Review of attachment 708645 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/chrome/content/settings.js
> @@ +209,5 @@
> >    try {
> >      let current = libcutils.property_get("persist.sys.usb.config");
> >      let prefix = current.replace(/,adb/, "");
> > +    let new_config = prefix + (value ? ",adb" : "");
> > +    if (new_config != current) {
> 
> nit: s/new_config/newConfig

This is going to restart adb if the previous had adb, but not at the end.

I did this, locally:
diff --git a/b2g/chrome/content/settings.js b/b2g/chrome/content/settings.js
index 5b1ecc4..2090d88 100644
--- a/b2g/chrome/content/settings.js
+++ b/b2g/chrome/content/settings.js
@@ -200,10 +200,10 @@ SettingsListener.observe('devtools.debugger.remote-enabled', false, function(val
   // Configure adb.
   try {
     let current = libcutils.property_get("persist.sys.usb.config");
-    let prefix = current.replace(/,adb/, "");
-    libcutils.property_set("persist.sys.usb.config",
-                           prefix + (value ? ",adb" : ""));
-    current = libcutils.property_get("persist.sys.usb.config");
+    if (value && !current.contains(",adb"))
+      libcutils.property_set("persist.sys.usb.config", current + ",adb");
+    else if (!value && current.contains(",adb"))
+      libcutils.property_set("persist.sys.usb.config", current.replace(/,adb/, ""));
   } catch(e) {
     dump("Error configuring adb: " + e);
   }

Comment 4

[Dave Hylands [:dhylands]]

(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to Fabrice Desré [:fabrice] from comment #2)
> > Comment on attachment 708645 [details] [diff] [review]
> > Don't set persist.sys.usb.config if it doesn't change.
> > 
> > Review of attachment 708645 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: b2g/chrome/content/settings.js
> > @@ +209,5 @@
> > >    try {
> > >      let current = libcutils.property_get("persist.sys.usb.config");
> > >      let prefix = current.replace(/,adb/, "");
> > > +    let new_config = prefix + (value ? ",adb" : "");
> > > +    if (new_config != current) {
> > 
> > nit: s/new_config/newConfig
> 
> This is going to restart adb if the previous had adb, but not at the end.

True, but ,adb is always at the end. For example, if persist.sys.usb could be:

diag,modem,nmea,mass_storage,adb

diag,modem,nmea,adb,mass_storage

is NOT equivalent. The way the android property thing work, the string MUST EXACTLY MATCH the strings presented in init.rc

So for example, from /init.qcom.usb.rc:

on property:sys.usb.config=diag,modem,nmea,mass_storage,adb

This will only match exactly that string and not a string with adb in a different location.

So while, I think you may have a valid point, it requires more than just what you're proposing. If we're going to add ,adb and its not at the end, then we have to somehow know where to insert it.

Comment 5

[Dave Hylands [:dhylands]]

Attachment: Don't set persist.sys.usb.config if it doesn't change. v2

Addresses glandium's concerns

Also deals with the other edge cases

Comment 6

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dea32aeff13e

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dea32aeff13e

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking to go with blocking dependencies.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5db70f674de7
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/830b5f88e3c3

Comment 10

[amiller]

Does not make sense to create a regression issue.

Comment 11

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 12

[Dave Hylands [:dhylands]]

Since adb is only changed when the phone was built using VARIANT=user, you'll need a phone running a VARIANT=user build. The nightlies that we run on the unagis will satisfy this.

First: Verify that you can actually control adb using the remote debugging setting.

1 - Launch the setting app
2 - Navigate to Device Information -> More Information -> Developer
3 - Verify that when "Remote debugging" is checked that the phone shows up in adb devices, and that when "Remote debugging" is unchecked, that the phone does NOT show up in adb devices.

If the phone shows up in adb devices regardless of whether "Remote debugging" is checked or not, then you're most likely not running a VARIANT=user build.

Steps to verify this fix:

1. Leave "Remote debugging" checked.
2. Start an adb shell session (i.e. issue command adb shell)
3. From within the shell do: stop b2g
4. Then do: start b2g
5. The adb shell session should remain active while b2g is booting, and you shouldn't be dropped from the session by the time you see the lock screen on the phone.
6. Verify that the adb session is still active by doing something like: b2g-ps

Comment 13

[Cody Roesch [:croesch]]

Unagi Build: 20130401070203
Gecko:  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/b28463f2e718
Gaia:   ddb38ac8a34f9e30e09d0ff3b5c1bfb9b664b7c3
Kernel: Dec 5th

Unagi Build: 20130401070203
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/f9f11b8cbf8a
Gaia   663101b6eb809383e5882d9bc3868a923a57998a
Kernel: Dec 5th

Wow these were great steps and easily understandable! Thanks for that!
Verified the fix was was active in both builds shown above. Sessions remained active both times.