843417 - LMK will never kill the Musicplayer app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Michael Schwartz [:m4]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.57 Safari/537.17

Steps to reproduce:

If the MusicPlayer app is playing music when it goes to the background, it will get the background perceivable adj value.


Actual results:

When the app is done playing music, it keeps this priority which is higher than the homescreen - it will never be killed by LMK.  This results in fewer useful apps being kept in the background which is especially relevant on devices with a small memory footprint and one without background app aging.


Expected results:

adj value of the music player should be downgraded to background.

Comment 1

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

Oh yuck ... I hope we didn't regress this recently.

Nice find!

Comment 2

[Justin Lebar (not reading bugmail)]

> I hope we didn't regress this recently.

Is it better if it never worked?  :)

Comment 3

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

Slightly, yeah ;).

Comment 4

[Justin Lebar (not reading bugmail)]

It looks like the AudioChannelService has some major issues.  I'm seeing the last line here

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

return false, which indicates that aChildID is not in mChannelCounters[type].  I presume that the child is hanging out in one of the other mChannelCounters, which is what's causing us to be stuck in the fg.

Comment 5

[Justin Lebar (not reading bugmail)]

I still can't see what's going wrong here, but njn will be happy to have yet another example of counters kicking our butts.  :)

Comment 6

[Justin Lebar (not reading bugmail)]

I wonder how hard it would be to compute mChannelCounters on demand, instead of keeping track of it manually (and somehow messing up).  If we have hundreds of audio channels we have another problem entirely, so computing this array from the hashtable of audio channels should be fast.

Comment 7

[Justin Lebar (not reading bugmail)]

I can continue looking into this, but I have other bugs I could look at, so feel free to steal this from me.

Comment 8

[Andrea Marchesini [:baku]]

(In reply to Justin Lebar [:jlebar] from comment #6)
> I wonder how hard it would be to compute mChannelCounters on demand, instead
> of keeping track of it manually (and somehow messing up).  If we have
> hundreds of audio channels we have another problem entirely, so computing
> this array from the hashtable of audio channels should be fast.

What do you mean with 'computing this array from the hashtable' ?
Unfortunately these 2 data structs contain different information. The hashtable of the agents is used only for the local agents (per process). The mChannelCounters contains a map of any audio channel in the system.

This means that the process A has an hashtable with its agents and the main process don't know them at all.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 10

[Justin Lebar (not reading bugmail)]

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

f=me, but I really think we should add these RemoveElement assertions; see bug 844765 comment 5.

Comment 11

[Justin Lebar (not reading bugmail)]

> What do you mean with 'computing this array from the hashtable' ?

I clearly had no idea what was going on here; thanks for figuring it out!  :)

Comment 12

[Marco Chen [:mchen]]

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

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

Looks good.

::: dom/audiochannel/AudioChannelService.cpp
@@ +143,5 @@
> +                                       uint64_t aChildID,
> +                                       bool aElementHidden,
> +                                       bool aElementWasHidden)
> +{
> +  // Calculating the new and old type and update the hashtable if needed.

About comment, please refer to bug 844765 comment 5 from Justin.

Comment 13

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #10)
> Comment on attachment 717954 [details] [diff] [review]
> patch
> 
> f=me, but I really think we should add these RemoveElement assertions; see
> bug 844765 comment 5.

The reason for this bug is that AudioChannelServiceChild didn't update count of each channels. So Can the assertions of RemoveElement help to find out this issue?
Or just to make sure that count is updated correctly without fail of RemoveElement.

Comment 14

[Justin Lebar (not reading bugmail)]

> The reason for this bug is that AudioChannelServiceChild didn't update count of each 
> channels. So Can the assertions of RemoveElement help to find out this issue?

What I saw in my (limited) testing was that, when we went to remove the audio channel (because e.g. I paused the music), we'd try to remove the child-id from the bucket for the wrong internal type.  That bucket would be empty, so the RemoveElement() call would silently do nothing.  But there should always be something for us to remove when we call RemoveElement; otherwise, I think that's indicative of a bug.

It's quite possible I'm missing something here, but that's what it seems like to me, anyway.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 16

[Justin Lebar (not reading bugmail)]

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

Are you planning to do the RemoveElement checks in a separate bug?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe6e46c05587

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fe6e46c05587

Comment 19

[Andrea Marchesini [:baku]]

> Are you planning to do the RemoveElement checks in a separate bug?

I think this is enough:
MOZ_ASSERT(!mChannelCounters[type].IsEmpty());

.. or maybe you want some additional check?

Comment 20

[Justin Lebar (not reading bugmail)]

(In reply to Andrea Marchesini (:baku) from comment #19)
> > Are you planning to do the RemoveElement checks in a separate bug?
> 
> I think this is enough:
> MOZ_ASSERT(!mChannelCounters[type].IsEmpty());
> 
> .. or maybe you want some additional check?

That's not as strong as ASSERT(mChannelCounters[type].RemoveElement(childID)), right?  Also, I'd like us to make noise if this fails in both debug and release builds.  Since nobody runs debug builds of B2G, your chances of discovering that your assertion is failing are pretty low if you don't log something when the assertion fails in a release build.