We need an RAII holder class for Interfaces returned by Foo::GetInterface(Voice/VideoEngine) calls (which have to be Release()d to avoid internal reference count mismatches). This will avoid inadvertent leaks like the one we had for VoiceEngine and simplify cleanup code.

applies on top of bug 980096; no webrtc.org leaks in an lsan run.

Comment on attachment 8387431 [details] [diff] [review] Simple template for holding WebRTC Interface pointers to ensure Release() For reference: webrtc.org module classes all are instantiated via webrtc::classname::GetInterface(voice_engine or video_engine) (static methods), and released with ptr->Release() (they internally refcount). We've had leaks (see bug 980096) where GetInterfaces were added, but Releases weren't (or were missed on some error path). We can't support certain copies due to no equivalent to AddRef being exposed; you have to GetInterface() with the correct engine, and in practice we don't need these methods.

Comment on attachment 8387431 [details] [diff] [review] Simple template for holding WebRTC Interface pointers to ensure Release() Review of attachment 8387431 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/common/Wrapper.h @@ +209,5 @@ > + ptr = t; > + } > + > + T* ptr; > +}; You're essentially duplicating what is provided by mozilla/Scoped.h. And you already have something similar in media/webrtc/signaling/src/media-conduit/MediaEngineWrapper.h using mozilla/Scoped.h. Seems to me you want to move that one instead.

(In reply to Mike Hommey [:glandium] from comment #3) > You're essentially duplicating what is provided by mozilla/Scoped.h. And you > already have something similar in > media/webrtc/signaling/src/media-conduit/MediaEngineWrapper.h using > mozilla/Scoped.h. Seems to me you want to move that one instead. Good find. I didn't know it was there... because nothing uses it (anymore). I'm looking into why... it's old, from the early days working on alder. Probably someone thought we'd need it, but when someone else wrote code that would use it they didn't know about it. But in any case, I had the right idea. Hmmm... It was used originally, and seems to have been removed in bug 792188 patch 5 (earlier versions of patch 5 didn't remove it; the version from 2012-10-01 16:xx switched it back to raw pointers with no comments. There were some earlier discussions about some alternative thing that was later removed; no discussion ever of switching to a raw pointer. Looks like something Suhas did on his own with no comment; perhaps there was a face-to-face discussion with ekr or ethan. In any case, now we know.

Comment on attachment 8387704 [details] [diff] [review] Merged patch of this and bug 980096 Review of attachment 8387704 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webrtc/MediaEngineWebRTC.cpp @@ +32,5 @@ > #include "nsIComponentRegistrar.h" > #include "MediaEngineTabVideoSource.h" > #include "nsITabSource.h" > > +#include "MediaEngineWrapper.h" you shouldn't need his in both the header and the cpp, unless I'm missing something, since you include the .h above.