1022804 - [B2G][Settings][Download Manager] When deleting the ringtone and restarting the phone, the music still appears in the download manger

Status: RESOLVED FIXED

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

Description

[pete siphantong [:Petes]]

Attachment: rt.txt

Description:
When the user sets a ringtone in download manger and restarts the phone, the music's name/tittle still appears in the download manger 


Repro Steps:
1) Update a Flame to 20140609040203
2) Download music from the Browser
3) Go to Settings > Downloads 
4) Select the music, Set as ringtone > Save
5) Restart the phone
6) Go to Settings > Downloads 


Actual:
After deleting the music and restarting the phone, the music's name still appears


Expected:
After deleting the music and restarting the phone, the music's name should not appear 

The issue occurs on the following devices:

Environmental Variables:
Device: Flame 2.0
Build ID: 20140609040203
Gaia: 12af93123c5db55212d51fe235d39f21209a1eaa
Gecko: 9305a8ec77fe
Version: 32.0a1 (2.0) 
Firmware Version: v10G-2

Flame 1.4
Build ID: 20140606000204
Gecko: https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/960c48b5a5fa
Gaia: 183efb374be1bcbf92f1fb3d0212a68b564a6d3e
Platform Version: 30.0
Firmware Version: V10g-2


The issue does not occur on the following devices:

Buri 2.0
Build ID: 20140606043015
Gecko: https://hg.mozilla.org/mozilla-central/rev/c8288d0c7a15
Gaia: 857129928b6e56a809cee9d5445effb8fa9f1c2c
Platform Version: 32.0a1
Firmware Version: V1.2-device.cfg


User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0


Notes: When the user opens the music file, the file does not exist 


Repro frequency: 100%
See attached: video clip, logcat,  https://www.youtube.com/watch?v=_jOu7oAHmdM

Comment 1

[Jim Porter (:squib)]

It's not clear to me where the deletion should be taking place?

This seems more like a download manager bug anyway, unless there's something else happening that I'm not aware of...

Comment 2

[Jason Smith [:jsmith]]

The STR looks incorrect here. There's no mention of deleting of ringtones in the STR. Can you rewrite the STR here?

Comment 3

[pete siphantong [:Petes]]

Updated Repro Steps:
1) Update a Flame to 20140609040203
2) Download music from the Browser
3) Go to Settings > Downloads 
4) Select the music, Set as ringtone > Save
5) Delete the music
5) Restart the phone
6) Go to Settings > Downloads

Comment 4

[Jason Smith [:jsmith]]

(In reply to Jim Porter (:squib) from comment #1)
> It's not clear to me where the deletion should be taking place?
> 
> This seems more like a download manager bug anyway, unless there's something
> else happening that I'm not aware of...

Right - this sounds more like a download manager bug.

Comment 5

[Jason Smith [:jsmith]]

Aus - The behavior here seems weird, so I'd like a second opinion on what's going on here.

So we download a music file. I set it as my main ringtone. I restart the phone and the music file isn't deleted.

What I think might be happening here is that the deletion is failing to occur in the underlying API because the file is actively in use by another app. The settings app however sends the user perception that the file was deleted, when in reality, it wasn't deleted at all.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #5)
> So we download a music file. I set it as my main ringtone. I restart the
> phone and the music file isn't deleted.

That should say "So we download a music file. I set it as my ringtone. I delete it. I restart my phone. I notice the file wasn't deleted."

Comment 7

[Aus Lacroix [:aus]]

:jsmith, Sounds like we're not reporting the failure to the user. If the file is indeed not deleted because it's in use, it is something we should be able to report. This sounds like a valid bug to me.

However, we would have a hard time telling the user exactly why the file is in use. Which in this case, is because they set it as their ringtone.

:djf, what happens exactly when a user sets an audio file as their ringtone? Do my comments above reflect reality?

Comment 8

[Jim Porter (:squib)]

The particulars of Blobs are a bit of a mystery to me, but once the ringtones app receives the activity, we take the Blob and then eventually save it here: <https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/share.js#L207> which calls this: <https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/custom_ringtones.js#L218-L252>.

Comment 9

[David Flanagan [:djf]]

Ringtone management in 2.0 and later is completely different than in 1.4. Jim knows the 2.0 code. It looks like he's storing the file into an IndexedDB. This would make a private copy if it, so this should not be holding on to the original file.  At this point, the ringtone app that handles the "share as ringtone" activity has exited, so even if it had open file handles, they would have been released.

In 1.4 when you share a ringtone, the blob just gets copied into the settings db. Again, no handle to the original file is retained.  

Note that the original description of the bug says "When the user opens the music file, the file does not exist". That says to me that the file is actually being deleted. But the Download manager is not correctly recording the fact that it is deleted and somehow thinks it still exists.

Comment 10

[Jason Smith [:jsmith]]

The STR is still not clear. Where did you delete the ringtone?

Comment 11

[pete siphantong [:Petes]]

I deleted the ringtone/music in the download list. Updated video, https://www.youtube.com/watch?v=FXdXN94x_h0

Updated Repro Steps:
2) Have a mp3 downloaded on the phone
3) Go to Settings > Downloads 
4) Select the music, Set as ringtone > Save
5) Tap edit > select the mp3 > delete 
5) Restart the phone
6) Go to Settings > Downloads

