980304 - Add an RAII/holder for webrtc.org Interface pointers

Status: RESOLVED DUPLICATE of bug 980096

(Core :: WebRTC, defect)

Description

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

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.

Comment 1

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

Attachment: Simple template for holding WebRTC Interface pointers to ensure Release()

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

Comment 2

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

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 3

[Mike Hommey [:glandium]]

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.

Comment 4

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

Attachment: Use ScopedCustomReleasePtr to hold WebRTC Interface ptrs to ensure Release()

Comment 5

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

(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 6

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

Attachment: Merged patch of this and bug 980096

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.