1091544 - Replace the way SD card handling in Shared/js/mediadb.js for Music/Photo/Video with implemented in system/js/external_storage_monitor.js for system app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Music, defect)

Description

[howie [:howie]]

+++ This bug was initially created as a clone of Bug #1087253 +++

[1.Description]:
"Music can not be used while phone is plugged in" view is displayed after disable usb storage and launch music
Occurence time:3:29pm

[2.Testing Steps]: 
1.Plug in phone with pc while usb storage enabled.
2.Unplug phone.
3.Launch music.

[3.Expected Result]: 
The music can be launched

[4.Actual Result]: 
It still prompt"Music can not be used while phone is plugged in"

[5.Reproduction build]: 
Gaia        :a273ab9c18e9184eb02722b25c73e2ba7680cc09
Gecko     :e7df4dde2d9dbedee942333d34eaea2afe32bebc
BuildID    :20141017100433
Version    :32.0

[6.Reproduction Frequency]: 
Always Recurrence

Comment 2

[David Flanagan [:djf]]

Josh: In https://bugzilla.mozilla.org/show_bug.cgi?id=1087253#c13 you make it sound like the cause of this bug is well understood and that MediaDB just needs to upgrade to use a new API. But I don't actually see an explanation of the bug anywhere. Can you explain what is failing with the current MediaDB implementation?

Dave: The comment linked above says that there is a new device storage api for querying the availability of storage areas. Can you tell me what has changed?

Comment 3

[Josh Cheng [:josh]]

Hi Wayne,
Can you help to provide more information as we discussed before? Thanks!

Comment 4

[Alphan Chen [:alchen]]

Hi David,
in bug 1087253, the bug is happened when inserting an unsupported sdcard.
In the same time, there are some songs in the internal storage also.

There are two points in the following comment.
https://bugzilla.mozilla.org/show_bug.cgi?id=1087253#c12

1. There should be some error handle info to prompt user that they are using an unsupported SD.
=> "Insert an unsupported SD" can be recognized by the new event called "storage-state-change" (See [1] for more detail) So the new API should be related to this.

2. When user inserts one unsupported SD, mount the SD on PC, and unmount, the internal storage should be released from PC, and Music app can recognize songs on internal storage. So launching music should be successfull, instead of showing that "Music can not be used while phone is plugged in".
=> Since there are two different volumes, music still can access internal storage in this case.
   The message will confuse user.




[1]
In the past, gaia monitor "change" event for volume state change.
The events only include 'available', 'unavailable', 'shared'.
Besides, there is API called "storageStatus()" which will return the current state.('available', 'unavailable', 'shared'.)

In Bug 1029403, gecko expose a "storage-state-change" event when there is volume state change.
These states include 'NoMedia', 'Idle', 'Pending', 'Checking', 'Mounted', 'Unmounting', 'Formating', 'Shared', and 'ShareMnt'.
We can use this state to identify what volume status is.
System APP now use this event to maintain the truly volume state.
If needed, Gecko can also add an API for get the status state which include all states. ('NoMedia', 'Idle', 'Pending', 'Checking', 'Mounted', 'Unmounting', 'Formating', 'Shared', and 'ShareMnt')

Comment 5

[Dave Hylands [:dhylands]]

So if I'm interpreting things correctly here, once you've unmounted, then the internal storage is available, but the external storage is unavailable?

And that's causing the problem?

Can you include a logcat? I'd like to see what the actual volume states for both cards are.

Comment 6

[Alphan Chen [:alchen]]

Here is the logcat.
https://bugzilla.mozilla.org/attachment.cgi?id=8509385

You can find more detail in bug 1087253.

Comment 7

[Josh Cheng [:josh]]

Per conclusion of triage meeting with Keven, Shawn, Evelyn, Wesly and Ken on 12/10. This should be 2.2+

Comment 8

[Josh Cheng [:josh]]

Dear Dave, 
Please check the log cat per comment 6. Thank you!

Comment 9

[Dave Hylands [:dhylands]]

I was able to reproduce the problem.

