1179691 - [Flame][Settings]During formatting internal storage, toggle on USB as UMS mode, then "Media storage" and "USB Storage" options will turn grey and can't work normally.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: MTP/UMS, defect)

Description

[helen（leave from Mozilla）]

Attachment: VIDEO0619.3gp

[1.Description]:
[Flame v2.2&master]Settings]During formatting internal storage, connect device to PC with USM mode, then media storage and USB storage options will not be available, moreover, charging icon and camera works abnormally.
See attachment: logcat.txt, VIDEO0619.3gp
Found at: 23:38

[2.Testing Steps]: 
1. Launch Settings;
2. Toggle on USB Storage;
3. Tap "Media storage" 
4. Tap "Format internal storage" button;
5. Then quickly insert USB cable to connect to PC;
6. After a while, disconnect USB cable, observe device

[3.Expected Result]: 
6. Device works normally

[4.Actual Result]: 
6.1 USB storage options can't work normally;
6.2 Media storage will be always grey and not available;
6.3 Charging icon will be always shown on status bar
6.4 Camera can't be launched since it prompts device is plugged in.

[5.Reproduction build]: 
Flame 2.2 version(Affected):
Build ID               20150701002506
Gaia Revision          bd386f346eb1591fddbc84bf034b22700e7e2a58
Gaia Date              2015-06-30 15:53:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f16c1125b9d6
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150701.035459
Firmware Date          Wed Jul  1 03:55:10 EDT 2015
Bootloader             L1TC000118D0

Device: Flame master(Flame 2.5/master)(Affected)
Build ID               20150701160205
Gaia Revision          858764a56982eb558259ccc689bbee855f090085
Gaia Date              2015-07-01 16:59:48
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/c36f68439496
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150701.193443
Firmware Date          Wed Jul  1 19:34:55 EDT 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

[8. Note]:
Nexus 5 does not exist this bug, because there is no "Format internal storage" function in "Media storage".

Comment 1

[helen（leave from Mozilla）]

Attachment: logcat.txt

Comment 2

[Hermes Cheng (inactive after July 27, 2015)]

Alison, I think this issue is really critical. It results in user cannot use storage totally until rebooting the device. I guess it results from async handling between formatting and storage access request from PC.

Comment 3

[Alison Shiue]

Hi Alphan, could you please help to check this issue? The storage status seems incorrect in this use case.

Comment 4

[Alphan Chen [:alchen]]

Update conclusion first.
From the log of vold, it looks normal.
Device experience "format internal storage" -> "Enable UMS mode" -> "Disable UMS mode" -> "Enable UMS mode".
We also need the feedback from gaia team.
I will also try to reproduce the symptom in the same time.



From log, the case is like below.
23:36:00.860 vold begin to format the internal storage 
            (Mounted->Unmounting->Idle->Formatting->Checking->Mounted)

23:36:06.610 vold begin to share internal storage (Enable UMS mode)
            (Mounted->Unmounting->Idle->Share)

23:36:18.840 vold got mount request (Disable UMS mode)
            (Share->Idle->Checking->Mounted)

23:37:29.500 vold begin to share internal storage (Enable UMS mode)
            (Mounted->Unmounting->Idle->Share)

Comment 5

[Alphan Chen [:alchen]]

I found the root cause after reproducing the symptom by myself.
The symptom is happened when there are format and share request in the same time.
Automounter will do share request first in current codebase.
However, format request cannot be accomplished.
In this case, we will stuck in current situation. (keep this volume in sharing status)

Comment 6

[Alphan Chen [:alchen]]

Attachment: (0703) Bug 1179691 - [Automounter] Fix the problem of get format and share request in the same time.

Hi Dave,
could you take a look about this patch?
Thanks.

Comment 7

[Dave Hylands [:dhylands]]

Comment on attachment 8629238 [details] [diff] [review]
(0703) Bug 1179691 - [Automounter] Fix the problem of get format and share request in the same time.

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

I'd like to find out some more explanation about why the PostDelayedTask of UpdateState is required?

The rest of the patch looks good.

::: dom/system/gonk/AutoMounter.cpp
@@ +977,5 @@
> +        // Trigger UpdateState again to see if there is other unfinished request .
> +        MessageLoopForIO::current()->
> +          PostDelayedTask(FROM_HERE,
> +                          NewRunnableMethod(this, &AutoMounter::UpdateState),
> +                          delay);

It's not clear to me why this is required.

The code will fall through the if and then try to do the mount/format/whatever. THat should then initiate state changes, issue commands, etc. We shouldn't need to artificially call UpdateState

Comment 8

[Alphan Chen [:alchen]]

