870327 - If sd card is ejected suddenly during playing music, MediaServer is died.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P2)

Description

[leo.bugzilla.gecko]

[VER.]
BuildID revision="7cb46499e0b91ca20f6aed58d6067d7c451875b9"
gaia revision="5cbb19e4bb78a7ad879fbe4b9a841e1c35714f5c"
gecko revision="950b402b6188bb2f3ce3176e620ed5249719d720"

[Precondition]
Eject sdcard during playing music.

[Symptoms] 
1. media server is died (100%).
2. volume set is not updated. (somtimes) [Bug 868932 - All channel volume is not updated.]
3. If headset is pluged, It`s output from speaker. [leo mofied] (somtimes)

When sdcard is eject, It`s processed later stop event than process of the remained buffer in audioflinger/OMX.
It should need to improve stop event from gecko to gaia.

Comment 1

[Marco Chen [:mchen]]

Hi Leo,

I don't get your point from description.
Could you give more info here? Thanks for your explanation.

Comment 2

[Wayne Chang [:wchang]]

Triage - not blocking as this is not a normal use case, although possible to occur.

Hi Marco, the report is:
1. Play some music
2. pull out the SD card while music is still playing

Hi Leo,
Can you describe the KO symptoms more clearly?

Comment 3

[leo.bugzilla.gecko]

I changed title
The main issue is media server is died. The other symptoms currently is resolved. 
Audio Codec/AudioFlinger has the shared buffer for processing PCM data to DAC(Digital analog converter)
Even though sd card is ejected suddenly during playing music, Audio Codec/AudioFlinger still has the remained pcm data in the shared buffer. After processing the stop
event, Audio Codec/AudioFlinger can stop nomally. 
But If the stop event is processed lately, the process is killed and the error occurs. 

