915974 - USB connections gets disconnected when phone screen times out

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Parul Mathur]

Test Environment:
Device: Leo
OS version: 1.2.0.0-prerelease
Firmware revision: D300f10a
Hardware revision: d300
Platform version: 26.0a1
Build Identifier: 20130911040201
Update channel: leo/1.2.0/nightly
Gaia: ebbb325
Git commit info: 2013-09-11 10:05:39

Steps to reproduce:
1. Ensure phone has SD Card inserted. Connect phone to computer via USB cable.
2. Go to Settings > Display and set Screen timeout to 1 minute.
3. Go to Settings > Storage and enable USB storage. 

Expected Results:
The SD Card on the phone is mounted on the computer and shown as file storage. This should persist even after the phone screen times out in 1 minute.

Actual Results:
The SD Card on the phone is mounted on the computer and shown as file storage. After the phone screen times out in 1 minute, the USB connection breaks. The computer no longer shows the phone SD Card as file storage.

Comment 1

[Natalya Kot [:nkot]]

blocked to test this issue on Leo m-r due to a bug 887486, 
the behavior seems to be different on other devices, have tested on Buri 20130911040201 build (same as in comment 0)- the issue does NOT reproduce

Comment 2

[Jason Smith [:jsmith]]

Okay - that implies that this is likely a partner-specific issue.

Comment 3

[Dave Hylands [:dhylands]]

The correct behaviour is as follows:

If UMS (USB Mass Storage) is enabled, and the phone is locked, then UMS will remain disabled until the phone is unlocked.

Once UMS is enabled, it should stay enabled, even when the phone locks itself, until the cable is unplugged, or the option is disabled in settings (which further requires that the cable be unplugged to complete).

I tested on master/unagi and it behaves the way its supposed to.

Having a logcat of the failing device would help to identify where the problem is, but it seems to be device specific.

Comment 4

[Dave Hylands [:dhylands]]

Hmm. I wonder if this is related to bug 810092 (adb gets disabled when the screen is locked).

If adb is enabled, then disabling adb might be having a side-effect of also disabling UMS (they're both controlled through the same android property), and the actual implementation is vendor specific.

I'll do some further tests on a user build to see if this happens (VARIANT=eng and VARIANT=eng always have adb enabled, so they wouldn't be affected)

Comment 5

[Dave Hylands [:dhylands]]

So I can reproduce this on my unagi using a VARIANT=user build with remote debugging enabled. When the screen locks, the UMS session is disabled and re-enabled.

In the code before the screen is locked, persist.sys.usb.config is set to: diag,modem,nmea,mass_storage,adb

When the screen is locked we change persist.sys.usb.config to diag,modem,nmea,mass_storage

The exact behaviour of how this gets dealt with is determined by the vendor portion of the code.

Comment 6

[Tony Chung [:tchung]]

please triage.  comment 5 proves its not specific to partner.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Tony Chung [:tchung] from comment #6)
> please triage.  comment 5 proves its not specific to partner.

This is likely povb per above discussion, which doesn't fall under our scope to fix. Unless I'm misinterpreting the above discussion.

Comment 8

[Dave Hylands [:dhylands]]

