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

Status: RESOLVED FIXED

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

Description

[Lucas Aliaga]

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.

Comment 1

[Jason Smith [:jsmith]]

This is unrelated to burirun3

Comment 2

[Hema Koka [:hema]]

Looks like it is related to 1.3 feature that landed. Jim, please look into whats going on

Comment 3

[Jim Porter (:squib)]

Attachment: 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.

Comment 4

[Jim Porter (:squib)]

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.

Comment 5

[Jim Porter (:squib)]

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.

Comment 6

[Jason Smith [:jsmith]]

(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.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

(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 8

[(inactive after 6/18) Alive Kuo [:alive]]

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?

Comment 9

[Jim Porter (:squib)]

(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.

Comment 10

[Jim Porter (:squib)]

Checked in: https://github.com/mozilla-b2g/gaia/commit/03b81ac963487abf90d5a40c566018674f97d185

Comment 11

[Preeti Raghunath(:Preeti)]

moved to 1.3 per comments 6 and 7.

Comment 12

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(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 :]

Comment 13

[Jason Smith [:jsmith]]

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?

Comment 14

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(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 !!

Comment 15

[Jim Porter (:squib)]

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.

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(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.

Comment 17

[Jim Porter (:squib)]

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

Comment 18

[Jason Smith [:jsmith]]

(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?

Comment 19

[Jim Porter (:squib)]

(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 20

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

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

Thanks!

Comment 21

[Jason Smith [:jsmith]]

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.

Comment 22

[Jason Smith [:jsmith]]

(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 23

[Jason Smith [:jsmith]]

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

No longer accepting approvals for 1.2, so minusing this.

Comment 24

[Jim Porter (:squib)]

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.

Comment 26

[Jason Smith [:jsmith]]

blocking+ for bad bug in 1.3 feature