Closed Bug 843417 Opened 9 years ago Closed 9 years ago

LMK will never kill the Musicplayer app

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mschwart, Assigned: baku)

Details

Attachments

(1 file, 1 obsolete file)

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.
Oh yuck ... I hope we didn't regress this recently.

Nice find!
Status: UNCONFIRMED → NEW
Ever confirmed: true
> I hope we didn't regress this recently.

Is it better if it never worked?  :)
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.
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.  :)
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.
I can continue looking into this, but I have other bugs I could look at, so feel free to steal this from me.
Assignee: nobody → justin.lebar+bug
(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.
Attached patch patch (obsolete) — Splinter Review
Attachment #717954 - Flags: review?(mchen)
Attachment #717954 - Flags: feedback?(justin.lebar+bug)
Assignee: justin.lebar+bug → amarchesini
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.
Attachment #717954 - Flags: feedback?(justin.lebar+bug) → feedback+
> 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 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.
Attachment #717954 - Flags: review?(mchen) → review+
(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.
> 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.
Attached patch patchSplinter Review
Attachment #717954 - Attachment is obsolete: true
Attachment #718418 - Flags: review+
Keywords: checkin-needed
Comment on attachment 718418 [details] [diff] [review]
patch

Are you planning to do the RemoveElement checks in a separate bug?
https://hg.mozilla.org/mozilla-central/rev/fe6e46c05587
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
> 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?
(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.
You need to log in before you can comment on or make changes to this bug.