[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.

VERIFIED FIXED

Status

--
major
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: hedan, Assigned: alchen)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.5?, firefox42 fixed, b2g-v2.2 affected, b2g-master verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8628702 [details]
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".
(Reporter)

Comment 1

3 years ago
Created attachment 8628703 [details]
logcat.txt
(Reporter)

Updated

3 years ago
status-b2g-v2.2: --- → affected
status-b2g-v2.5: --- → affected
status-b2g-master: --- → affected
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.
Flags: needinfo?(ashiue)

Comment 3

3 years ago
Hi Alphan, could you please help to check this issue? The storage status seems incorrect in this use case.
blocking-b2g: --- → 2.2?
QA Whiteboard: [COM=MTP/UMS]
Flags: needinfo?(ashiue) → needinfo?(alchen)
(Assignee)

Comment 4

3 years ago
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)
Flags: needinfo?(alchen)

Updated

3 years ago
status-b2g-v2.5: affected → ---
(Assignee)

Comment 5

3 years ago
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)
(Assignee)

Comment 6

3 years ago
Created attachment 8629238 [details] [diff] [review]
(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.
Attachment #8629238 - Flags: review?(dhylands)
(Assignee)

Updated

3 years ago
Attachment #8629238 - Attachment description: (0703) [Automounter] Fix the problem of get format and share request in the same time. → (0703) Bug 1179691 - [Automounter] Fix the problem of get format and share request in the same time.
(Assignee)

Updated

3 years ago
Assignee: nobody → alchen
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
Attachment #8629238 - Flags: review?(dhylands)
(Assignee)

Comment 8

3 years ago
(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.
(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

3 years ago
[Blocking Requested - why for this release]:
2.2 CCed. Continue investigate in next release.
blocking-b2g: 2.2? → 2.5?
(Assignee)

Comment 11

3 years ago
Created attachment 8630359 [details] [diff] [review]
(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.
Attachment #8629238 - Attachment is obsolete: true
Attachment #8630359 - Flags: review?(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
Attachment #8630359 - Flags: review?(dhylands) → review+
(Assignee)

Comment 13

3 years ago
Created attachment 8633270 [details] [diff] [review]
[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
Attachment #8630359 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Component: Gaia::Settings → MTP/UMS
https://hg.mozilla.org/mozilla-central/rev/a8de926c8ea5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
(Reporter)

Comment 16

3 years ago
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
QA Whiteboard: [COM=MTP/UMS] → [COM=MTP/UMS][MGSEI-Triage+]
status-b2g-master: affected → verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.