Closed Bug 980304 Opened 8 years ago Closed 8 years ago

Add an RAII/holder for webrtc.org Interface pointers

Categories

(Core :: WebRTC, defect)

defect
Not set
minor

Tracking

()

RESOLVED DUPLICATE of bug 980096

People

(Reporter: jesup, Unassigned)

Details

Attachments

(2 files, 1 obsolete file)

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.
Summary: Add an RAII holder for webrtc.org Interface pointers → Add an RAII/holder for webrtc.org Interface pointers
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.
Attachment #8387431 - Flags: review?(mh+mozilla)
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.
Attachment #8387431 - Flags: review?(mh+mozilla) → review-
Attachment #8387431 - Attachment is obsolete: true
(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.
Attachment #8387636 - Attachment description: Simple template for holding WebRTC Interface pointers to ensure Release() → Use ScopedCustomReleasePtr to hold WebRTC Interface ptrs to ensure Release()
Attachment #8387636 - Flags: review?(mh+mozilla)
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.
Attachment #8387704 - Flags: review?(khuey) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 980096
Attachment #8387636 - Flags: review?(mh+mozilla)
You need to log in before you can comment on or make changes to this bug.