I formatted an sdcard using ext4 and stuck in my flame. Prior to enabling USB Mass Storage, everything seems to be working fine.

After enabling and disabling USB Mass Storage, then I saw the problem.

DS Test showed this important clue:

> 17:51:00.819 1933/1933                 GeckoDump  I   Available status for type 'sdcard'
> 17:51:00.819 1933/1933                 GeckoDump  I   Processing sdcard
> 17:51:00.849 1933/1933                 GeckoDump  I     [sdcard] = available
> 17:51:00.849 1933/1933                 GeckoDump  I   Processing sdcard1
> 17:51:00.889 1933/1933                 GeckoDump  I     [sdcard1] = shared

so the error is that the AutoMounter is showing sdcard1 as shared even though it isn't shared any more.

So when the mount fails, we probably need to create a new state (instead of IDLE). This will then cause the volume to be unavailable (instead of shared) and will also cause the AutoMounter to stop trying to re-mount the volume.

Probably something like STATE_MOUNT_FAILED with a value of 101.

Comment 10

[Josh Cheng [:josh]]

Modify marking from blocking-b2g to feature-b2g 2.2+. Still in scope of 2.2.

Comment 11

[Hema Koka [:hema]]

Alphan,

Is this something you can help fix? Dave is on another committed feature for 2.2. But can help with any questions or guidance around it. 

Thanks
Hema

Comment 12

[Dave Hylands [:dhylands]]

I put myself down as the assignee for now. If alphan can work on it, please feel free to change.

Comment 13

[Alphan Chen [:alchen]]

OK, I will take this bug.

Comment 14

[Josh Cheng [:josh]]

Thank you Alphan,
Is the ETA 1/23 okay for you?

Comment 15

[Alphan Chen [:alchen]]

Hi Josh,
We will fix the problem which Dave mentioned in comment 9.

After the fix, we will get correct state for internal storage and sdcard.
internal storage : available
sdcard1          : unavailable   (insert an un-supported sdcard)

However, we still need the confirm from gaia: 
  Can gaia handle the condition like this (two volumes: one available and another one unavailable)?

Comment 16

[Josh Cheng [:josh]]

Hi David,
Can you help to confirm whether gaia can handle the new way after Alphan provide fix? Thank you!

Comment 17

[Hubert Lu[:hlu] <[e-mail]>]

Hi Alison,
    Please check if this cases should be added in moztrap. Thank you.

Comment 18

[Eden Chuang[:edenchuang]]

Attachment: bug1091544_set_the_volume_as_STATE_MOUNT_FAIL_while_mount_the_volume_failed

Creating a new volume state 'STATE_MOUNT_FAIL' for mounting failed case.

One question, should we share the volume that cannot available on the device?
I feel strange about this volume sharing. The connected PC might access the volume through the device, but the device cannot access the volume.

Comment 19

[Eden Chuang[:edenchuang]]

Attachment: bug1091544_set_the_volume_as_STATE_MOUNT_FAIL_while_mount_the_volume_failed

Creating a new volume state 'STATE_MOUNT_FAIL' for mounting failed case.

One question, should we share the volume that cannot available on the device?
I feel strange about this volume sharing. The connected PC might access the volume through the device, but the device cannot access the volume.

Comment 20

[Eden Chuang[:edenchuang]]

Attachment: bug1091544_set_the_volume_as_STATE_MOUNT_FAIL_while_mount_the_volume_failed

Creating a new volume state 'STATE_MOUNT_FAIL' for mounting failed case.

One question, should we share the volume that cannot available on the device?
I feel strange about this volume sharing. The connected PC might access the volume through the device, but the device cannot access the volume.

Comment 21

[David Flanagan [:djf]]

