1215674 - If the user closes music and quickly locks the device, the music widget will still be present

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Music Control, defect, P3)

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

If the user plays a song, goes into card view, swipes up to close the music app and then quickly locks their device, the music widget will still be present on the lockscreen as well as on the utility tray.

Repro Steps:
1) Update a Aries to 20151016122951
2) Open the music app and play a song.
3) Go into card view, swipe up to close the music app and then quickly lock the device. (Timing issue)
4) Wake the phone back up and observe the lockscreen. 
5) Unlock the device and pull down the notification tray. 

Actual:
The music widget is still present on the lockscreen and in the utility tray after force closing the music app. 


Expected:
The music widget goes away when the user closes the music app. 

Environmental Variables:
Device: Aries 2.5
Build ID: 20151016122951
Gaia: 8999f0ba6326d815c8366e3c1155b7e4e9763b40
Gecko: ccf288f658211b6cfab33c458aaf033baed2375b
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 44.0a1 (2.5)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

Repro frequency: 5/5 100%
See attached: Video, logcat

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

I am not sure if this is an NGA Music or a task manager issue specific to master.

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

This issue also occurs on the Flame 2.5

The music widget will still be present after closing out the music app.

Device: Flame 2.5 (Full Flash)(KK)(319mb)
Build ID: 20151016030225
Gaia: 8ea9029190af2ffeb04dcd97b323738125e31a0e
Gecko: d374d16cbb251c9dac5af69f8e186e821ce82fe2
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 44.0a1 (2.5)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

------------------

This issue does not occur on Flame 2.2

The music widget goes away after closing the music app.

Environmental Variables:
Device: Flame 2.2 (Full Flash)(KK)(319mb)
Build ID: 20151006032504
Gaia: 5dd95cfb9f1d6501ce0e34414596ef3dd9c2f583
Gecko: fc588eb28eab
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 3

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: MusicWidgetLogcat.txt

Comment 4

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Setting qawanted to check the Music OGA app on an engineering build to see if this reproduces there.

Comment 5

[Pi Wei Cheng [:piwei] (inactive)]

With Music NGA, I repro'ed the bug 4 out of 7 attempts. With Music OGA, I repro'ed the bug 0 out of 10 attempts. So it's probably a Music NGA issue.

Tested on:
Device: Flame 2.5
BuildID: 20151016030225
Gaia: 8ea9029190af2ffeb04dcd97b323738125e31a0e
Gecko: d374d16cbb251c9dac5af69f8e186e821ce82fe2
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 44.0a1 (2.5) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

Comment 6

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

[Blocking Requested - why for this release]:
This issue results in UI that doesn't function and could be confusing for the user.

Justin and No-Jun, can you take a look at this please?

Comment 7

