1034327 - Memory leak with TURN relay

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Nils Ohlmeier [:drno]]

If FF Nightly is wrapping its media in TURN packages it consumes big amount of memory.
In our SDE environment FF gets reproducible killed 35min into the call, because the host runs out of memory.

Comment 1

[Eric Rescorla (:ekr)]

Can you do a 1-way call where the Nightly is the sender and see if this still happens?

Comment 2

[Nils Ohlmeier [:drno]]

Running FF in Valgrind was way to slow for any ICE connection to get established.
Now trying TURN unit tests in valgrind...

Comment 3

[Byron Campen [:bwc]]

We probably need to be using nr_stun_message_destroy here:

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

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: bug_1034327_fix_turn_memory_leaks.patch

This fixes the major leak I see in the turn_unitest.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8450658 [details] [diff] [review]
bug_1034327_fix_turn_memory_leaks.patch

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

Looks good to me; we definitely call this if nr_stun_build_send_indication fails, so it seems right. Test, of course.

Comment 6

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

I presume we'll want to uplift this as far as possible?

Comment 7

[Nils Ohlmeier [:drno]]

(In reply to Randell Jesup [:jesup] from comment #6)
> I presume we'll want to uplift this as far as possible?

Indeed.

I want to throw a manual build with the fix onto the sunny day QA lab on Monday for a 3 hour call to verify this is really the only fix needed.

Comment 8

[Nils Ohlmeier [:drno]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=dece9840ae3a

Comment 9

[Nils Ohlmeier [:drno]]

3 hour test run did not fail after 30min like it used to do. But there might be another smaller memory leak.

Comment 10

[Nils Ohlmeier [:drno]]

I looks like we still have a smaller leak somewhere:

Client 1:
 4123 mozilla   20   0 1050m 178m  46m S   70  4.6  72:51.88 firefox

Client 2 (the one which uses send indication):
6214 mozilla   20   0 1692m 578m  10m S   74 14.8  80:34.06 firefox

Unfortunately this leak does not show up in the turn_unittest, and is probably a lot harder to find. I opened Bug 1035332 to continue with that investigation, but get this fix landed earlier.

As the test run is green I'm requesting checkin.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1f2cc228a2

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c1f2cc228a2

Comment 13

[Nils Ohlmeier [:drno]]

Comment on attachment 8450658 [details] [diff] [review]
bug_1034327_fix_turn_memory_leaks.patch

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.

[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 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8450658 [details] [diff] [review]
bug_1034327_fix_turn_memory_leaks.patch

Aurora+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/529c1ec49ed0

Might be worth taking on older branches as well?

Comment 16

[Nils Ohlmeier [:drno]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> Might be worth taking on older branches as well?

Yes, I just wanted to do it one by one to see if I encounter any problems with the approvals in between, and because the guidelines say that the code should sit and rest in Nightly & Aurora for some time before uplifting into Beta.

Comment 17

[Nils Ohlmeier [:drno]]

Comment on attachment 8450658 [details] [diff] [review]
bug_1034327_fix_turn_memory_leaks.patch

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 least 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 18

[Nils Ohlmeier [:drno]]

My assumption is that, because of the lack of video support for WebRTC on B2G < 2.0 the usage of WebRTC is so low on B2G (this might be totally wrong), that it is not worth trying to fix it.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Nils, can you give me some clearer instructions on how I can verify this manually? Thanks!

Comment 21

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

(Or, if it's something you're set up better for, ie to do memory leak measurements, can you verify this fix? Thanks!)

Comment 22

[Nils Ohlmeier [:drno]]

It is not that easy to reproduce, because the problem occurs only in a special network setup.
I took the 31 rc1 and made a manual call in our QA lab setup. No memory leak visible any more after running the call for 10min (without the patch the memory footprint used to increase every second).

Comment 23

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Thank you Nils!