The behaviour is common across many phones, but not all phones (IIRC my sgs2 didn't behave this way).

The behaviour comes from the kernel/init.rc files provided by the SoC provider, which is why you see it across multiple phones.

Comment 9

[Marco Chen [:mchen]]

Hi Leo,

Refer to command 5, when B2G tried to disable ADB from persist.sys.usb.config, it seems to that the following actions will effect the mass_storage too. Could you help to give some suggestion on this? Thanks.

Comment 10

[Dave Hylands [:dhylands]]

Another option that we have is that we could detect that mass_storage and adb are both turned on, and not turn off adb when mass_storage is enabled, although I'd probably need to hear that this is OK from our security team.

Comment 11

[leo.bugzilla.fw]

I think comment10 is the best way to solve the issue, at now.

Comment 12

[Marco Chen [:mchen]]

(In reply to Dave Hylands [:dhylands] (on PTO back Oct 15) from comment #10)
It is a quick and simple way for this issue if there is no security concern. Good suggestion.

Comment 13

[Dave Hylands [:dhylands]]

And even though it says I'm on PTO, I will be trying to check emails once a day, and actually working a couple of days after the summit before I get back.

So I'll be available to either code this up, or review a patch if somebody else codes it up (and if not, fabrice can probably do it).

I'd just like to get an ok from Paul. Not disabling adb if mass storage is pretty simple, and would just go in b2g/chrome/content/settings.js

We can easily not disable adb if mass_storage is enabled.

It isn't so easy to have adb also get disabled when mass_storage gets disabled (I'm not aware of any notifications that occur when this happens), but adb will get back in sync as soon as the user goes through the next unlock/lock cycle of the phone.

Right now the automounter uses the ums.mode setting to control the automounter (the settings app uses ums.enabled), but the automounter can change the mode internally without updating the setting. If we had the Automounter reflect its internal mode in the ums.mode, the we could easily add an observer that watches when ums.mode gets set to disabled.

The other way would be if there is a way to observe changes in an android property (which would be useful for another automounter bug). IN that case we could just update the adb state anytime we observe a change in persist.sys.usb.config.

Comment 14

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Not disabling adb while a phone is mounted via USB Mass storage seems fine to me. But I think you mean 'not disabling adb while the user has the 'USB Mass Storage Setting' enabled. That isn't terrible but if there was a way to do the former I would be better.

Or put it this way, my ideal preference for all things USB related are below.

State: Device not connected via USB cable:
If your phone is locked, you can't initiate any USB related connections (UMS, adb shell, remote debugging etc).
If your phone is unlocked, you can initiate any USB connections based on your settings.

State: Device connected via USB cable (as, plugged in, regardless of active debugging, UMS etc)
Only make changes to connections based on settings changes (ie no need to make any changes based on lockscreen state)

Possible Solution?
In "setLockscreenState" can we check if the device is currently connected via USB? If it is connected, then just don't call  this.updateState(); 

Would that do what I outlined above? Not sure if I am missing edge cases or shooting myself in the foot here.

Comment 15

[Paul Theriault [:pauljt] (no longer reading bugmail)]

PS I am away from today for a while so CC'ing dchan who can answer in my absence.

Comment 16

[Dave Hylands [:dhylands]]

So I think that this means that we need to know either:

1 - When the USB cable is unplugged
2 - When USB Mass Storage changes mode from "disabled when unplugged" to "disabled" (which happens then the USB cable is unplugged).

I'd prefer to implement #2, and since ums.mode is a setting, it would be trivial to make adb update its state whenever ums.mode is changed to disabled.

Comment 17

[Preeti Raghunath(:Preeti)]

Dave,

This sounds like expected behavior. If not, what next steps? Also, why would this block 1.2?

Comment 18

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I would say that we should probably file a separate bug to cleanly disconnect the USB Mass storage so that it doesn't corrupt the SD card before locking out the USB Mass storage.

Comment 19

[Andrew Overholt [:overholt]]

I spoke about this with Dave via IRC.  He agrees we shouldn't hold 1.2 for this since this is only an issue when adb is enabled which isn't terribly common.

Comment 20

[Dave Hylands [:dhylands]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #18)
> I would say that we should probably file a separate bug to cleanly
> disconnect the USB Mass storage so that it doesn't corrupt the SD card
> before locking out the USB Mass storage.

We can't cleanly disconnect the USB Mass Storage from the phone side. It has to be done from the PC side. So all we can do is to leave adb running until the user stops mass storage on the PC side, and unplugs the USB cable.

Comment 21

[Dave Hylands [:dhylands]]

(In reply to Preeti Raghunath(:Preeti) from comment #17)
> Dave,
> 
> This sounds like expected behavior. If not, what next steps? Also, why would
> this block 1.2?

I think that we should fix this as described in comment 16 point #2
I don't think that this should block 1.2

Comment 23

[Dave Hylands [:dhylands]]

We should also experiment and see if turning remote debugging on after UMS is enabled causes any troubles. If it does, then we need to not allow turning on adb while UMS is enabled.

Tethering may also be impacted, so whatever it is we do for UMS will probably require similar treatment for tethering.

Comment 24

[cajbir (:cajbir)]

I'm seeing this on a ZTE Open running master. I was copying music files from PC to phone and it kept getting disconnected when the screen turned off.

Comment 25

[Dave Hylands [:dhylands]]

(In reply to Chris Double (:doublec) from comment #24)
> I'm seeing this on a ZTE Open running master. I was copying music files from
> PC to phone and it kept getting disconnected when the screen turned off.

If you turn off remote debugging then it should behave properly.

Comment 26

[Peipei Cheng (needinfo if you need my action)]

Turning off remote debugging make it behave properly. I tested this on Buri V1.2 build.

Steps to reproduce:
1. Ensure phone has SD Card inserted. Connect phone to computer via USB cable.
2. Go to Settings > Device information > More information > Developer and Enable Remote Debugging
3. Go to Settings > Storage and enable USB storage. 
   You can see SD card is mounted to PC
4. Click "Power" button to Lock Screen
   => USB connection disconnects.
5. Unlock screen
   USB connection is established
6. Disable remote debugging
7. Repeat 4,5
   USB connection is connected all the time.

Tested build:
 Gaia:     ce276842c9ac1746073271fb736dfdb626a89240                            
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/36c4c667b9f2  
BuildID   20131121004002                                                      
Version   26.0

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This bug does not only affect mass storage devices connected via USB to the phone. It's simply everything which runs via USB. What drives me crazy those days is the internet sharing option via USB. Whenever the screen goes off the USB connection gets killed and so the internet connection on my connected PC. I really don't want to have the screen always on and enabling/disabling the ADB setting is also not an option for me because it makes it even harder to report good logs while dog fooding. For me this is a major limitation as of now. So can we make some progress on it?

Comment 29

[Dave Hylands [:dhylands]]

Attachment: Don't disable adb if a USB function is currently active.

Add code to detect if USB Mass Storage, USB Tethering, or a debug session is active, and leave adbg enabled in those scenarios.

Comment 30

[[:fabrice] Fabrice Desré]

Comment on attachment 8347514 [details] [diff] [review]
Don't disable adb if a USB function is currently active.

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

Instead of having storageIndex has an AdbController property, turn it into a parameter to updateStorageState().

Comment 31

[Dave Hylands [:dhylands]]

Attachment: Don't disable adb if a USB function is currently active. v2

I like that (I had a fleeting thought about doing that way when I coded it - not sure why I didn't).

Comment 32

[Dave Hylands [:dhylands]]

Comment on attachment 8347555 [details] [diff] [review]
Don't disable adb if a USB function is currently active. v2

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

::: b2g/chrome/content/settings.js
@@ +411,5 @@
>      if (this.DEBUG) {
>        this.debug("updateState: enableAdb = " + enableAdb +
>                   " remoteDebuggerEnabled = " + this.remoteDebuggerEnabled +
>                   " lockEnabled = " + this.lockEnabled +
> +                 " locked = " + this.locked + 

I've removed this trailing space in my local patchset.

Comment 33

[[:fabrice] Fabrice Desré]

Comment on attachment 8347555 [details] [diff] [review]
Don't disable adb if a USB function is currently active. v2

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

r=me with nits fixed

::: b2g/chrome/content/settings.js
@@ +340,5 @@
> +    }.bind(this);
> +    req.onerror = function(e) {
> +      dump("AdbController: error querying storage availability for '" +
> +           this.storages[storageIndex].storageName + "' (ignoring)\n");
> +      this.updateStorageState();

this should be this.updateStorageState(storageIndex + 1);

@@ +411,5 @@
>      if (this.DEBUG) {
>        this.debug("updateState: enableAdb = " + enableAdb +
>                   " remoteDebuggerEnabled = " + this.remoteDebuggerEnabled +
>                   " lockEnabled = " + this.lockEnabled +
> +                 " locked = " + this.locked + 

nit: trailing whitespace.

Comment 34

[Dave Hylands [:dhylands]]

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

Comment 35

[Carsten Book [:Tomcat]]

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

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I hope that we can get this backported to 1.2.

Comment 37

[Dave Hylands [:dhylands]]

Nominating for 1.2 and/or 1.3

Impact: This patch only affects users which use adb (i.e. typically developers).

Risk: This patch affects the code which is used to determine when adb is enabled/disabled. I think this makes it fairly low risk.

Comment 38

[Jason Smith [:jsmith]]

(In reply to Dave Hylands [:dhylands] from comment #37)
> Nominating for 1.2 and/or 1.3
> 
> Impact: This patch only affects users which use adb (i.e. typically
> developers).
> 
> Risk: This patch affects the code which is used to determine when adb is
> enabled/disabled. I think this makes it fairly low risk.

I don't think this would be a good candidate to block 1.2 on - we're way late in the release at this point, so we should only be blocking at extremely critical issues. We could certainly consider this for approval for 1.3, however.

Comment 39

[Dave Hylands [:dhylands]]

That was my thinking as well. I'll change it to 1.3?

Comment 40

[Jason Smith [:jsmith]]

(In reply to Dave Hylands [:dhylands] from comment #39)
> That was my thinking as well. I'll change it to 1.3?

I was actually meaning approval, not blocking however. Can you ask for approval for 1.3 here?

Comment 41

[Dave Hylands [:dhylands]]

Comment on attachment 8347555 [details] [diff] [review]
Don't disable adb if a USB function is currently active. v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 810092
User impact if declined: USB sessions (USB Mass Storage, USB Tethering, adb sessions, and remote debugging sessions) get disconnected on production devices when the screen gets locked, or the 12 hour timeout expires.
Testing completed (on m-c, etc.): Already landed on m-c. Seems to be working fine.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 42

[Dave Hylands [:dhylands]]

Removed leo from the title since this impacts all of the phones we're supporting.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/284895507d2d

Is this wontfix for v1.2?

Comment 44

[Dave Hylands [:dhylands]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #43)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/284895507d2d
> 
> Is this wontfix for v1.2?

I believe so.

Comment 45

[vampirefo]

I Have this problem on my ZTE OPEN C, how to fix?

Comment 46

[Dave Hylands [:dhylands]]

(In reply to vampirefo from comment #45)
> I Have this problem on my ZTE OPEN C, how to fix?

Which version of software are you running?

If you're running 1.2, then the only fix I'm aware of is to disable the lock screen.

Looking at the patch from https://hg.mozilla.org/releases/mozilla-aurora/rev/284895507d2d it appears to be pure JS, so if you've got root access you can probably patch your phone.

You would need to pull /system/b2g/omni.ja, unpack it (it's a zip file), and apply the patch to the chrome/chome/content/settings.js file, repack omni.ja and push it to the phone.

Comment 47

[vampirefo]

I am using 1.3, I am new to firefox and don't know to do this, but will give it a try thanks.

Comment 48

[Dave Hylands [:dhylands]]

If you(In reply to vampirefo from comment #47)
> I am using 1.3, I am new to firefox and don't know to do this, but will give
> it a try thanks.

This was fixed in 1.3, so you shouldn't need to do anything.

Comment 49

[vampirefo]

LOL, if it was fixed, I wouldn't have the problem, so it's not fixed in my version of 1.3

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I have to add that I also have seen this yesterday with latest 1.4. I was using internet sharing via USB and the connection dropped when the screen turned off. Looks like we need a follow-up bug here.

Comment 51

[vampirefo]

I was told yesterday this isn't a bug but a security feature added to the OS, I didn't like that answer, as this is an inconvenience, not a security issue, some developer wants to play big brother, was told to install another OS to fix problem, then finally one person told me to disable lock screen, this fixed it. Now if firefox would stop playing big brother and just develop the OS and stop adding fake security features to the phones, firefox would be a great OS.

firefox has decided to go the way windows did, and start crippling the OS for no reason um (security), that's why I stopped using windows back in 2003 the fake security bs, hopefully firefox devs, will revers this in next build or simply add a override or disable feature to this useless feature.

https://groups.google.com/forum/#!topic/mozilla.dev.b2g/o7IvjPuZCMA

Comment 52

[Dave Hylands [:dhylands]]

(In reply to Henrik Skupin (:whimboo) from comment #50)
> I have to add that I also have seen this yesterday with latest 1.4. I was
> using internet sharing via USB and the connection dropped when the screen
> turned off. Looks like we need a follow-up bug here.

I think that we should open a separate bug for internet sharing.

This code:
http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#364
will need to have "usbFuncActive" updated to include "internet sharing".

Isn't "internet sharing" the same thing as "rndis".

Comment 53

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I filed bug 1025791 for the USB tethering case.