1040345 - GMP automated tests leak GeckoMediaPluginService

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Benjamin Smedberg]

Test-running the GMP tests, we leak the GeckoMediaPluginService:


 3:20.57 nsTraceRefcnt::DumpStatistics: 1200 entries
 3:20.57 TEST-INFO | leakcheck | leaked 1 GeckoMediaPluginService (40 bytes)
TEST-INFO | leakcheck | leaked 2 Mutex (24 bytes)
TEST-INFO | leakcheck | leaked 1 ReentrantMonitor (16 bytes)
TEST-INFO | leakcheck | leaked 2 nsTArray_base (8 bytes)
TEST-INFO | leakcheck | leaked 1 nsThread (124 bytes)
 3:20.58 TEST-UNEXPECTED-FAIL | leakcheck | 212 bytes leaked (GeckoMediaPluginSe
rvice, Mutex, ReentrantMonitor, nsTArray_base, nsThread)
 3:20.58 runtests.py | Running tests: end.
 3:20.71 TEST-UNEXPECTED-FAIL | leakcheck | 212 bytes leaked (GeckoMediaPluginSe
rvice, Mutex, ReentrantMonitor, nsTArray_base, nsThread)

Comment 1

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

Attachment: WIP leak fixes

still leaks with this

Comment 2

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

Attachment: WIP leak fixes

Found the major cause - GMPParent::UnloadProcess() was killing the plugin container before it could process the Encoding/DecodingComplete requests and respond.  It was assuming they were synchronous.  Modified it to check if we can unload as each encoder/decoder is removed.  This now appears to work, but exposed a refcounting problem with GMPParent now that the cleanup code is actually running.

Comment 3

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

Attachment: WIP leak fixes

more cleanup of encoders/decoders in shutdown; now only leaks the service and nsThread

Comment 4

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

Attachment: WIP leak fixes

with lots of debugging, and assumes you have the real plugin in the path.  Now only leaks a single nsThread and children (the GMPThread)

Comment 5

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

Attachment: Fix shutdown design issues with Webrtc GMP interfaces and quash leaks

This works and passes cleanly locally.  Last key was noting that the GMP WebrtcCodec wasn't getting cleaned up - it would get Release()ed (not it's not refcounted), but not deleted.  Now stored in an AutoPtr in the conduit (the config already was, but not the external codec).

Comment 6

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

Comment on attachment 8458819 [details] [diff] [review]
Fix shutdown design issues with Webrtc GMP interfaces and quash leaks

the feedbacks for cpearce and bsmedberg are optional, in case they want to comment

Comment 7

[Benjamin Smedberg]

Comment on attachment 8458819 [details] [diff] [review]
Fix shutdown design issues with Webrtc GMP interfaces and quash leaks

Did you fold the patch from bug 1039572 into yours? I just landed that, so this may need to be unfolded/rebased.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8458819 [details] [diff] [review]
Fix shutdown design issues with Webrtc GMP interfaces and quash leaks

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

Looks sane to me but someone who's more familiar with how this works should sign of too.

::: content/media/gmp/GMPParent.cpp
@@ +128,5 @@
>  
> +  // Note: the shutdown of the codecs is async!  don't kill
> +  // the plugin-container until they're all safely shut down via
> +  // MaybeUnloadProcess()
> +  MaybeUnloadProcess(); // really only for the case of none

naming nit: Usually I'd expect MaybeUnloadProcess would call UnloadProcess. Think about this way: you're calling a function called UnloadProcess, but it may not actually end up doing that.

I'd call this StartUnloadProcess but that's actually an awkward name too.

@@ +185,5 @@
> +    // Not really safe if we just grab to the mGMPThread, as we don't know
> +    // what thread we're running on and other threads may be trying to
> +    // access this without locks!  However, debug only, and primary failure
> +    // mode outside of compiler-helped TSAN is a leak.  But better would be
> +    // to use swap() under a lock.

...so why not do that?

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ +120,5 @@
>    mMPS = do_GetService("@mozilla.org/gecko-media-plugin-service;1");
>    MOZ_ASSERT(mMPS);
>  
> +  if (!mGMPThread) {
> +    if (NS_WARN_IF(NS_FAILED(mMPS->GetThread(getter_AddRefs(mGMPThread))))) {

No need to try to cram this on one line, let alone that the line that outputs a warning is somehow embedded in the if...

@@ +556,5 @@
>  
>  int32_t
>  WebrtcGmpVideoDecoder::Reset()
>  {
> +  // XXX ?

Bug nr?

Comment 9

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

Attachment: Fix shutdown design issues with Webrtc GMP interfaces and quash leaks

unbitrotted

Comment 10

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

Comment on attachment 8459121 [details] [diff] [review]
Fix shutdown design issues with Webrtc GMP interfaces and quash leaks

Merged with changes by bsmedberg
carry forward  r=gcp

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/289695fac6be

Comment 12

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

https://tbpl.mozilla.org/?tree=Try&rev=736f590d66f1

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/289695fac6be