(In reply to Alphan Chen[:Alphan] from comment #15)

> After the fix, we will get correct state for internal storage and sdcard.
> internal storage : available
> sdcard1          : unavailable   (insert an un-supported sdcard)
> 
> However, we still need the confirm from gaia: 
>   Can gaia handle the condition like this (two volumes: one available and
> another one unavailable)?

I think this is exactly the same state gaia would see if there was no sd card inserted at all, right? So if the user inserts an unsupported SD card, gaia shouldn't even know that anything has happened.

The only relevant code that I know about is in shared/js/mediadb.js, which is used by Gallery, Music and Video. See the getState() function at line 725 in particular. And note that MediaDB (and therefore the media apps that use it) behave as if all volumes are shared even if only one volume is shared.  But in this case if we just have the external sdcard in an unavailable state and nothing is shared, it should all be okay.

Comment 22

[Alison Shiue]

https://moztrap.mozilla.org/manage/cases/?filter-id=15246

Comment 23

[Dave Hylands [:dhylands]]

Comment on attachment 8542867 [details] [diff] [review]
bug1091544_set_the_volume_as_STATE_MOUNT_FAIL_while_mount_the_volume_failed

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

Excellent.

I'm going to give this an r+, so you don't have to give it back to me review again.

::: dom/system/gonk/AutoMounter.cpp
@@ +1061,5 @@
>          }
> +        case nsIVolume::STATE_IDLE:
> +        case nsIVolume::STATE_MOUNT_FAIL: {
> +          LOG("UpdateState: Volume %s is %s", vol->NameStr(),
> +               volState == nsIVolume::STATE_IDLE ? "nsIVolume::STATE_IDLE":"nsIVolume::STATE_MOUNT_FAIL");

nit: Rather than do this, just call vol->StateStr() and use that instead.

@@ +1069,5 @@
>              vol->StartMount(mResponseCallback);
>              break;
>            }
>            if (tryToShare && vol->IsSharingEnabled()) {
>              // Volume is unmounted. We can go ahead and share.

We definitely want to allow formatting of the card, but I don't think we should try to share.

So I would be inclined to only initiate sharing if the volume is in the IDLE state.

::: dom/system/gonk/Volume.cpp
@@ +543,5 @@
>              // then the AutoMounter will set the volume as STATE_MOUNTED.
>              SetState(nsIVolume::STATE_CHECKMNT);
>            } else {
> +            if (State() == nsIVolume::STATE_CHECKING && newState == nsIVolume::STATE_IDLE) {
> +              LOG("Mount volume %s failed, set it as STATE_MOUNT_FAIL", NameStr());

nit: Let's reword this to just be:

"Mount of volume '%s' failed"

When you call SetState, it will log the fact that it's being changed to STATE_MOUNT_FAIL.

::: dom/system/gonk/nsIVolume.idl
@@ +19,5 @@
>    const long STATE_FORMATTING  = 6;
>    const long STATE_SHARED      = 7;
>    const long STATE_SHAREDMNT   = 8;
>    const long STATE_CHECKMNT    = 100;
> +  const long STATE_MOUNT_FAIL  = 101;

You should also modify nsVolume.cpp and add this state into the NS_VolumeStateStr function.

Perhaps add a comment here that says:

// Note: Changes made to the STATE_xxx names should also be reflected in the
//       NS_VolumeStateStr function found in Volume.cpp

Comment 24

[Eden Chuang[:edenchuang]]

Attachment: bug1091544_set_the_volume_as_STATE_MOUNT_FAIL_while_mount_the_volume_failed_v2. r=dhylands

Improved patch with suggestions in comment 23.

Comment 25

[Eden Chuang[:edenchuang]]

Comment on attachment 8547398 [details] [diff] [review]
bug1091544_set_the_volume_as_STATE_MOUNT_FAIL_while_mount_the_volume_failed_v2. r=dhylands

Approval Request Comment
[Feature/regressing bug #]: Set volume with state STATE_MOUNT_FAIL when fail to mount the volume.
[User impact if declined]: Music/Photo/Video apps cannot work normally and get message "can not beused while phone is plugged in" even the phone is unplugged.  
[Describe test coverage new/current, TBPL]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b5f9578af1e
[Risks and why]: Low risk (new state will not affect other behavior)
[String/UUID change made/needed]: none

Comment 26

[Ben Tian (inactive)]

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

Comment 27

[Carsten Book [:Tomcat]]

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