1044514 - b2g process is leaking memory for webaudio

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: Diff between about memory 0 and about memory 23

STR : 
1) place a call and enable automatic answering in other end
2) Play sdcard video (We used only 3 videos on sdcard)
3) End call and kill all running apps using |gcli killallapps|.
4) Receive call on testphone continuously. 
4) Repeat 1,2,3,4  for at least 12 hours 



about-memory-23 is collected at timestamp 2014-07-26 19:13:20 and You can see device activity in logcat(or any logs) during this timestamp.

I also attached diff between about-memory-0 and about-memory-23.
From gecko memory report , it seems like b2g is leaking webaudio and b2g has allocated 47MB additional memory between about-memory-23 and about-memory-0.


b2g USS hits >80MB after 12hours of running stability test on 256MB target.

FULL Logs: https://drive.google.com/file/d/0B1cSMS8_GuAESTVSTG5saE5NYWc/edit?usp=sharing

Comment 1

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

[Blocking Requested - why for this release]:

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ehsan, can you give us a bit of background here?  Is it possible for a child process to cause webaudio memory usage in the parent?  Or does this have to be coming from the system app?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

njn will be happy to see that those reporters proved useful.

Comment 4

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

I saw that webaudio allocations are increasing in about-memory-23 but DMD report does not show anything about it (See log in Comment 0). Any ideas on it ?

Comment 5

[Andrew McCreight [:mccr8]]

DMD only gives information for allocations under heap-unclassified, so it would not include anything for the web audio allocations that increased.

Probably the most useful thing would be to get cycle collector and garbage collector logs, which I think are grabbed by the standard scripts used to pull about:memory logs off of the phones.  Hopefully the web audio nodes would show up there, and then we could figure out why they are alive and maybe more about them.

Comment 6

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> DMD only gives information for allocations under heap-unclassified, so it
> would not include anything for the web audio allocations that increased.
> 
> Probably the most useful thing would be to get cycle collector and garbage
> collector logs, which I think are grabbed by the standard scripts used to
> pull about:memory logs off of the phones.  Hopefully the web audio nodes
> would show up there, and then we could figure out why they are alive and
> maybe more about them.
You can find cycle collector and garbage collector logs in the log location pointed by us in Comment 0

Comment 7

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

In Comment 0, I am seeing b2g USS has increased to >80MB. 

We also noticed that b2g VSS has increased from 243396KB (timestamp 2014-07-26 09:59:15, about-memory-0) to 298108K (timestamp 2014-07-26 19:13:19, about-memory-23) . So the VSS increment is  

So VSS Increase is also mostly 54712KB or 53MB. I am seeing that b2g process has 31 AudioTrack thread [1] at timestamp 2014-07-26 19:13:19(about-memory-23)


Can we find out why we have 31 AudioTrack threads at timestamp 2014-07-26 19:13:19(about-memory-23). I guess that this will also answer USS growth of b2g process. 


b2g process smaps logs for this stability run is in : https://drive.google.com/file/d/0B1cSMS8_GuAEUDEyYWhnOU8yNlE/edit?usp=sharing

Comment 8

[Nicholas Nethercote [inactive]]

> njn will be happy to see that those reporters proved useful.

erahm even more so.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Tapas Kumar Kundu from comment #6)
> You can find cycle collector and garbage collector logs in the log location
> pointed by us in Comment 0

I missed that, thanks.  I downloaded the logs, and looked at the last one, about-memory-23.  Because of bug 1026713, these all show up as DOMEventTargetHelper, but fortunately destination nodes have the somewhat unique field mAudioChannelAgent.

It looks like these AudioDestinationNodes are from app://callscreen.gaiamobile.org/index.html#&timestamp=1406393995221 There are 319 DETH from that URL in the log.

I looked at two of these nodes, and the common part of the path that is holding them alive is:

0xae2ed9a0 [nsGlobalWindow #15 inner app://callscreen.gaiamobile.org/index.html#&timestamp=1406393995221]
    --[mListenerManager]--> 0xab1fe700 [EventListenerManager]
    --[mListeners event=onvisibilitychange listenerType=0 [i]]--> 0xac7b6670 [DOMEventTargetHelper app://callscreen.gaiamobile.org/index.html#&timestamp=1406393995221]

(various things are holding the global window alive)

AudioDestination nodes seem to be created in the dialer here:
  https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/audio_competing_helper.js#L75

Comment 10

[Andrew McCreight [:mccr8]]

I'm guessing the dialer is failing to remove these audio nodes, but I could be wrong.

Comment 11

[Karl Tomlinson (:karlt)]

_silenceBufferSource.stop(0) should be enough to release the BufferSource (when the AudioCompetingHelper no longer holds a reference) and so the AudioContext with its AudioDestinationNode.

The code seems to be written assuming that compete() and leaveCompetition() are always balanced.

As AudioCompetingHelper is a singleton, it would be a little more efficient to release the reference to _silenceBufferSource, after stop(), in leaveCompetition().
Then it would be easier to assert that _silenceBufferSource is always null when compete() is called.

Comment 12

[(no longer active)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> Ehsan, can you give us a bit of background here?  Is it possible for a child
> process to cause webaudio memory usage in the parent?

No, it shouldn't be possible.

> Or does this have to be coming from the system app?

Most likely, yes.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So the issue here is that AudioDestinationNode is registered as an event listener, which screws up AudioNode's attempts to disconnect itself.

Comment 14

[(no longer active)]

So this event listener holds the destination node, and by extension, everything connected to it, alive forever. <http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/AudioDestinationNode.cpp#309>

Comment 15

[(no longer active)]

Andrea, can you take this please?

In order to fix this, you may want to create a proxy event handler object which holds a weak reference to the destination node, or something.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: wa.patch

The only problem about using a weakreference is that we cannot use them in the constructor. This because of this:

https://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsWeakReference.cpp#77

where nsCOMPtr is used. But in the constructor we have refcnt 0, and when this factoryPtr is destroyed the AudioDestinationNode is destroyed too.
I moved the initialization of the AudioChannel in a separated method, called in AudioContext.

Comment 17

[(no longer active)]

Comment on attachment 8463858 [details] [diff] [review]
wa.patch

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

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +293,2 @@
>  NS_IMPL_CYCLE_COLLECTION_INHERITED(AudioDestinationNode, AudioNode,
>                                     mAudioChannelAgent)

You need to tell the cycle collector about mEventProxyHelper.

@@ +591,5 @@
> +    return;
> +  }
> +
> +  if (!mEventProxyHelper) {
> +    nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(GetOwner());

Please add a comment explaining why we are using the event proxy handler here.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: wa.patch

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ab8cefd61315

Comment 19

[Andrea Marchesini [:baku]]

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

Comment 20

[Inder]

Andrea -- can you please share a 2.0 rebased patch asap. We want to give it a try in our stability testing.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: patch for b2g32_v2.0

Comment 22

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Andrea Marchesini (:baku) from comment #21)
> Created attachment 8464128 [details] [diff] [review]
> patch for b2g32_v2.0

Thanks for your help here. We started running automation with your patch.We will provide feedback soon :)

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2bae166f13c9

Comment 25

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

we are not seeing this anymore. Thanks for your help in this.

Comment 26

[Yeojin Chung [:YeojinC]]

Not enough information on this bug to create a valid test case.