Reject double RefCountedObject inheritance in rtc::make_ref_counted.

Bug: webrtc:12701
Change-Id: Ie45707e3266e6a27cae073f824a1c77707d77000
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/256240
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36281}

api/test/mock_peerconnectioninterface.h

@@ -25,8 +25,7 @@
 
 namespace webrtc {
 
-class MockPeerConnectionInterface
-    : public rtc::RefCountedObject<webrtc::PeerConnectionInterface> {
+class MockPeerConnectionInterface : public webrtc::PeerConnectionInterface {
  public:
   static rtc::scoped_refptr<MockPeerConnectionInterface> Create() {
     return rtc::make_ref_counted<MockPeerConnectionInterface>();
@@ -199,7 +198,9 @@ class MockPeerConnectionInterface
   MOCK_METHOD(void, Close, (), (override));
 };
 
-static_assert(!std::is_abstract<MockPeerConnectionInterface>::value, "");
+static_assert(
+    !std::is_abstract_v<rtc::RefCountedObject<MockPeerConnectionInterface>>,
+    "");
 
 }  // namespace webrtc
 

pc/peer_connection_rtp_unittest.cc

@@ -69,8 +69,7 @@ using ::testing::Values;
 const uint32_t kDefaultTimeout = 10000u;
 
 template <typename MethodFunctor>
-class OnSuccessObserver : public rtc::RefCountedObject<
-                              webrtc::SetRemoteDescriptionObserverInterface> {
+class OnSuccessObserver : public webrtc::SetRemoteDescriptionObserverInterface {
  public:
   explicit OnSuccessObserver(MethodFunctor on_success)
       : on_success_(std::move(on_success)) {}

pc/peer_connection_signaling_unittest.cc

@@ -610,8 +610,7 @@ TEST_P(PeerConnectionSignalingTest,
 
 TEST_P(PeerConnectionSignalingTest, ImplicitCreateOfferAndShutdown) {
   auto caller = CreatePeerConnection();
-  rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
-      new FakeSetLocalDescriptionObserver());
+  auto observer = rtc::make_ref_counted<FakeSetLocalDescriptionObserver>();
   caller->pc()->SetLocalDescription(observer);
   caller.reset(nullptr);
   // The new observer gets invoked because it is called immediately.
@@ -632,8 +631,7 @@ TEST_P(PeerConnectionSignalingTest,
 
 TEST_P(PeerConnectionSignalingTest, CloseBeforeImplicitCreateOfferAndShutdown) {
   auto caller = CreatePeerConnection();
-  rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
-      new FakeSetLocalDescriptionObserver());
+  auto observer = rtc::make_ref_counted<FakeSetLocalDescriptionObserver>();
   caller->pc()->Close();
   caller->pc()->SetLocalDescription(observer);
   caller.reset(nullptr);
@@ -655,8 +653,7 @@ TEST_P(PeerConnectionSignalingTest,
 
 TEST_P(PeerConnectionSignalingTest, CloseAfterImplicitCreateOfferAndShutdown) {
   auto caller = CreatePeerConnection();
-  rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
-      new FakeSetLocalDescriptionObserver());
+  auto observer = rtc::make_ref_counted<FakeSetLocalDescriptionObserver>();
   caller->pc()->SetLocalDescription(observer);
   caller->pc()->Close();
   caller.reset(nullptr);
@@ -670,8 +667,7 @@ TEST_P(PeerConnectionSignalingTest,
   auto caller = CreatePeerConnection();
   auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
 
-  rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
-      new FakeSetLocalDescriptionObserver());
+  auto observer = rtc::make_ref_counted<FakeSetLocalDescriptionObserver>();
   caller->pc()->SetLocalDescription(std::move(offer), observer);
   // The new observer is invoked immediately.
   EXPECT_TRUE(observer->called());

pc/test/mock_data_channel.h

@@ -18,7 +18,7 @@
 
 namespace webrtc {
 
-class MockSctpDataChannel : public rtc::RefCountedObject<SctpDataChannel> {
+class MockSctpDataChannel : public SctpDataChannel {
  public:
   MockSctpDataChannel(int id, DataState state)
       : MockSctpDataChannel(id,
@@ -41,11 +41,11 @@ class MockSctpDataChannel : public rtc::RefCountedObject<SctpDataChannel> {
       const InternalDataChannelInit& config = InternalDataChannelInit(),
       rtc::Thread* signaling_thread = rtc::Thread::Current(),
       rtc::Thread* network_thread = rtc::Thread::Current())
-      : rtc::RefCountedObject<SctpDataChannel>(config,
-                                               nullptr,
-                                               label,
-                                               signaling_thread,
-                                               network_thread) {
+      : SctpDataChannel(config,
+                        nullptr,
+                        label,
+                        signaling_thread,
+                        network_thread) {
     EXPECT_CALL(*this, id()).WillRepeatedly(::testing::Return(id));
     EXPECT_CALL(*this, state()).WillRepeatedly(::testing::Return(state));
     EXPECT_CALL(*this, protocol()).WillRepeatedly(::testing::Return(protocol));

pc/test/mock_peer_connection_observers.h

@@ -336,7 +336,7 @@ class MockSetSessionDescriptionObserver
 };
 
 class FakeSetLocalDescriptionObserver
-    : public rtc::RefCountedObject<SetLocalDescriptionObserverInterface> {
+    : public SetLocalDescriptionObserverInterface {
  public:
   bool called() const { return error_.has_value(); }
   RTCError& error() {
@@ -355,7 +355,7 @@ class FakeSetLocalDescriptionObserver
 };
 
 class FakeSetRemoteDescriptionObserver
-    : public rtc::RefCountedObject<SetRemoteDescriptionObserverInterface> {
+    : public SetRemoteDescriptionObserverInterface {
  public:
   bool called() const { return error_.has_value(); }
   RTCError& error() {

rtc_base/ref_counted_object.h

@@ -142,11 +142,17 @@ class FinalRefCountedObject final : public T {
 // Note that in some cases, using RefCountedObject directly may still be what's
 // needed.
 
-// `make_ref_counted` for classes that are convertible to RefCountInterface.
-template <typename T,
-          typename... Args,
-          typename std::enable_if<std::is_convertible_v<T*, RefCountInterface*>,
-                                  T>::type* = nullptr>
+// `make_ref_counted` for abstract classes that are convertible to
+// RefCountInterface. The is_abstract requirement rejects classes that inherit
+// both RefCountInterface and RefCounted object, which is a a discouraged
+// pattern, and would result in double inheritance of RefCountedObject if this
+// template was applied.
+template <
+    typename T,
+    typename... Args,
+    typename std::enable_if<std::is_convertible_v<T*, RefCountInterface*> &&
+                                std::is_abstract_v<T>,
+                            T>::type* = nullptr>
 scoped_refptr<T> make_ref_counted(Args&&... args) {
   return scoped_refptr<T>(new RefCountedObject<T>(std::forward<Args>(args)...));
 }