Closed Bug 836770 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 verified, b2g18-v1.0.0 fixed, b2g18-v1.0.1 verified)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- verified
b2g18-v1.0.0 --- fixed
b2g18-v1.0.1 --- verified

People

(Reporter: dhylands, Assigned: dhylands)

References

Details

Attachments

(1 file, 1 obsolete file)

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+
Assignee: nobody → dhylands
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.
Attachment #708645 - Flags: review?(fabrice)
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
Attachment #708645 - Flags: review?(fabrice) → review+
(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);
   }
(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.
Addresses glandium's concerns

Also deals with the other edge cases
Attachment #708645 - Attachment is obsolete: true
Attachment #708705 - Flags: review?(fabrice)
Attachment #708705 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/dea32aeff13e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 836973
Blocking to go with blocking dependencies.
blocking-b2g: tef? → tef+
Does not make sense to create a regression issue.
Flags: in-moztrap-
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
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
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.