Closed Bug 1148342 Opened 6 years ago Closed 5 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
148.95 KB, text/plain
5.13 MB, video/mp4
46 bytes, text/x-github-pull-request
|Details | Review|
[1.Description]: [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 https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/820c2f2e817a 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 Flame3.0::(Unaffected) Build ID 20150326160206 Gaia Revision 525c341254e08f07f90da57a4d1cd5971a3cc668 Gaia Date 2015-03-26 16:34:16 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/59554288b4eb 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 [7.TCID]: Free Test
"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+
A blocker so taking it.
Assignee: nobody → dkuo
Dominic, Please update on the progress of this bug? Can you wrap this up this week? Thanks Hema
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.
SandiKing: would you test this patch and see if it resolves the bug for you?
Coler: I think this patch fixes bug 1149090 which you reported. Can you confirm that?
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?
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 https://hg.mozilla.org/mozilla-central/rev/a9311ec2dd39 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
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.
Attachment #8596151 - Flags: review?(dkuo) → review+
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/0c5e2ee1173f3c53379ef3cd10de714836258fe8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8596151 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
(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
This bug has been verified successful on latest Flame 2.2, STR: 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 https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1908685d798d 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
Bug 1158692 is the obstacle for us to verify on 3.0.
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
You need to log in before you can comment on or make changes to this bug.