[B2G][Music] Force closing the Music app while music is playing results in notification media controls not working on subsequent launches.

RESOLVED FIXED in 1.3 Sprint 5 - 11/22

Status

Firefox OS
Gaia::Music
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Lucas Aliaga, Assigned: squib)

Tracking

({regression})

unspecified
1.3 Sprint 5 - 11/22
ARM
Gonk (Firefox OS)
regression

Firefox Tracking Flags

(blocking-b2g:1.3+)

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
alive
: review+
Details | Review | Splinter Review
(Reporter)

Description

4 years ago
While listening to music, if the user long presses the home button and force closes the music app, any time the app is launched after the notification controls will not function.

Repro Steps:
1. Update Buri device to 1.3 buildID: 20131101040203.
2. Launch the music app and play a song.
3. Pull down the notification shade and pause the music.
4. Force close the music app via long pressing the home button and swiping the card away.
5. Relaunch the music app and play a song.
6. Pull down the notification shade and attempt to pause the music.

Actual Results:
Media notification controls are not functional after force closing the music app.

Expected Results:
Notification media controls to always remain functional while media is playing.

Repro Rate: 5/5 100%

Environment Variables:
Gaia   ccdf357ea150fc7d8b8a4b74c7adf31e7a57e465
SourceStamp abe6790a5dd8
BuildID 20131101040203
Version 28.0a1

Unable to obtain logcat due to media not being able to be accessed while plugged in.

Updated

4 years ago
Whiteboard: burirun3

Updated

4 years ago
blocking-b2g: --- → 1.3?
This is unrelated to burirun3
Whiteboard: burirun3
QA Contact: mozillamarcia.knous

Comment 2

4 years ago
Looks like it is related to 1.3 feature that landed. Jim, please look into whats going on
Assignee: nobody → squibblyflabbetydoo
Target Milestone: --- → 1.3 Sprint 5 - 11/22
(Assignee)

Comment 3

4 years ago
Created attachment 832393 [details] [review]
Fix this

Here's a fix for this; the IAC helper had a bug that was keeping the old, closed message port around. I'm waiting on a successful Travis run before I request review.
(Assignee)

Comment 4

4 years ago
Comment on attachment 832393 [details] [review]
Fix this

Alive: this is all in /shared/js, but the system app is the only consumer of that code right now, so you're probably the best person to ask for review. The actual fix is just removing an if statement, but I cleaned up a couple of things too, and added marionette tests.
Attachment #832393 - Flags: review?(alive)
(Assignee)

Comment 5

4 years ago
This regressed from bug 911681 and so will need uplift to 1.2 when r+ed.

I thought I was supposed to get a little template for filling out the impact of this regression, but since I didn't, I'll just do it manually.