--------------------------------------------------------------------------
Vold    : /mnt/extsecure/staging/.android_secure sucessfully unmounted
Vold    : /mnt/extsecure/extasec sucessfully unmounted
Vold    : Failed to unmount /mnt/extsecure/staging (Device or resource busy, retries 2, action 1)
Statemachine: 0x44bb4300 Decoder playing 8064 frames of data to stream for AudioData at 227317581
05-13 10:43:14.739   146   192 E ProcessKiller: Process /system/b2g/plugin-container (4060) has open file /mnt/extsecure/staging/Music/Lee Morgan `1963 - The Sidewinder
[Bebop]/Lee Morgan [The Sidewinder] 01 - The Sidewinder.mp3
ProcessKiller: Sending SIGHUP to process 4060
Gecko   : 
Gecko   : ###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv
Gecko   : 
AudioFlinger: removeNotificationClient() 0xf2c9d8, binder 0xf2c948
AudioFlinger: 153 died, releasing its sessions
-----------------------------------------------------------------

Comment 4

[Marco Chen [:mchen]]

Hi Leo,

May I make sure we are on the same page?

Pre-Condition:
  Vold will kill a specific process when it is trying to un-mount a SD Card and that specific process has acquire a file handle on SD Card.

Issue:
  If music app doesn't release file handle in SD Card in time after receiving notification from Device Storage about volume removing then it will be killed by Vold.

Is this right? And I didn't see any log shown media server is crashed here.

Comment 5

[leo.bugzilla.gecko]

Yes, you are right.
AudioFlinger`s removeNotificationClient() means audioflinger`s destructor, and mediaserver is died
Thanks.

Comment 6

[Dave Hylands [:dhylands]]

This seems to be the same problem as described in bug 871383

Comment 7

[Marco Chen [:mchen]]

Hi Dave,

I think we should fix the issue on this bug for

  1. To check music app will stop to play when receiving device storage is not available. 
  2. To check why volume state doesn't been updated to unmounting state which reported from bug description.

And leave bug 871383 for discussing the force argument used by automounter for issuing unmount command. May I know your suggestion?

Comment 8

[Dave Hylands [:dhylands]]

The mediaserver may be dying due to the music app dying. See bug 864188

Comment 9

[Sotaro Ikeda [:sotaro]]

Or maybe related to Bug 871018.

Comment 10

[Marco Chen [:mchen]]

Hi Dave & Sotaro,

The bugs you mentioned is the symptoms or result of this bug.

The root cause of this bug is
  1. Music app will listen the unavaliable from devicestorage then pause the music and release the audio.src attribute.
  2. The release of audio.src attribute should release the music file's fd from sd card.

So if everything goes well, there is nothing happened but from comment 3'log it seems music app doesn't receive devicestorage.onunavalible so it didn't stop to play. Then finally vold found music app held fd in sd card after 8 times of un-mount try then vold kill music app. And this just cause media server die.

So the strange issue here is why music app didn't receive devicestorage.onunavaiable for releasing resource.

Comment 11

[Dave Hylands [:dhylands]]

(In reply to Marco Chen [:mchen] from comment #10)
> So the strange issue here is why music app didn't receive
> devicestorage.onunavaiable for releasing resource.

So I suspect that with composite storage, the composite storage device won't go unavailable when the sdcard is ejected. If you were to enumerate navigator.getDeviceStorages('music') then the device storage volume associated with

Comment 12

[Dave Hylands [:dhylands]]

...continuing from last comment...

the external sdcard (probably one with a storage name of extsdcard) would get an onchange with unavailable.

So I suspect that we need some changes in mediadb to provide more/better/different support for composite storage.

Comment 13

[Marco Chen [:mchen]]

Hi Dave,

Yes, I agree your assumption and actually that is what bug 872800 did already.

Composite Device Behavior Now:
  If app used navigator.getDeviceStorage('music') then it will get a composite device which contains two components of sdcard & extsdcard. And only both of two components are unavailable, the composite device will just report unavailable.

Issue Now:
  If app starts to play a file located on extsdcard, then once extsdcard is ejected app doesn't receive unavailable event from composite device object (because unavailable would be fired only when both sdcard & extsdcard are all unavailable.). So app doesn't release the file resource and be killed by vold daemon.

Possible Solution:
  1. App who created composite device should use navigator.getDeviceStorages() to get an array of each storage then listen their unavailable event individually. 

   Q: This needs app to create more then one devicestorage to monitor individual status. 

Hi David, Is this ok to you on mediadb.js? Thanks.

  2. The composite device reports status for each of it's internal components (ex: sdcard, extsdcard & composite). And app needs to recognize each of them by e.path.

    Q: This seems to be more reasonable to me. (Sorry for I should discuss with Dave on bug 872800 before it is landed).

Hi Dave, may I know your comment? Thanks.

Comment 14

[Dave Hylands [:dhylands]]

So I guess either method is feasible.

Either way requires a different change in a different portion of the mediadb.

Comment 15

[Dave Hylands [:dhylands]]

And method 1 has the complication that calling available on the composite device can only return 1 status.

So if we go with solution 2, you'd still need to call available on each of the storage areas returned by calling getDeviceStorages (as per solution 1) to get the initial state.

Comment 16

[David Flanagan [:djf]]

(In reply to Marco Chen [:mchen] from comment #13)

> Possible Solution:
>   1. App who created composite device should use
> navigator.getDeviceStorages() to get an array of each storage then listen
> their unavailable event individually. 
> 
>    Q: This needs app to create more then one devicestorage to monitor
> individual status. 
> 
> Hi David, Is this ok to you on mediadb.js? Thanks.
> 

Yes, this is okay with me. I think this is what Dave and I have discussed, and I think he has filed a bug and is going to modify mediadb as described here.

>   2. The composite device reports status for each of it's internal
> components (ex: sdcard, extsdcard & composite). And app needs to recognize
> each of them by e.path.
> 
>     Q: This seems to be more reasonable to me. (Sorry for I should discuss
> with Dave on bug 872800 before it is landed).
> 

We could make this work if we had a new type of change event. If DeviceStorage fired a 'volumechanged' notification of some sort, and had a volumes property that was an array of root directories for all currently available volumes.  Then, available(), freeSpace(), etc. could also take a volume root directory as an optional argument.  With that, you wouldn't need getDeviceStorages() anymore, and all device storage objects would be implicitly composite.

I also like option 2 a little bit better.  But I think we can do option 1 with only gaia changes, and option 2 changes the API again and requires gecko changes, so I'd probably vote for option 1 at this point.

Comment 17

[Marco Chen [:mchen]]

Hi David,

Thanks for your comment here. Yes, the option 1 will be more suitable on this project stage and leave option 2 be a possible plan (Of course, this still need to discuss with Dave) on next version.

Comment 18

[David Flanagan [:djf]]

Dominic,

This is listed as a priority bug for the San Diego work week. Do you want to take it?  We can coordinate with Dave Hylands about the media db work he's doing. But even if we don't figure out how mediadb is going to be updated, the music app could register its own device storage listeners to stop playing when the SD card is ejected. 

(The video app probably has exactly the same problem and we should apply the same fix to it.)

Comment 19

[Leo]

As discussed in San Diego work week marking this as leo+.

Comment 20

[David Flanagan [:djf]]

Taking this for myself.  If Marco's diagnosis is correct, then fixing MediaDB to send notifications will fix the bug.

Comment 21

[David Flanagan [:djf]]

See also bug 875641: a similar bug for gallery where we need to get an event when the external sdcard is pulled but internal storage remains.

Comment 22

[David Flanagan [:djf]]

Now that I have a Leo device I can reproduce this: pulling the sdcard causes an instant crash in the music app.

Comment 23

[David Flanagan [:djf]]

Attachment: link to patch on github

Dave: the mediadb.js changes in this pull request are ready for your review.

Dominic: the music.js changes in this pull request listen for a new 'volumechange' event from mediadb, and stop playing music when that event is received. This prevents the crash.  But the patch is not complete: it introduces other problems that I need your help with. Please see the XXX comments in the patch.

(Perhaps I'll attach the mediadb change to the gallery bug that also needs it and land that, and then reassign this bug to you.  Would that make sense?)

Comment 24

[Dave Hylands [:dhylands]]

Comment on attachment 755217 [details]
link to patch on github

As per email discussion, I'll r- this.

I think that we should add an option to scan so that we can tell it what volumes to scan, and that we should only scan a particular volume when it becomes available.

This then addresses the problem of having two volumes going shared one right after the other. Going shared wouldn't initiate a scan, just tell the app that a volume went shared.

I think that would address the issues.

Comment 25

[David Flanagan [:djf]]

Dave,

So I've actually tried out my Leo device now and can see that Settings allows the two volumes to be shared independently... So my assumption about two shared events always coming on the heels of each other was obviously invalid.

I guess I do need to send a volumechange event when only one volume is shared. Maybe I can use a short timeout to catch the case where both are shared one after the other.  Or, maybe I can modify scan so that any current scan stops when a volumechange event is triggered.  So if two volumes are shared and we send a volumechange immediately followed by an unavailable I need that not to crash any apps.

So maybe instead of sending a volumechange event, it would be better to just act like all the files on the removed volume had been deleted?  Basically, removing a card or shareing a volume requires a very specialized scan: just to remove files that are not available anymore. We can do that quickly in mediadb.

This is hard. I just want to find a simple, minimal solution.

Comment 26

[Dave Hylands [:dhylands]]

Removing the files which aren't available on shared or unavailable (just for one volume) and initiating a full scan on available is probably a reasonable compromise.

There is still the problem that multiple volumes will become available near to the same time, but the worst case is that we do two scans (one for each available event).

Later on we can optimize the scanning to just scan a single volume that becomes available.

Comment 27

[David Flanagan [:djf]]

More thoughts about this...

I see shared and unavailable as fundamentally different states. They both mean "your files aren't availble now" but unavailable seems more likely to mean "they're gone for good".  If a user pulls an sdcard, they're likely to keep it out, or swap in another one, or add or remove many files. When a card goes unavailable, it seems reasonable to delete all files from that card from the database. If it is re-inserted a full scan will be needed to find everything.

But when a volume is shared, that seems like a much more temporary thing. A few files may be added or deleted. We'll need to rescan when the volume is available again, but we don't want to have to start over from scratch. That is: we shouldn't delete all of the files on the volume from the db because it is likely that the user will be keeping them. At the same time, though, we can't allow the user to keep using the app, because many of the files will be unavailable.

If mediadb didn't use indexed db and instead stored metadata in flat files on the same volume as the files themselves, then we could just reinitialize the app when a volume became unavailable.  With indexeddb, we could probably do something clever to retain files but make them unusable.  But both of those are major changes, not suitable for fixing Leo+ bugs.

So here's what I propose:

1) sdcard becomes unavailable: delete all db records for files on that sdcard.
There is no longer a need for a volumechange event. Just make the files disappear with a deleted event. (But send scanstart and scanend events so apps display their progress bars.)

2) sdcard becomes available: send a ready event just as we would if it was the only card

3) sdcard or internal storage shared: send unavailable event, locking the app even if just one volume is shared.

Dave: does this sound like it would work?

Comment 28

[Dave Hylands [:dhylands]]

(In reply to David Flanagan [:djf] from comment #27)
> More thoughts about this...
> 
> I see shared and unavailable as fundamentally different states. They both
> mean "your files aren't availble now" but unavailable seems more likely to
> mean "they're gone for good".  If a user pulls an sdcard, they're likely to
> keep it out, or swap in another one, or add or remove many files. When a
> card goes unavailable, it seems reasonable to delete all files from that
> card from the database. If it is re-inserted a full scan will be needed to
> find everything.
> 
> But when a volume is shared, that seems like a much more temporary thing. A
> few files may be added or deleted. We'll need to rescan when the volume is
> available again, but we don't want to have to start over from scratch. That
> is: we shouldn't delete all of the files on the volume from the db because
> it is likely that the user will be keeping them. At the same time, though,
> we can't allow the user to keep using the app, because many of the files
> will be unavailable.

Yes - that's a very good point about shared versus unavailable.

> If mediadb didn't use indexed db and instead stored metadata in flat files
> on the same volume as the files themselves, then we could just reinitialize
> the app when a volume became unavailable.  With indexeddb, we could probably
> do something clever to retain files but make them unusable.  But both of
> those are major changes, not suitable for fixing Leo+ bugs.

Even using a flat file, you still need to parse it and make up the differences between it and what's on the filesystem, since adding files to the sdcard (while its say on the PC) won't update the flat file.

As far as future options go, we could probably store something unique on the sdcard and allow up to N sdcards to be retained in the indexedDB.

I guess we could also add some type of option to store the indexed db right on the volume (which is similar to your flat-file option). I like this because then if the user sticks in a huge sdcard with a ton of images, we don't take away storage from /data.

> So here's what I propose:
> 
> 1) sdcard becomes unavailable: delete all db records for files on that
> sdcard.
> There is no longer a need for a volumechange event. Just make the files
> disappear with a deleted event. (But send scanstart and scanend events so
> apps display their progress bars.)

we still need to lock the app if there are no other available volumes (for example, with the unagi).

> 2) sdcard becomes available: send a ready event just as we would if it was
> the only card
> 
> 3) sdcard or internal storage shared: send unavailable event, locking the
> app even if just one volume is shared.
> 
> Dave: does this sound like it would work?

This also seems like a reasonable compromise for the time being.

We should file a followup bug to make the scanning be per-volume.

Comment 29

[David Flanagan [:djf]]

Comment on attachment 755217 [details]
link to patch on github

Dave,

I've updated my pull request with something much more robust, I think.  Please see what you think. I'd love to be able to land this on Friday because in addition to fixing the Music bug, it will also fix two gallery bugs and a (unfiled) video bug.

Dominic,

I've also asked for your review of the music.js changes. In addition to the new event handling code to handle sdcard removal, I've also added a few other minor bug fixes.

Comment 30

[Dave Hylands [:dhylands]]

Comment on attachment 755217 [details]
link to patch on github

There was some minor nits, and 2 issues I discovered.

The first is that 'created' is a valid file change reason (along with modified and deleted).

The second was that the storageName can be empty on non-phone devices (so desktop for example) and in that case, relative files are still returned, rather than //file

Other than that, this looks good (although since I'm still a JS newbie, I may not have picked up on a subtlty here or there)

Comment 31

[Dave Hylands [:dhylands]]

I only reviewed mediadb.js and not music.js

Comment 32

[Dominic Kuo [:dkuo]]

Comment on attachment 755217 [details]
link to patch on github

David,

This patch looks good, and I tried to use a leo device to test the scenario when sdcard is removed and Music app is playing, it doesn't crash again. But I found while Music is scanning files on external sdcard, if we try to remove the external sdcard before the scanning finishes, Music will crash. I guess MediaDB didn't release the scanning file so cause the crash, and probably we need a cancel method for scanning of MediaDB? so that we can handle the sdcard is removed before scanning is finished. And we can file another bug as a followup to fix that issue.

Comment 33

[David Flanagan [:djf]]

Thanks, Dominic!

I've just landed the already-reviewed MediaDB change as part of bug 875641, so I'll change this pull request to just land the Music app portion of the patch.

I think the crash while scanning isn't anything I can fix in MediaDB itself. Bug 876782 may fix that however.

But if the user pull the sdcard while mediadb is scanning and we don't crash, I'd expect the mediadb to be in some partially corrupt state. You wouldn't expect most software to recover gracefully when you pull out a spinning hard drive, and I think that crashing in this case might be the most graceful way to handle the really extreme situation where a user starts the app the removes the back cover of their phone and pulls out the card!

Comment 34

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/e6b05b52f421c6122a31e9167cdeeb80d227d290

Bug 875641 needs to be uplifted before this one can be uplifted.

Comment 35

[David Flanagan [:djf]]

Uplifted to v1-train: https://github.com/mozilla-b2g/gaia/commit/e7fd566656c3ad01fff04270b973f3e23d006c31

Comment 36

[Deepa Subramanian]

When the user removes the SD card while the music is playing, the music app handles it my prompting a message "Load songs on to the memory card." 

Issue seems to be fixed.

Verified on
Leo Build ID: 20130809041203
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/45480132106b
Gaia: c9d0901564cf6f50e375ab48e4124b8378a2e246
Platform Version: 18.1