[Justin D'Arcangelo [:justindarc]]

Moving this to the Lockscreen component. There is nothing that can be done to prevent this from within the Music app itself :-/

Comment 8

[No-Jun Park [:njpark]]

Noming it to 2.5, to start a discussion whether it's a blocker.  I'm on the fence on this one, but 40% occurrence of plainly false widget seems wrong to me.

Comment 9

[Mahendra Potharaju [:mahe]]

Blocking for 2.5 with a P3 priority.

Comment 10

[Greg Weng [:snowmantw][:gweng][:λ]]

As Comment 5:

"With Music NGA, I repro'ed the bug 4 out of 7 attempts. With Music OGA, I repro'ed the bug 0 out of 10 attempts. So it's probably a Music NGA issue."

What's your reason to just change the component and think it's all LockScreen issue?

Comment 11

[Justin D'Arcangelo [:justindarc]]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #10)
> As Comment 5:
> 
> "With Music NGA, I repro'ed the bug 4 out of 7 attempts. With Music OGA, I
> repro'ed the bug 0 out of 10 attempts. So it's probably a Music NGA issue."
> 
> What's your reason to just change the component and think it's all
> LockScreen issue?

See Comment 7 -- Because neither version of the Music app does anything to signal to the LockScreen/Notification tray that it is being terminated. AFAIK, apps don't get an opportunity to "clean up" when being terminated, so I believe it is the System app's responsibility to be watching for these apps to be killed.

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

For me the case is: if it is a regression as Comment 2 shows, no matter whether it's caused by Music, NGA or LockScreen, there should be a regression window and I will bisect it. After that, he who landed the patch to break the thing should fix that, since that's how we handle regressions so far.

And since there are no regression flag and window while it was reported as a regression as Comment 2, I will update the bug status and do what I'm saying, according to the window now we have. If QA can help, we may shrink the window to make it more narrow.

Comment 13

[Greg Weng [:snowmantw][:gweng][:λ]]

While it's obvious a regression according to Comment 2 and the causing bug is undiscovered (although Comment 5 may indicated the root cause without the actual bug), I don't know why in Comment 6 we removed the flag. So I add it again.

Comment 14

[Greg Weng [:snowmantw][:gweng][:λ]]

Wait: I notice in the STR the utility tray was broken, too. So there is apparently some messaging things gets wrong. Anyway, after my bisecting the causing bug will be picked.

Comment 15

[Justin D'Arcangelo [:justindarc]]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #14)
> Wait: I notice in the STR the utility tray was broken, too. So there is
> apparently some messaging things gets wrong. Anyway, after my bisecting the
> causing bug will be picked.

Yes. I believe the Notification Tray also listens for the Music app's termination.

To clarify, the reason I moved to the Gaia::System::Lockscreen component is because the Lockscreen seems to directly deal with observing the termination of app processes/frames and I had hoped that someone who is familiar with Lockscreen may have an explanation as to why Lockscreen/Notification Tray is not being notified immediately about an app closure. Although, I suppose it might actually belong under the Window Mgmt component.

Comment 16

[Greg Weng [:snowmantw][:gweng][:λ]]

Alright, but I still want to pin down the root cause and the bug. The latest result is I could confirm the bug doesn't exist at commit

    fa8f195df7e2470a67ffc2aeeddf4443051427fa
    Bug 1204716 - [Music][NGA] Wire up music controls to lockscreen

And I'm bisecting according to the result.

Comment 17

[Greg Weng [:snowmantw][:gweng][:λ]]

After bisecting, I've found Bug 1208154 caused the issue. The commit is:

    commit 971df213c86811dce3370b2ff0d12f44041c9fe9
    Author: Justin D'Arcangelo <[e-mail]>
    Date:   Fri Sep 25 17:43:58 2015 -0400

    Bug 1208154 - [Music][NGA] Replace 'apps/music' with 'dev_apps/music-nga'

I've also uploaded the video since the issue is a bit tricky: you need to pause about 0.5 second before you click the power button to lock the screen, or to click it too fast the issue will not occur. I don't know why the pause is so important to reproduce the issue, but anyway the video is here:

https://www.youtube.com/watch?v=dBQ1CGqhUgw

You can see I failed to reproduce the issue without the pause, but did that successfully after that with the pause.

And I tried to revert the bug to test if the build behave normally. The video is here:

https://www.youtube.com/watch?v=789nRDRara4

This time the issue seems gone. So I think the bug is the root cause. According to that, I think the change of Music app made something different about the messages of terminating, thus we now have the issue.  
Besides that, since the bug didn't touch anything about System app but to move the testing Music to replace the old one, I hope someone is familiar with Music app could help us to find out the real cause of such issue. Because I'm totally out of NGA Music and the deadline of 2.5 is very soon.

If my bisecting result is wrong, welcome to correct it by a new actual bug.

Comment 18

[Greg Weng [:snowmantw][:gweng][:λ]]

And according to the result, change the component again. If there are some causes inside System app, please change it to System::WindowManagement.

Comment 19

[Justin D'Arcangelo [:justindarc]]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #18)
> And according to the result, change the component again. If there are some
> causes inside System app, please change it to System::WindowManagement.

Maybe the Music app should listen for when its about to be terminated and then at the last second, right before we kill the process, we send some magical event up to the Lockscreen and NotificationTray to let them know that they need to clean up...

Oh, wait.. Nevermind.. Apps in FxOS don't get notified of their termination. As I've said already in Comment 7, Comment 11 and Comment 15.

Comment 20

[Jim Porter (:squib)]

I'm almost 100% certain this has nothing to do with NGA. I've seen this issue appear with OGA as well, but never had an easy way to reproduce it.

I've argued for a long time that we need a more-reliable way to determine when an app with an IAC connection has been killed (see bug 915880 and bug 920682), but no one has listened, and I've had many other more-pressing issues to handle, so I haven't delved into the Gecko code that manages this. Given that we'll be moving towards the Media Controller API(?) for this in the future, it might not even be worth fixing properly. I'd be totally happy to see IAC purged from Gecko.

Comment 21

[Justin D'Arcangelo [:justindarc]]

As Jim said in Comment 20, this happened in OGA too, but was more inconsistent to reproduce. If I had to take a guess as to why this *might* be easier to reproduce in NGA, maybe it has something to do with the <iframe>-based views and the clean-up that comes with it. I suppose its possible that its taking longer to clean up the new app and that's why the media controls linger around a bit longer.

Comment 23

[Greg Weng [:snowmantw][:gweng][:λ]]

Therefore the bug status should be updated again. Currently, the regression window is uncleared since Comment 20 said it could happen before that and the proper solution already exists. So I think either we remove the regression tag and correct the bug status, especially for the tracking flags, and make it as a fundamental change relates to Gecko and API, or we split the bug into two bugs: one for easing the symptom to meet what 2.5 needs, although it must come with workarounds that is not proper as the API solution, and another one is for the proper solution by the API changes.

And for the already existing symptom as Comment 20 mentioned, I also suspect the original STR needs to be verified again. Since in Comment 5 QA looks not be able to reproduce the issue with only OGA. So to get a reliable method that across OGA and NGA we need a new STR.

By the way, I just checked the Media Control API which I've been noticed it can replace the IAC while I was in Portland, but I haven't got any message or scheduled item about that after the workweek except one discussion about the interface on the mailing list. So I don't know if we now have any plan in releases to migrate to that.

Comment 24

[Justin D'Arcangelo [:justindarc]]

Here's a simple workaround that the System app could do:

When waking the lockscreen or opening the notification tray, if the media controls are visible, check for the existence of the Music app window. If the Music app window no longer exists, hide the media controls.

That should at least mask the issue at hand. Obviously, we shouldn't have to do this additional check, but for some reason, lockscreen and notification tray are not being notified of app termination in a timely matter. This solution would at least compensate for the cases where the user goes to the lockscreen or notification tray before the kill notification happens.

Also, removing the "regression" keyword as suggested in Comment 23 based on information in Comment 20.

Comment 25

[Steve Chung [:steveck]]

Just took some time on this issue and I found the media playback(tray/lockscreen) simply controls the container display by play status and appterminated event. And this issue will be reproduced when one more playing event comes after appterminated event. IMO it seems like IAC issue because status event should not be fired after the music closed. I don't think that music will have chance to stop the music before terminated, so easier workaround might be checking the music existence or add a throttle control for events instead of setting the playback display directly.

Comment 26

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

(In reply to Steve Chung [:steveck] from comment #25)
> Just took some time on this issue and I found the media
> playback(tray/lockscreen) simply controls the container display by play
> status and appterminated event. And this issue will be reproduced when one
> more playing event comes after appterminated event. IMO it seems like IAC
> issue because status event should not be fired after the music closed. I
> don't think that music will have chance to stop the music before terminated,
> so easier workaround might be checking the music existence or add a throttle
> control for events instead of setting the playback display directly.

Agreed. We should do the workaround in comment 24, or find the underlining IAC issue and fix it. This bug should have put into System: Music Control which was setup for this part of System.

Comment 27

[GitHub Autolander Bot]

Attachment: [gaia] steveck-chung:bug-1215674-music-control-hide > mozilla-b2g:master

Comment 28

[Steve Chung [:steveck]]

Comment on attachment 8681131 [details] [review]
[gaia] steveck-chung:bug-1215674-music-control-hide > mozilla-b2g:master

Hi Justine/Greg,
I think the IAC event handling might not be solved soon, so I create a small workaround for it that simply add addition checking before setting to play. I can fire another bug for IAC if you agree with landing the workaround first.

Justine,
I set the review to you because this module seems maintained by music peer previously, but feel free to redirect to others, thanks!

Comment 29

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8681131 [details] [review]
[gaia] steveck-chung:bug-1215674-music-control-hide > mozilla-b2g:master

LGTM.. Thanks!!

Comment 30

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8681131 [details] [review]
[gaia] steveck-chung:bug-1215674-music-control-hide > mozilla-b2g:master

With one comment nit. Thanks.

Comment 31

[Steve Chung [:steveck]]

In master:https://github.com/mozilla-b2g/gaia/commit/d11b77ef20f2dab702a5cd89624d11b768a609ea
Thanks for the review！

Comment 32

[Tony(Leave from Mozilla)]

This bug has been verified as "pass" on the latest build of Aries KK v2.6 and Flame KK v2.6 512mb by the STR in comment 0.

Actual results: The music widget goes away when the user closes the music app.
See attachment: Verified_Aries_KK_v2.6.3gp
Reproduce rate: 0/10

Device: Aries KK 2.6 (master) (Pass)
Build ID               20151104004249
Gaia Revision          61918ddd9ccce104c009e873e34a0791e125753a
Gaia Date              2015-11-03 17:22:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/f742b9412ed5aace90ad863b276faae0641090a8
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151104.000116
Firmware Date          Wed Nov  4 00:01:24 UTC 2015
Bootloader             s1


Device: Flame KK 2.6 (master) 512mb (Pass)
Build ID               20151103150203
Gaia Revision          61918ddd9ccce104c009e873e34a0791e125753a
Gaia Date              2015-11-03 17:22:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/f742b9412ed5aace90ad863b276faae0641090a8
Gecko Version          45.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151103.182550
Firmware Date          Tue Nov  3 18:26:03 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Comment 33

[Tony(Leave from Mozilla)]

Attachment: Verified_Aries_KK_v2.6.3gp