980270 - ICE leaks many allocations

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P3)

Description

[Randell Jesup [:jesup] (needinfo me)]

Lots and lots of things get leaked in ICE/etc

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: lsan leak logs (with patches for voiceengine leaks and some fixes for SIPCC dcb leaks)

Comment 2

[Byron Campen [:bwc]]

So, I suspect that the following line is causing a leak when ctx->response is already set:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c#524

Comment 3

[Byron Campen [:bwc]]

nr_ice_pre_answer_request_destroy doesn't seem to be freeing the buffer created here:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c?from=nr_ice_pre_answer_request_destroy#60
http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c?from=nr_ice_pre_answer_request_destroy#85

Comment 4

[Byron Campen [:bwc]]

The rest of the nICEr stuff in this trace is related to the nr_registry, which is set up once but never torn down. Do we want to put priority on cleaning this up on shutdown? I don't see a whole lot of reason to expend extra effort to clean this up when all NrIceCtx are gone (prior to shutdown), since it looks like tens of K, but if it is easier/cleaner this way it is an option.

Comment 5

[Byron Campen [:bwc]]

Attachment: Part 1: Plug a couple of common leaks in nICEr.

First attempt at plugging the recurring leaks.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

SUMMARY: LeakSanitizer: 198910 byte(s) leaked in 1824 allocation(s).
That's down in the same test from ~360K/2000, and no errors flagged.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: y11

Comment 8

[Byron Campen [:bwc]]

Do we want to spend the time to clean up the one-time nr_registry leaks?

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

There seem to be 3x 32K+delta allocs there (perhaps some other smaller ones).  Note that due to jemalloc, the 'slop' for a 32K + delta allocation is almost 32K.... so these are poor sizes to begin with.

Given at least 100K is locked in use after the first ref to ICE, and probably closer to 200K, I'd like to see them released when we're not using them (when we shut down peerconnections), even if it's off a timer to avoid churn or some such (I don't think a timer is needed).  I'm not so concerned with releasing them in shutdown; if that's all we should just add entries in the suppression list for lsan (see the grandparent bug to this one on lsan for suppression info IIRC).

Also: are these registry sizes appropriate for this use?  they seem large for a browser that at most might have a handful of sessions active (doubly so with bundle/rtcpmux)

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

slop here is more like 3*(1 page - delta) apparently.  Still we're well over 100K total.

Comment 11

[Byron Campen [:bwc]]

(In reply to Randell Jesup [:jesup] from comment #9)
> There seem to be 3x 32K+delta allocs there (perhaps some other smaller
> ones).  Note that due to jemalloc, the 'slop' for a 32K + delta allocation
> is almost 32K.... so these are poor sizes to begin with.
> 
> Given at least 100K is locked in use after the first ref to ICE, and
> probably closer to 200K, I'd like to see them released when we're not using
> them (when we shut down peerconnections), even if it's off a timer to avoid
> churn or some such (I don't think a timer is needed).  I'm not so concerned
> with releasing them in shutdown; if that's all we should just add entries in
> the suppression list for lsan (see the grandparent bug to this one on lsan
> for suppression info IIRC).
> 
> Also: are these registry sizes appropriate for this use?  they seem large
> for a browser that at most might have a handful of sessions active (doubly
> so with bundle/rtcpmux)

This is configuration setup and setting up the logging system/s, so the max number of ICE contexts isn't really a factor here.

Comment 12

[Byron Campen [:bwc]]

Attachment: Part 2: (WIP) Plug one-time leak of nr_registry and r_log.

Begin experimenting with cleaning up nr_registry and r_log.

Comment 13

[Byron Campen [:bwc]]

Try out patch #2 and see how much it helps.

Comment 14

[Eric Rescorla (:ekr)]

(In reply to Byron Campen [:bwc] from comment #4)
> The rest of the nICEr stuff in this trace is related to the nr_registry,
> which is set up once but never torn down. Do we want to put priority on
> cleaning this up on shutdown? I don't see a whole lot of reason to expend
> extra effort to clean this up when all NrIceCtx are gone (prior to
> shutdown), since it looks like tens of K, but if it is easier/cleaner this
> way it is an option.

I don't see a lot of value in clenaning up the shutdown stuff.

Comment 15

[Nils Ohlmeier [:drno]]

Comment on attachment 8387137 [details] [diff] [review]
Part 1: Plug a couple of common leaks in nICEr.

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

LGTM

With this patch applied a three hour call as a TURN client no longer showed a memory leak.

Comment 17

[Nils Ohlmeier [:drno]]

I think this is worth considering uplifting as this leak consumes ~1MB per second per stream if the TURN relay of your client gets selected by ICE.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Do we have a Try link for this? :)

Comment 19

[Nils Ohlmeier [:drno]]

Try run for Part 1 patch: https://tbpl.mozilla.org/?tree=Try&rev=52424c947a91

Comment 20

[Nils Ohlmeier [:drno]]

Try run looks green to me. Requesting checkin.

Comment 21

[Andrew McCreight [:mccr8]]

Thanks for fixing this!  Hopefully we're a little closer to being able to do more targeted LSan whitelisting of WebRTC leaks.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54ee8a6f85a4

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/54ee8a6f85a4

Comment 24

[Nils Ohlmeier [:drno]]

Comment on attachment 8387137 [details] [diff] [review]
Part 1: Plug a couple of common leaks in nICEr.

Approval Request Comment
[Feature/regressing bug #]: TURN support is available at least since FF23; 786235 is oldest ticket I was able to find regarding TURN support.

[User impact if declined]: Firefox might get killed by the OS in a WebRTC call if the host runs out of memory (this eats at around 1MB/s if the scenario applies).

[Describe test coverage new/current, TBPL]: No TBPL coverage as TBPL allows no proper TURN testing. This was caught in the WebRTC QA lab and can be verified with that as well. Additionally manual verification has been done and can be done as needed.

[Risks and why]: low risk - we are freeing memory which should not be used any more; the same function for freeing the memory is in use in other places in the code and has not caused problems so far.

[String/UUID change made/needed]: None.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d8e7408cb510

Comment 26

[Andrew McCreight [:mccr8]]

I'm not sure how verifiable this really is.  This code still leaks, just less.

Comment 27

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Andrew McCreight [:mccr8] from comment #26)
> I'm not sure how verifiable this really is.  This code still leaks, just
> less.

Thanks Andrew for the info! Removing keyword for now... add it back if you think the QA team can help with this.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/d8e7408cb510

Comment 29

[Sue]

Could you provide the detailed reproduce steps and video for me to verify this bug. Thanks!

Comment 30

[Nils Ohlmeier [:drno]]

(In reply to Sue from comment #29)
> Could you provide the detailed reproduce steps and video for me to verify
> this bug. Thanks!

There is no specific video. You would need to establish a call between two different networks, so that the call goes through a TURN relay. And then leave the call up for quite some time and watch the memory usage.