(In reply to Dave Hylands [:dhylands] from comment #7)
> Comment on attachment 8629238 [details] [diff] [review]
> (0703) Bug 1179691 - [Automounter] Fix the problem of get format and share
> request in the same time.
> 
> Review of attachment 8629238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to find out some more explanation about why the PostDelayedTask of
> UpdateState is required?
> 
> The rest of the patch looks good.
> 
> ::: dom/system/gonk/AutoMounter.cpp
> @@ +977,5 @@
> > +        // Trigger UpdateState again to see if there is other unfinished request .
> > +        MessageLoopForIO::current()->
> > +          PostDelayedTask(FROM_HERE,
> > +                          NewRunnableMethod(this, &AutoMounter::UpdateState),
> > +                          delay);
> 
> It's not clear to me why this is required.
> 
> The code will fall through the if and then try to do the
> mount/format/whatever. THat should then initiate state changes, issue
> commands, etc. We shouldn't need to artificially call UpdateState

Got it.
The reason why I added PostDelayedTask here is after formatting device don't share the storage until I try to toggle the USB Storage. I will try to find a better way to solve this problem.

Comment 9

[Dave Hylands [:dhylands]]

(In reply to Alphan Chen [:alchen] from comment #8)
> (In reply to Dave Hylands [:dhylands] from comment #7)
> > Comment on attachment 8629238 [details] [diff] [review]
> > (0703) Bug 1179691 - [Automounter] Fix the problem of get format and share
> > request in the same time.
> > 
> > Review of attachment 8629238 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'd like to find out some more explanation about why the PostDelayedTask of
> > UpdateState is required?
> > 
> > The rest of the patch looks good.
> > 
> > ::: dom/system/gonk/AutoMounter.cpp
> > @@ +977,5 @@
> > > +        // Trigger UpdateState again to see if there is other unfinished request .
> > > +        MessageLoopForIO::current()->
> > > +          PostDelayedTask(FROM_HERE,
> > > +                          NewRunnableMethod(this, &AutoMounter::UpdateState),
> > > +                          delay);
> > 
> > It's not clear to me why this is required.
> > 
> > The code will fall through the if and then try to do the
> > mount/format/whatever. THat should then initiate state changes, issue
> > commands, etc. We shouldn't need to artificially call UpdateState
> 
> Got it.
> The reason why I added PostDelayedTask here is after formatting device don't
> share the storage until I try to toggle the USB Storage. I will try to find
> a better way to solve this problem.

What are the state transitions when formatting? Doesn't the state eventually return to IDLE when formatting is done?

I think what may be happening is that when we're formatting, the volume goes to IDLE and then this code: https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/AutoMounter.cpp?from=AutoMounter.cpp#1054-1061
changes it to mounting. It should probably check if sharing is also requested and initiate the sharing.

Comment 10

[Josh Cheng [:josh]]

[Blocking Requested - why for this release]:
2.2 CCed. Continue investigate in next release.

Comment 11

[Alphan Chen [:alchen]]

Attachment: (0707) [Automounter] Fix the problem of get format and share request in the same time.

Hi Dave, you are right.
I rewrite the patch as attachment.
Please review it.
Thanks.

Comment 12

[Dave Hylands [:dhylands]]

Comment on attachment 8630359 [details] [diff] [review]
(0707) [Automounter] Fix the problem of get format and share request in the same time.

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

Looks good.

::: dom/system/gonk/AutoMounter.cpp
@@ +1055,5 @@
>          case nsIVolume::STATE_MOUNT_FAIL: {
>            LOG("UpdateState: Volume %s is %s", vol->NameStr(), vol->StateStr());
>            if (vol->IsFormatting() && !vol->IsFormatRequested()) {
>              vol->SetFormatRequested(false);
> +            if (!tryToShare || !vol->IsSharingEnabled()) {

This should also check that the state is IDLE. We don't want to try mounting if we're in the mount failed state.

Actually, it might be clearer to split apart IDLE and MOUNT_FAIL and the only thing that we look at in the  MOUNT_FAIL state is whether a format has been requested.

@@ +1064,5 @@
>            }
> +
> +          // If there are format and share requests in the same time,
> +          // we should do format first then share.
> +          if (vol->IsFormatRequested()){

nit: space before open curly brace

Comment 13

[Alphan Chen [:alchen]]

Attachment: [Automounter] Fix the problem of get format and share request in the same time. r=dhylands

Here is try server result.
It looks nice.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=187f5906f626

Comment 14

[Pulsebot]

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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 16

[helen（leave from Mozilla）]

This bug has been verified as pass on latest build of Flame master by the STR in Comment 0. 

Results:
After disconnecting USB cable,device works normally.

Reproduce rate: 0/10

Device: Flame master(Verified)
Build ID               20150715160204
Gaia Revision          b9968cdc4a1dee49848fed6159a59c378cea062d
Gaia Date              2015-07-15 12:13:34
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/49683d4e9ebd
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150715.192231
Firmware Date          Wed Jul 15 19:22:42 EDT 2015
Bootloader             L1TC000118D0