Closed Bug 1044514 Opened 5 years ago Closed 5 years ago

b2g process is leaking memory for webaudio

Categories

(Core :: Web Audio, defect, P1, blocker)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla34
blocking-b2g 2.0+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: tkundu, Assigned: baku)

References

Details

(Keywords: memory-footprint, perf, Whiteboard: [caf priority: p1][CR 691739][MemShrink])

Attachments

(3 files, 1 obsolete file)

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
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.0?
Whiteboard: [CR 1041751]
Whiteboard: [CR 1041751] → [CR 691739]
Summary: b2g process s leaking memory for webaudio → b2g process is leaking memory for webaudio
Whiteboard: [CR 691739] → [caf priority: p1][CR 691739]
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?
Flags: needinfo?(ehsan)
njn will be happy to see that those reporters proved useful.
Whiteboard: [caf priority: p1][CR 691739] → [caf priority: p1][CR 691739][MemShrink]
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 ?
Flags: needinfo?(khuey)
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.
(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
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
> njn will be happy to see that those reporters proved useful.

erahm even more so.
(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
I'm guessing the dialer is failing to remove these audio nodes, but I could be wrong.
Component: Gaia::Video → Gaia::Dialer
_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.
Keywords: footprint, perf
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Flags: needinfo?(ehsan)
So the issue here is that AudioDestinationNode is registered as an event listener, which screws up AudioNode's attempts to disconnect itself.
Component: Gaia::Dialer → Web Audio
Flags: needinfo?(khuey)
Product: Firefox OS → Core
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>
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.
Flags: needinfo?(amarchesini)
blocking-b2g: 2.0? → 2.0+
Blocks: 874508
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch wa.patch (obsolete) — Splinter Review
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.
Attachment #8463858 - Flags: review?(ehsan)
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.
Attachment #8463858 - Flags: review?(ehsan) → review+
Attached patch wa.patchSplinter Review
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ab8cefd61315
Attachment #8463858 - Attachment is obsolete: true
Andrea -- can you please share a 2.0 rebased patch asap. We want to give it a try in our stability testing.
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
(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 :)
Flags: needinfo?(tkundu)
https://hg.mozilla.org/mozilla-central/rev/e8a6a7a22125
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
we are not seeing this anymore. Thanks for your help in this.
Flags: needinfo?(tkundu)
Flags: in-moztrap?(ychung)
Not enough information on this bug to create a valid test case.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(ychung)
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.