820704 - Failure in "MW1: Music stays alive"; Music playback pauses when in background, unexpectedly

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

+++ This bug was initially created as a clone of Bug #820702 +++

STR
 (1) Follow the steps at https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW0:_Every_app_is_successfully_launched_into_the_foreground_.5BPASS.5D
 (2) After step 18, the homescreen failed to load

Switching to a different app and back to homescreen, locking and unlocking the screen, repeatedly tapping where the homescreen should have been, failed to get it to reload.

Extremely bad bug.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Damnable fat-fingering.

STR
 (1) Follow the steps at https://wiki.mozilla.org/B2G/Memory_acceptance_criteria#MW1:_Music_stays_alive_.5BUNEXPECTED-
 (2) After step 4, the music stream pauses

Broken feature.

Comment 2

[Andrew Overholt [:overholt]]

Andrea, please investigate.

Comment 3

[Andrew Overholt [:overholt]]

dhylands opined that the music app was perhaps killed in the background.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

No, that's not the case.  Playback stops immediately when after the music app is sent to the background.  See comment 0.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

IOW, I can switch back to the open music app after comment 4.

Comment 6

[Andrea Marchesini [:baku]]

(In reply to Andrew Overholt [:overholt] from comment #2)
> Andrea, please investigate.

I cannot reproduce this bug. I'm using the latest code from central + gaia.
The Music playback continues perfectly after step 4. I have problems after step 11, but this is not related to the audio: the homescreen doesn't appear and I cannot continue with step 12.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) Open music app
 (2) Play song
 (3) Send to background (go to homescreen)
 (4) Open task switcher and kill music app
 (5) Re-open music app and play song again
 (6) Send to background

Playback stops.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment 7 implicates me in not running a clean test in comment 0, so this isn't blocking memory tests anymore.

However, that doesn't change the nature of this bug.

Comment 9

[Andrea Marchesini [:baku]]

>  (1) Open music app
>  (2) Play song
>  (3) Send to background (go to homescreen)
>  (4) Open task switcher and kill music app
>  (5) Re-open music app and play song again
>  (6) Send to background

I see the bug. Thanks. /me working on it.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch

This fixes the problem but I need a feedback.
With this patch we store the appIds in the audio channel service. Then when the child process quits, we call RemoveAppIds().
This patch doesn't involve FMRadio because there is not an easy way to get the appid and we don't support properly multiple apps using FMRadio at the same time.

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

Is this a dup of bug 815355?

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Nope, different bugs that occur on the same testcase.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 692291 [details] [diff] [review]
patch

Review of attachment 692291 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/audiochannel/AudioChannelService.cpp
@@ +68,5 @@
>  
>  AudioChannelService::AudioChannelService()
>  : mCurrentHigherChannel(AUDIO_CHANNEL_NORMAL)
>  {
> +  mChannelCounters = new nsTArray<uint32_t>[AUDIO_CHANNEL_PUBLICNOTIFICATION+1];

Why not make mChannelCounters of type:

nsTArray<uint32_t>[AUDIO_CHANNEL_PUBLICNOTIFICATION+1];

@@ +76,5 @@
>  }
>  
>  AudioChannelService::~AudioChannelService()
>  {
>    delete [] mChannelCounters;

...and remove this.

@@ +117,2 @@
>  {
> +  mChannelCounters[aType].RemoveElement(aAppId);

Add a note that there might be multiple elements with the same appid in the array and this will, and should, only remove the first one.

@@ +223,5 @@
>  }
>  
> +PLDHashOperator
> +AudioChannelService::NotifyEnumerator(AudioChannelAgent* aAgent,
> +                                      AudioChannelAgentData aData, void* aUnused)

Put a "// static" comment above this to show that it's static. We do this somewhat consistently in gecko code.

@@ +297,5 @@
>  void
>  AudioChannelService::SetPhoneInCall(bool aActive)
>  {
>    //while ring tone and in-call mode, mute media element
> +  if (aActive && mChannelCounters[AUDIO_CHANNEL_TELEPHONY].IsEmpty()) {

Leave a comment, (and file a followup if you are so inclined) on that this should ideally pass in a appid too.

@@ +313,5 @@
> +  for (int32_t type = AUDIO_CHANNEL_NORMAL;
> +         type <= AUDIO_CHANNEL_PUBLICNOTIFICATION;
> +         ++type) {
> +    while (mChannelCounters[type].Contains(aAppId)) {
> +      mChannelCounters[type].RemoveElement(aAppId);

Not a huge deal, but a faster/cleaner solution would be to add

nsTArray::RemoveAllElements(value)

Actually, a pair of them, one with a comparator and one using the default comparator. See how RemoveElement(...) is implemented.

But not a huge deal.

::: dom/ipc/TabParent.cpp
@@ +159,5 @@
> +
> +  AudioChannelService *service = AudioChannelService::GetAudioChannelService();
> +  if (service) {
> +    service->RemoveAppId(OwnAppId());
> +  }

Justin should review this change

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 692291 [details] [diff] [review]
patch

Justin: Can you review the TabParent.cpp change? Is that the only place where we need to do cleanup for a child process that went away?

Comment 15

[Justin Lebar (not reading bugmail)]

> Is that the only place where we need to do cleanup for a child process that went away?

Yes, I'm pretty sure that's fine.  I'd prefer if it was loosely-coupled, however -- that is, if we fired an observer instead of sticking the audio manager code into TabParent.

But I'm confused about what this patch is doing.  It seems like we're using appIds as a way to identify PBrowsers?  But that's not really correct.  In particular, multiple processes could all have appId 0.

What are we using the appId for?

> void
> AudioChannelService::SetPhoneInCall(bool aActive)
> {
>   //while ring tone and in-call mode, mute media element
>-  if (aActive) {
>-    mChannelCounters[AUDIO_CHANNEL_TELEPHONY] = 1;
>-  } else {
>-    mChannelCounters[AUDIO_CHANNEL_TELEPHONY] = 0;
>+  if (aActive && mChannelCounters[AUDIO_CHANNEL_TELEPHONY].IsEmpty()) {
>+    mChannelCounters[AUDIO_CHANNEL_TELEPHONY].AppendElement(0);
>+  } else if(!aActive && !mChannelCounters[AUDIO_CHANNEL_TELEPHONY].IsEmpty()) {
>+    mChannelCounters[AUDIO_CHANNEL_TELEPHONY].RemoveElement(0);

Is this safe given the implementation of RemoveAppId()?  That is, what happens
if I call RemoveAppId(0) while we're in an active call?

Comment 16

[Andrea Marchesini [:baku]]

> But I'm confused about what this patch is doing.  It seems like we're using
> appIds as a way to identify PBrowsers?  But that's not really correct.  In
> particular, multiple processes could all have appId 0.

Basically the idea is that when an app quits, we have to remove any count/reference to its audio channel components.

> Is this safe given the implementation of RemoveAppId()?  That is, what
> happens
> if I call RemoveAppId(0) while we're in an active call?

0 is not allowed and the telephony channel is used by appId 0.

Comment 17

[Justin Lebar (not reading bugmail)]

> 0 is not allowed and the telephony channel is used by appId 0.

I don't see anything preventing the TabParent code from calling this function with appId == 0, though.  OwnAppId() may be 0.

> Basically the idea is that when an app quits, we have to remove any count/reference to 
> its audio channel components.

A TabParent being destroyed is not really the same thing as an app quitting.  For example, the browser app runs inside the main process and doesn't have a TabParent.  Are we handling this properly?

+++ b/dom/ipc/PContent.ipdl
> +    async AudioChannelRegisterType(AudioChannelType aType, uint32_t aAppId);
> +    async AudioChannelUnregisterType(AudioChannelType aType, uint32_t aAppId);

Do we want to be trusting the child to supply correct appIds here?  Perhaps a better way to do this would be to send PBrowser's through the IPDL interface.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Justin Lebar [:jlebar] from comment #17)
> > 0 is not allowed and the telephony channel is used by appId 0.
> 
> I don't see anything preventing the TabParent code from calling this
> function with appId == 0, though.  OwnAppId() may be 0.

When can OwnAppId()==0 in B2G? Should we simply not call RemoveAppId if AppId returns 0?

> > Basically the idea is that when an app quits, we have to remove any count/reference to 
> > its audio channel components.
> 
> A TabParent being destroyed is not really the same thing as an app quitting.
> For example, the browser app runs inside the main process and doesn't have a
> TabParent.  Are we handling this properly?

I think we are. The normal shutdown path is that all media elements unregister with the AudioChannelService in their process when they are stopped.

The problem this bug is covering is when we don't shut down the normal way, such as when a child process is killed.

> +++ b/dom/ipc/PContent.ipdl
> > +    async AudioChannelRegisterType(AudioChannelType aType, uint32_t aAppId);
> > +    async AudioChannelUnregisterType(AudioChannelType aType, uint32_t aAppId);
> 
> Do we want to be trusting the child to supply correct appIds here?  Perhaps
> a better way to do this would be to send PBrowser's through the IPDL
> interface.

I don't care strongly. We haven't secured the audio channels code against child processes getting hacked so far. I suggest we do that as a followup since the failure case isn't bad enough to be blocking IMO.

Comment 19

[Justin Lebar (not reading bugmail)]

> When can OwnAppId()==0 in B2G?

A browser tab process has OwnAppId() == 0.  See the comments in TabContext.h for an explanation of what these attributes mean.

I don't think we ever have OwnOrContainingAppId() == 0 in B2G.  I think that might be correct for your purposes here, but it would break with nested content processes, because then you could have two processes with the same OwnOrContainingAppId()'s.

> The problem this bug is covering is when we don't shut down the normal way, such as when a child 
> process is killed.

Then should we run this code only when the child process shuts down abnormally?

I still feel like using AppIds to identify TabParent objects is a hack.  I'm not in a position to judge whether we need to accept technical debt to Get This Fixed Now, but in my experience, these follow-ups are rarely done, and the debt is rarely repaid.

Comment 20

[Justin Lebar (not reading bugmail)]

If you do switch to OwnOrContianingAppId(), please add an assertion in the code that the ID you get is not 0, if the correctness of the code relies on that.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, yes, I see what you mean that using appids rather than TabParents is the wrong thing.

In particular, if we have two child processes that both are running pages from the same appid then things break down.

I can think of three ways forward:

1. Use the current solution for now.
2. Rather than sending AudioChannelRegisterType/AudioChannelUnregisterType
   messages to the parent process, create a new protocol object and send
   constructors and destructors. When the child process goes away the protocol
   object will automatically be destroyed and so will take care of unregistering
   automatically.
3. Keep AudioChannelRegisterType/AudioChannelUnregisterType, but send a
   TabParent as the second argument rather than an appid. Then track things
   by TabParent in the AudioChannelService. When the TabParent goes away we
   tell the AudioChannelService and it'll unregister things keyed on TabParent
   rather than appid.

Comment 22

[Justin Lebar (not reading bugmail)]

> In particular, if we have two child processes that both are running pages from the same appid then 
> things break down.

Yes, exactly.  And at the very least, we spawn multiple browser tab processes, all with OwnAppId == 0 (and with the same OwnOrContainingAppId(), corresponding to that of the browser app).

Comment 23

[Andrea Marchesini [:baku]]

Attachment: patch a

Justin, can you take a look of this usage of mChildID ?

Comment 24

[Andrea Marchesini [:baku]]

Attachment: patch a

sorry, wrong page. Can you review this?

Comment 25

[Justin Lebar (not reading bugmail)]

Comment on attachment 694499 [details] [diff] [review]
patch a

> void
>-AudioChannelService::UnregisterType(AudioChannelType aType)
>+AudioChannelService::UnregisterType(AudioChannelType aType, uint64_t aChildID)
> {
>-  mChannelCounters[aType]--;
>-  MOZ_ASSERT(mChannelCounters[aType] >= 0);
>+  // The array may contain multiple occurrence of this appId but
>+  // this should remove only the first one.
>+  mChannelCounters[aType].RemoveElement(aChildID);

Update this comment?

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
>--- a/dom/ipc/TabParent.cpp
>+++ b/dom/ipc/TabParent.cpp

>@@ -150,16 +151,22 @@ TabParent::ActorDestroy(ActorDestroyReas
>     if (why == AbnormalShutdown) {
>       nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>       if (os) {
>         os->NotifyObservers(NS_ISUPPORTS_CAST(nsIFrameLoader*, frameLoader),
>                             "oop-frameloader-crashed", nullptr);
>       }
>     }
>   }
>+
>+  AudioChannelService *service = AudioChannelService::GetAudioChannelService();
>+  if (service) {
>+    ContentParent* cp = static_cast<ContentParent*>(Manager());
>+    service->RemoveChildID(cp->ChildID());
>+  }

This assumes that the ContentChild has died any time we destroy a TabParent,
but that's not right.  For example, if we have two windows in a process (e.g. a
main window and a popup window generated via window.open), each window gets its
own TabParent, and we can close one TabParent without killing the whole
process.

I think you want ContentParent::ActorDestroy.

But then notice that ContentParent::ActorDestroy already notifies the observer service:

    obs->NotifyObservers((nsIPropertyBag2*) props, "ipc:content-shutdown", nullptr);

and one of the properties is the childID:

    props->SetPropertyAsUint64(NS_LITERAL_STRING("childID"), mChildID);

So I think you can just listen to ipc:content-shutdown.

The changes in ContentParent.{cpp,h} look fine to me, although I'm not positive cjones is OK with me reviewing this stuff.  I have a message into him.  But I guess if you make this change you won't need to modify anything other than the AudioChannel call in ContentParent.

Comment 26

[Andrea Marchesini [:baku]]

Attachment: patch b

Wow, I love this observer approach. Code is much more readable.
Justin, give me a feedback. Then I'll ask cjones for reviewing the ContentParent part, and then Jonas will review the rest.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 694756 [details] [diff] [review]
patch b

Maybe chris can review the use of the new ContentParent::ChildId function here.

Chris: The goal of this patch is to handle unregistering media elements (and other sound sources) in child processes that crash or are otherwise killed.

Comment 28

[Justin Lebar (not reading bugmail)]

I'd really prefer to have a look at this, if I can; I've found non-trivial bugs in the past few similar patches I've reviewed.

Are you OK waiting until I have time, Jonas?

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 694756 [details] [diff] [review]
patch b

Sure, that's ok. We are getting a non-trivial amount of dupes/dependencies though, so please put this high on your list.

Comment 30

[Justin Lebar (not reading bugmail)]

Comment on attachment 694756 [details] [diff] [review]
patch b

When have I ever left you hanging for a review, Jonas?  :)

>@@ -293,8 +291,41 @@ AudioChannelService::ChannelName(AudioCh
>+NS_IMETHODIMP
>+AudioChannelService::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* data)
>+{
>+  MOZ_ASSERT(!strcmp(aTopic, "ipc:content-shutdown"));
>+
>+  nsCOMPtr<nsIPropertyBag2> props = do_QueryInterface(aSubject);
>+  if (!props) {
>+    NS_WARNING("ipc:content-shutdown message without property bag as subject");
>+    return NS_OK;
>+  }
>+
>+  uint64_t childID = 0;
>+  nsresult rv = props->GetPropertyAsUint64(NS_LITERAL_STRING("childID"),
>+                                           &childID);
>+  if (NS_SUCCEEDED(rv)) {
>+    for (int32_t type = AUDIO_CHANNEL_NORMAL;
>+         type <= AUDIO_CHANNEL_PUBLICNOTIFICATION;
>+         ++type) {
>+      while (mChannelCounters[type].Contains(childID)) {
>+        mChannelCounters[type].RemoveElement(childID);
>+      }

Not to prematurely optimize, but this is a pretty inefficient way of doing
this.  Perhaps you could could at least do an IndexOf and then remove that
index.

>+    // We don't have to remove the agents from the mAgents hashtable because if
>+    // that table contains only agents running on the same process.

Fix this comment (the "if"), please.

r=me; thanks for waiting.  :)

Comment 31

[Andrea Marchesini [:baku]]

Attachment: patch c

Thanks for this review. After green on try, I mark this patch for checking in.

Comment 32

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=68ec7c07711d green on try

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08d43728e882

Comment 35

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/08d43728e882

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b2296f9aaa55
https://hg.mozilla.org/releases/mozilla-b2g18/rev/38f7f302385e