Closed
Bug 836770
Opened 11 years ago
Closed 11 years ago
Don't change persist.sys.usb.config if it already had the correct value (ADB)
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-b2g:tef+, 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)
1.84 KB,
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•11 years ago
|
Assignee: nobody → dhylands
Assignee | ||
Comment 1•11 years ago
|
||
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 2•11 years ago
|
||
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+
Comment 3•11 years ago
|
||
(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); }
Assignee | ||
Comment 4•11 years ago
|
||
(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.
Assignee | ||
Comment 5•11 years ago
|
||
Addresses glandium's concerns Also deals with the other edge cases
Attachment #708645 -
Attachment is obsolete: true
Attachment #708705 -
Flags: review?(fabrice)
Updated•11 years ago
|
Attachment #708705 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dea32aeff13e
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dea32aeff13e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 9•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5db70f674de7 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/830b5f88e3c3
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → fixed
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Target Milestone: --- → B2G C4 (2jan on)
Updated•11 years ago
|
status-b2g18-v1.0.1:
--- → fixed
Comment 11•11 years ago
|
||
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
Assignee | ||
Comment 12•11 years ago
|
||
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•11 years ago
|
||
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.
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•