Closed Bug 1148342 Opened 10 years ago Closed 9 years ago

[Flame][Music]When music play is over in sleep mode, wait for arround 20s, and wake up device, it will load to music play panel.


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

Gonk (Firefox OS)
Not set


(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

2.2 S11 (1may)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified


(Reporter: wangxin, Assigned: djf)




(3 files)

[Flame][v2.2][Music]Select a song from Music list, play it, and then lock screen.  After the music play is over, wait for about 20s. Then if you unlock screen, you can find device stays at the player view.
Found Time:03:51
See log:"logcat_0351.txt"
See video:"0351.mp4"

[2.Testing Steps]: 
1. Launch Music .
2. Play a music from "Music" page.
3. Press "Power" button to lock device.
4. Wait the music playing over, then wait arround 20s.
5. Press "Power" again to wake up the device.
6. Click Play/Star/Share button.

[3.Expected Result]: 
5. The page will load to Music list page.

[4.Actual Result]: 
5. The page will stop at play panel page.
6. These button are not availble.

[5.Reproduction build]: 
Flame 2.2:(Affected)
Build ID               20150326164141
Gaia Revision          6d0174e28576f2f93e696a43d1ac3b03340117f6
Gaia Date              2015-03-26 21:47:33
Gecko Revision
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150326.201153
Firmware Date          Thu Mar 26 20:12:05 EDT 2015
Bootloader             L1TC000118D0

Build ID               20150326160206
Gaia Revision          525c341254e08f07f90da57a4d1cd5971a3cc668
Gaia Date              2015-03-26 16:34:16
Gecko Revision
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150326.193247
Firmware Date          Thu Mar 26 19:32:58 EDT 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

Free Test
Attached video Bug video: 0351.MP4
"Unknown title" shown for this.  
[Blocking Requested - why for this release]: Weird UX for user, functionality not working
blocking-b2g: --- → 2.2?
poor UX experience, please fix.
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(dkuo)
A blocker so taking it.
Assignee: nobody → dkuo
Flags: needinfo?(dkuo)

Please update on the progress of this bug? Can you wrap this up this week?

Flags: needinfo?(dkuo)
See Also: → 1153960
See Also: → 1149090
Dominic is busy with music perf bugs, so I'm taking this to see if I can figure it out while it is nighttime in Taipei.  

This bug has been reported twice more. It keeps coming up, so it is pretty important that we get it solved.
Assignee: dkuo → dflanagan
I think the STR about putting the phone to sleep are something of a red herring. In the two bugs I've marked as dupes of this one, but same issue occurred  when the song played normally to the end, and when the user explicitly pushed the 'next song' button.

In all three cases, the common thread was that the user entered the player view from the tiles view, and the tile they clicked on represented a single song, not an album of songs.
Actually, it does not have to be a single song. I can tap on any tile in the tile list and then if I just keep clicking the 'next song' button I'll often see this bug when I reach the end of the album.  This happens for .m4a, .mp3 and .ogg files, so it is not related to the media format.  It does not seem to happen when we're in "repeat album" mode.

It does not seem to ever happen when I play a song or album through the Artists or Albums panels, only when I start playing from the tiles view.

This bug was reported long enough ago that I don't think it is a regression from any of the recent blockers or performance work we've been doing.
In player_view.js, when we reach the end of an album, the next() function calls stop(). It looks like stop() is supposed to call ModeManager.pop() to return to the previous view. Since it is not doing that, I'm guessing that ModeManager.currentMode is not correctly initialized when we go from the tiles view to the player view.  I'll investigate that.
The attached logcat shows JS errors, which may be a hint for this bug. The logcats for the two duplicate bugs do not show the same errors, and I'm not seeing the errors occur in my own testing, however.

This bug seems to reproduce more easily on 2.2 than it does in 3.0
I can reproduce the JS errors in the the attached logcat by getting the app into this "unknown title" state and then tapping the share button, the rating starts, or the previous-song button.
ModeManager.currentMode is correctly set when the bug occurs.  But when I look at the ModeManager.pop() method, I see that the stack size is 2 when the bug does not happen and it is 3 (or more, sometimes) when the bug does happen.

I think this is related to another really annoying bug that I've been finding frustrating. Sometimes tapping on a tile does not take us to the player view.  It seems that each tap is adding something to the mode manager stack.  So if I have to tap twice on a tile to enter the player view, then this bug will appear, and I'll have a stack size of 3. Which means that I'll have to tap the "next song" button one extra time to get back to the tiles.  If I have to tap three times to play my album, then I'll have to tap the next song two extra times to pop the stack and get back to the player view.

I'm not 100% certain yet that the extra taps are the cause of this, but I think so. 

Note that the video attached to this bug shows that it takes two taps on the tile to get the song to play. 
Also, the video for bug 1149090 requires two taps. I'm surprised that that has not been reported as a bug!
I think this bug is probably caused by the transitionend callback at tiles_view.js:203. If the transition is over before the click event is handled, then the click will not call  tv_playAlbum() to be called. But the handler will still be registered. So when we click again and don't miss the transition, tv_playAlbum() will be called twice in a row.

It was a terrible idea to make the JS code dependent on a transition duration defined in CSS. And anyway, why would we ever want to delay a callback just so an :active transition can finish?
Changing the transition time in music.css:603 does not seem to make any difference in whether the bug can reproduce or not, so I suspect that the event handler is synchronous and we are not missing the event.

Instead, I think that sometimes (especially for fast clicks) gecko is never getting around to setting the :active state on the tile, so we don't get any transition at all. We're not missing the transitionend event; that event is just never being sent.

If I comment out the :active style for the tiles at music.css:606, then tapping on a tile never does anything at all.
This does not appear to be a regression in gaia. We've had the transitionend code since at least November 2014, and we've had the :active transition since 2013.

I suspect that what regressed this is the way gecko is setting the :active state. But code that depends on a :active state transition is too brittle anyway. We shouldn't have had that JS dependency on CSS in the first place.

I'll prepare a patch to remove it.
Comment on attachment 8596151 [details] [review]
[gaia] davidflanagan:bug1148342 > mozilla-b2g:master

Since this bug is front-end and was assigned to Dominic, I'm asking for his review.

But if you want to take the review, Jim, we could get this closed out sooner.
Attachment #8596151 - Flags: review?(dkuo)
Attachment #8596151 - Flags: feedback?(squibblyflabbetydoo)
SandiKing: would you test this patch and see if it resolves the bug for you?
Flags: needinfo?(wangxin)
Coler: I think this patch fixes bug 1149090 which you reported. Can you confirm that?
Flags: needinfo?(liuyong)
Joshua or Peter: I think this patch fixes bug 1153960 which you were involved with. Could one of you test it and confirm that, please?
Flags: needinfo?(pbylenga)
Flags: needinfo?(jmitchell)
Comment on attachment 8596151 [details] [review]
[gaia] davidflanagan:bug1148342 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): some kind of gecko change affected really fragile gaia code in the music app. No known bug number