the mp3 name/tittle still remains. When the user opens the mp3, the file does not exist

Comment 12

[Preeti Raghunath(:Preeti)]

This is a good one but can live with in 1.4. Nom'ing for 2.0 as ringtone is a feature in that release.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #12)
> This is a good one but can live with in 1.4. Nom'ing for 2.0 as ringtone is
> a feature in that release.

This is a critical feature for 1.4 & with a broken use case here, so I disagree.

Comment 14

[Preeti Raghunath(:Preeti)]

(In reply to Jason Smith [:jsmith] from comment #13)
> (In reply to Preeti Raghunath(:Preeti) from comment #12)
> > This is a good one but can live with in 1.4. Nom'ing for 2.0 as ringtone is
> > a feature in that release.
> 
> This is a critical feature for 1.4 & with a broken use case here, so I
> disagree.

I agree with this being a lower set of features in 1.4 and will now block on it.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Ping, this need some help from your team.

Comment 16

[Jason Smith [:jsmith]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #15)
> Ping, this need some help from your team.

Gregor is on PTO, so I'm redirecting to Candice.

Comment 17

[Candice Serran (:cserran)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #15)
> Ping, this need some help from your team.

I will raise this in our standup and get someone on it...thanks

Comment 18

[Candice Serran (:cserran)]

Aus,

Can you take a look at this please?

Comment 19

[Aus Lacroix [:aus]]

There's a race condition between the Downloads API initializing and the DownloadManager in Gaia asking it to clear all completed downloads. In some instances, notifications of completed downloads would still make it through if the user did not click on the notification from the toaster or notification tray.

I've fixed this by clearing the downloads api reference to the download when we take ownership by adding it to the gaia download store.

https://github.com/mozilla-b2g/gaia/pull/21069

Comment 20

[Aus Lacroix [:aus]]

Attachment: Pull Request - Clear download from Downloads API when Download Store takes ownership.

Comment 21

[Aus Lacroix [:aus]]

I would like to land this on 2.0 and 1.4. The issue happens on 2.1 as well, but it's harder to reproduce on the Flame because it's a higher performance device.

Comment 22

[Jason Smith [:jsmith]]

Aus - What's the reason for moving this to a 2.0 nom? Was that accidental or intentional?

Comment 23

[Aus Lacroix [:aus]]

I wasn't sure if I needed to ask for nom for 2.0 if it had 1.4+ already. This fix should land in 2.0 as well.

Comment 24

[Jason Smith [:jsmith]]

(In reply to Ghislain Aus Lacroix [:aus] from comment #23)
> I wasn't sure if I needed to ask for nom for 2.0 if it had 1.4+ already.
> This fix should land in 2.0 as well.

I'd leave this as a 1.4+ bug & ask for 2.0 approval after this lands.

Comment 25

[Aus Lacroix [:aus]]

(In reply to Ghislain Aus Lacroix [:aus] from comment #23)
> I wasn't sure if I needed to ask for nom for 2.0 if it had 1.4+ already.
> This fix should land in 2.0 as well.

I also feel like it should be treated with the same severity as it is for 1.4. :)

Comment 26

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8446708 [details] [review]
Pull Request - Clear download from Downloads API when Download Store takes ownership.

ok, thanks a lot, LGTM, r+. Good job as usual

Comment 27

[Lucas Adamski [:ladamski]]

Per conversation with Lawrence and Bhavana we should not be requiring approvals for 1.4 to 2.0 uplift *before* FC.

Comment 28

[Aus Lacroix [:aus]]

Commit (master): https://github.com/mozilla-b2g/gaia/commit/ddfea87310513b5499200175c752ad14cf044bf7

Pull-Request (v2.0): https://github.com/mozilla-b2g/gaia/pull/21117 (for sheriff to merge)

Pull-Request (v1.4): https://github.com/mozilla-b2g/gaia/pull/21119 (for sheriff to merge)

Comment 29

[Aus Lacroix [:aus]]

Commit (v1.4): https://github.com/mozilla-b2g/gaia/commit/aa896d5db1b4929f3bf31a0f4bb7de50530222a8

Commit (v2.0): https://github.com/mozilla-b2g/gaia/commit/8e7c67013ecbfb594a7903e3633c4c8d150ce255

Fixed!

Comment 30

[Lancy(Leave from Mozilla)]

Attachment: Verify_Video_Flame.MP4

This issue has been verified successfully on Flame 2.0 & 2.1.
See attachment: Verify_Video_Flame.MP4
Reproducing rate: 0/5

Flame v2.0 version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141130000204
Version         32.0

Flame v2.1 version:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141130001203
Version         34.0