-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RTCEncodedFrame should have a virtual destructor #28757
RTCEncodedFrame should have a virtual destructor #28757
Conversation
EWS run on current version of this PR (hash 8bdd6b6) |
@@ -40,6 +40,8 @@ namespace WebCore { | |||
|
|||
class RTCEncodedFrame : public RefCounted<RTCEncodedFrame> { | |||
public: | |||
virtual ~RTCEncodedFrame() { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary? The constructor is marked as protected so no-one can destroy a RTCEncodedFrame directly. Having subclasses is fine as long as their destructor gets called first, which it would since the base class constructor is not public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe it is necessary because of the way RefCounted calls delete
? I have to look more into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't construct the base class, but you can
- construct the subclass
- cast the subclass pointer to the base class pointer
- delete the base class pointer
In this case, I think that can mean that you fail to destroy RTCEncodedVideoFrame::m_metadata::dependencies (which is a Vector).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, looking and RefCounted::deref, I think this is indeed needed.
https://bugs.webkit.org/show_bug.cgi?id=274353 Reviewed by Chris Dumez. Added a virtual destructor since this class has subclasses. * Source/WebCore/Modules/mediastream/RTCEncodedFrame.h: (WebCore::RTCEncodedFrame::~RTCEncodedFrame): Canonical link: https://commits.webkit.org/278964@main
8bdd6b6
to
5ccae9e
Compare
Committed 278964@main (5ccae9e): https://commits.webkit.org/278964@main Reviewed commits have been landed. Closing PR #28757 and removing active labels. |
5ccae9e
8bdd6b6