For triagers: This bug is pretty low-risk (it's just a minor change to how we track IAC ports), has tests, and fixes some pretty serious bustage: namely that IAC with the system app totally breaks if you force close and reopen the app sending the IAC messages. In particular, this means that the music controls in the notifications tray and lockscreen won't work after force closing the music app.
Blocks: 911681
blocking-b2g: 1.3? → koi?
Keywords: regression
(In reply to Jim Porter (:squib) from comment #5)
> This regressed from bug 911681 and so will need uplift to 1.2 when r+ed.
> 
> I thought I was supposed to get a little template for filling out the impact
> of this regression, but since I didn't, I'll just do it manually.
> 
> For triagers: This bug is pretty low-risk (it's just a minor change to how
> we track IAC ports), has tests, and fixes some pretty serious bustage:
> namely that IAC with the system app totally breaks if you force close and
> reopen the app sending the IAC messages. In particular, this means that the
> music controls in the notifications tray and lockscreen won't work after
> force closing the music app.

I think the better solution here would be to backout bug 911681, given that we're no longer shipping flatfish on 1.2.
(In reply to Jason Smith [:jsmith] from comment #6)
> (In reply to Jim Porter (:squib) from comment #5)
> > This regressed from bug 911681 and so will need uplift to 1.2 when r+ed.
> > 
> > I thought I was supposed to get a little template for filling out the impact
> > of this regression, but since I didn't, I'll just do it manually.
> > 
> > For triagers: This bug is pretty low-risk (it's just a minor change to how
> > we track IAC ports), has tests, and fixes some pretty serious bustage:
> > namely that IAC with the system app totally breaks if you force close and
> > reopen the app sending the IAC messages. In particular, this means that the
> > music controls in the notifications tray and lockscreen won't work after
> > force closing the music app.
> 
> I think the better solution here would be to backout bug 911681, given that
> we're no longer shipping flatfish on 1.2.

Agree.
Comment on attachment 832393 [details] [review]
Fix this

I think the problem is just it was using array to store key-value object...isn't it?
Attachment #832393 - Flags: review?(alive) → review+
(Assignee)

Comment 9

4 years ago
(In reply to Alive Kuo [:alive][NEEDINFO] from comment #8)
> Comment on attachment 832393 [details] [review]
> Fix this
> 
> I think the problem is just it was using array to store key-value
> object...isn't it?

That technically works, since a JS array is a kind of object, so you can add your own attributes to it without any issues. You end up with some extra attributes that you probably don't want though, like "length", "forEach", etc.

The real issue was the if statement, since closing the music app didn't clear the port out from IACHelper._ports, and then when we reopened the music app, the if statement told us not to update the port to the new one. The end result was that when the system app tried to send a message to the second music app, it was using a stale port.
(Assignee)

Comment 10

4 years ago
Checked in: https://github.com/mozilla-b2g/gaia/commit/03b81ac963487abf90d5a40c566018674f97d185
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
moved to 1.3 per comments 6 and 7.
blocking-b2g: koi? → 1.3?
(In reply to Jason Smith [:jsmith] from comment #6)
> (In reply to Jim Porter (:squib) from comment #5)
> > This regressed from bug 911681 and so will need uplift to 1.2 when r+ed.
> > 
> > I thought I was supposed to get a little template for filling out the impact
> > of this regression, but since I didn't, I'll just do it manually.
> > 
> > For triagers: This bug is pretty low-risk (it's just a minor change to how
> > we track IAC ports), has tests, and fixes some pretty serious bustage:
> > namely that IAC with the system app totally breaks if you force close and
> > reopen the app sending the IAC messages. In particular, this means that the
> > music controls in the notifications tray and lockscreen won't work after
> > force closing the music app.
> 
> I think the better solution here would be to backout bug 911681, given that
> we're no longer shipping flatfish on 1.2.

Yeah, because our flatfish would not be shipped with v1.2, the better way is to back out bug 911681 and leave the changes on master :]
So per discussions on bug 911681 - we aren't going to back that bug out due to risk.

Jim - What specific impact does this have to 1.2? I see comment 5's points, but doesn't that only apply to 1.3? Or are there specific areas that will affect 1.2?
Flags: needinfo?(squibblyflabbetydoo)
(In reply to Jason Smith [:jsmith] from comment #13)
> So per discussions on bug 911681 - we aren't going to back that bug out due
> to risk.
> 
> Jim - What specific impact does this have to 1.2? I see comment 5's points,
> but doesn't that only apply to 1.3? Or are there specific areas that will
> affect 1.2?

I have tried latest v1.2 and there is no this problem on v1.2 ! 

Jim, is there any problem causing by this on v1.2 !? If not, we can just leave this patch on master and I can land my patch on bug 934690 directly !!
(Assignee)

Comment 15

4 years ago
I don't think this breaks anything for music in 1.2, but I still think it'd be good to take this bug since it's low-risk and if the system app ever needs to *send* an IAC message in 1.2, it might break in the same way as the music app breaks in 1.3.
Flags: needinfo?(squibblyflabbetydoo)
(In reply to Jim Porter (:squib) from comment #15)
> I don't think this breaks anything for music in 1.2, but I still think it'd
> be good to take this bug since it's low-risk and if the system app ever
> needs to *send* an IAC message in 1.2, it might break in the same way as the
> music app breaks in 1.3.

Agree, this can make IAC more stable on 1.2. 

Jim, if there is no other side effect of landing this on v1.2, can we mark this "approval‑gaia‑v1.2" ? 

Tim will approve this change.
Flags: needinfo?(squibblyflabbetydoo)
(Assignee)

Comment 17

4 years ago
Comment on attachment 832393 [details] [review]
Fix this

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 911681
[User impact] if declined: could cause breakage if the system app tries to send IAC messages; I don't know if this happens in 1.2 (it certainly happens in 1.3), but I think we should be safe here.
[Testing completed]: unit tested on 1.3
[Risk to taking this patch] (and alternatives if risky): very low-risk
[String changes made]: none
Attachment #832393 - Flags: approval-gaia-v1.2?(timdream)
(Assignee)

Updated

4 years ago
Flags: needinfo?(squibblyflabbetydoo)
(In reply to Jim Porter (:squib) from comment #17)
> Comment on attachment 832393 [details] [review]
> Fix this
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): bug 911681
> [User impact] if declined: could cause breakage if the system app tries to
> send IAC messages; I don't know if this happens in 1.2 (it certainly happens
> in 1.3), but I think we should be safe here.
> [Testing completed]: unit tested on 1.3
> [Risk to taking this patch] (and alternatives if risky): very low-risk
> [String changes made]: none

This really needs to be driven by rel man at this point. Can you switch the approval request to one of the rel managers?

Fixing blocking flag also back to 1.3?
blocking-b2g: koi? → 1.3?
(Assignee)

Comment 19

4 years ago
(In reply to Jason Smith [:jsmith] from comment #18)
> This really needs to be driven by rel man at this point. Can you switch the
> approval request to one of the rel managers?

Is there a list of such people anywhere? I'm not sure who to request this from (it's not often I want to uplift something so late in the cycle).
Comment on attachment 832393 [details] [review]
Fix this

Thanks!
Attachment #832393 - Flags: approval-gaia-v1.2?(timdream) → approval-gaia-v1.2+
Comment on attachment 832393 [details] [review]
Fix this

As stated before this is not Tim's call to make at this point - this is a rel man call. Clearing approval & remarking for rel man to review.
Attachment #832393 - Flags: approval-gaia-v1.2+ → approval-gaia-v1.2?
(In reply to Jim Porter (:squib) from comment #19)
> (In reply to Jason Smith [:jsmith] from comment #18)
> > This really needs to be driven by rel man at this point. Can you switch the
> > approval request to one of the rel managers?
> 
> Is there a list of such people anywhere? I'm not sure who to request this
> from (it's not often I want to uplift something so late in the cycle).

bbajaj & Preeti are the people you should request approval for in the last 6 weeks of the release.
Comment on attachment 832393 [details] [review]
Fix this

No longer accepting approvals for 1.2, so minusing this.
Attachment #832393 - Flags: approval-gaia-v1.2? → approval-gaia-v1.2-
(Assignee)

Comment 24

4 years ago
Sorry about not updating this bug! I was actually going to say that I think this isn't worth landing on 1.2. I couldn't find anything that it actually broke, so landing it would have just been a "code cleanliness" issue, and that's not worth a late landing.
Duplicate of this bug: 946522
blocking+ for bad bug in 1.3 feature
blocking-b2g: 1.3? → 1.3+
You need to log in before you can comment on or make changes to this bug.