[User impact] if declined: the music app will sometimes require multiple clicks to play music, and will sometimes display "unknown title" at the end of an album instead of going back to the tiles.

[Testing completed]: I've tested it, and have requested testing by QA in the needinfos above.

[Risk to taking this patch] (and alternatives if risky): not risky. It is a straightforward change that also makes the app more responsive since we no longer wait for an animation to finish before playing the user's music.

[String changes made]: none
Attachment #8596151 - Flags: approval-gaia-v2.2?(bbajaj)
Surprisingly, I cannot find any Music app bugs filed for the issue where sometimes tapping on an album in tiles view would not play the album.  I suspect this means that the Flame touchscreen is unreliable enough that we are all used to having to tap multiple times in our apps :-(
Comment on attachment 8596151 [details] [review]
[gaia] davidflanagan:bug1148342 > mozilla-b2g:master

This seems reasonable, and I'm happy to see this code go.
Attachment #8596151 - Flags: feedback?(squibblyflabbetydoo) → feedback+
Hi David,
This bug has been verified succefully on Flame 3.0 using the patch provided in comment 18.
Same STR with comment0 
Rate: 0/10

Flme 3.0 build(Pass):
Build ID               20150422160203
Gaia Revision          4d87112bbf48cdd09c19e553cc9aebd2a2c4ddfd
Gaia Date              2015-04-23 04:16:36
Gecko Revision
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150422.193515
Firmware Date          Wed Apr 22 19:35:27 EDT 2015
Bootloader             L1TC000118D0
Flags: needinfo?(wangxin)
Comment on attachment 8596151 [details] [review]
[gaia] davidflanagan:bug1148342 > mozilla-b2g:master

Looking on the comments David wrote, counting on the transitionend event is a unreliable approach and I also agree we should just execute the tile handler when the click event fires.
Flags: needinfo?(dkuo)
Attachment #8596151 - Flags: review?(dkuo) → review+
Keywords: checkin-needed
Closed: 9 years ago
Resolution: --- → FIXED
Flags: needinfo?(pbylenga)
Keywords: qawanted, verifyme
Attachment #8596151 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
QA Contact: bzumwalt
(In reply to David Flanagan [:djf] from comment #22)
> Joshua or Peter: I think this patch fixes bug 1153960 which you were
> involved with. Could one of you test it and confirm that, please?

Confirmed; this patch fixes the issue that was written in bug 1153960

Device: Flame 3.0 (KK - Nightly - Full Flash - 319mem)
Build ID: 20150424010200
Gaia: 0c5e2ee1173f3c53379ef3cd10de714836258fe8
Gecko: 22a157f7feb7
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0
Flags: needinfo?(jmitchell)
See Also: → 1158397
This bug has been verified successful on latest Flame 2.2,
Same STR with comment0
Rate: 0/10

Flame 2.2 build(Pass):
Build ID               20150426002504
Gaia Revision          265ca0bc9408c21fc4b25a259fcee7fb642cd06b
Gaia Date              2015-04-24 19:13:28
Gecko Revision
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150426.043030
Firmware Date          Sun Apr 26 04:30:42 EDT 2015
Bootloader             L1TC000118D0
Flags: needinfo?(liuyong)
Bug 1158692 is the obstacle for us to verify on 3.0.
QA Contact: bzumwalt
This issue is verified fixed on the Flame 3.0

All controls work properly after following the steps in Comment 0

Environmental Variables:
Device: Flame 3.0 (Full Flash)(KK)(319mb)
BuildID: 20150429010205
Gaia: 6e35b0948c42a4398b8a5916015de167121683a1
Gecko: 1ad65cbeb2f4
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.