diff --git a/audio/audio_state.cc b/audio/audio_state.cc index 92f05ad943..2a84f5c92a 100644 --- a/audio/audio_state.cc +++ b/audio/audio_state.cc @@ -60,17 +60,17 @@ bool AudioState::typing_noise_detected() const { } // Reference count; implementation copied from rtc::RefCountedObject. -int AudioState::AddRef() const { - return rtc::AtomicOps::Increment(&ref_count_); +void AudioState::AddRef() const { + rtc::AtomicOps::Increment(&ref_count_); } // Reference count; implementation copied from rtc::RefCountedObject. -int AudioState::Release() const { - int count = rtc::AtomicOps::Decrement(&ref_count_); - if (!count) { +rtc::RefCountReleaseStatus AudioState::Release() const { + if (rtc::AtomicOps::Decrement(&ref_count_) == 0) { delete this; + return rtc::RefCountReleaseStatus::kDroppedLastRef; } - return count; + return rtc::RefCountReleaseStatus::kOtherRefsRemained; } } // namespace internal diff --git a/audio/audio_state.h b/audio/audio_state.h index c9c2cc561e..bdec529cd8 100644 --- a/audio/audio_state.h +++ b/audio/audio_state.h @@ -16,6 +16,7 @@ #include "call/audio_state.h" #include "rtc_base/constructormagic.h" #include "rtc_base/criticalsection.h" +#include "rtc_base/refcount.h" #include "rtc_base/thread_checker.h" #include "voice_engine/include/voe_base.h" @@ -38,8 +39,8 @@ class AudioState final : public webrtc::AudioState { private: // rtc::RefCountInterface implementation. - int AddRef() const override; - int Release() const override; + void AddRef() const override; + rtc::RefCountReleaseStatus Release() const override; rtc::ThreadChecker thread_checker_; rtc::ThreadChecker process_thread_checker_; diff --git a/media/engine/webrtcvoiceengine_unittest.cc b/media/engine/webrtcvoiceengine_unittest.cc index 1c81f33cbd..c8a452a99b 100644 --- a/media/engine/webrtcvoiceengine_unittest.cc +++ b/media/engine/webrtcvoiceengine_unittest.cc @@ -84,8 +84,9 @@ class MockTransmitMixer : public webrtc::voe::TransmitMixer { void AdmSetupExpectations(webrtc::test::MockAudioDeviceModule* adm) { RTC_DCHECK(adm); - EXPECT_CALL(*adm, AddRef()).WillOnce(Return(0)); - EXPECT_CALL(*adm, Release()).WillOnce(Return(0)); + EXPECT_CALL(*adm, AddRef()).Times(1); + EXPECT_CALL(*adm, Release()) + .WillOnce(Return(rtc::RefCountReleaseStatus::kDroppedLastRef)); #if !defined(WEBRTC_IOS) EXPECT_CALL(*adm, Recording()).WillOnce(Return(false)); EXPECT_CALL(*adm, SetRecordingChannel(webrtc::AudioDeviceModule:: @@ -3340,8 +3341,10 @@ TEST(WebRtcVoiceEngineTest, StartupShutdown) { // Tests that reference counting on the external ADM is correct. TEST(WebRtcVoiceEngineTest, StartupShutdownWithExternalADM) { testing::NiceMock adm; - EXPECT_CALL(adm, AddRef()).Times(3).WillRepeatedly(Return(0)); - EXPECT_CALL(adm, Release()).Times(3).WillRepeatedly(Return(0)); + EXPECT_CALL(adm, AddRef()).Times(3); + EXPECT_CALL(adm, Release()) + .Times(3) + .WillRepeatedly(Return(rtc::RefCountReleaseStatus::kDroppedLastRef)); { rtc::scoped_refptr apm = webrtc::AudioProcessing::Create(); diff --git a/modules/audio_device/include/fake_audio_device.h b/modules/audio_device/include/fake_audio_device.h index 94cb9194a3..c49e9fe455 100644 --- a/modules/audio_device/include/fake_audio_device.h +++ b/modules/audio_device/include/fake_audio_device.h @@ -19,8 +19,13 @@ class FakeAudioDeviceModule : public AudioDeviceModule { public: FakeAudioDeviceModule() {} virtual ~FakeAudioDeviceModule() {} - int32_t AddRef() const override { return 0; } - int32_t Release() const override { return 0; } + + // TODO(nisse): Fix all users of this class to managed references using + // scoped_refptr. Current code doesn't always use refcounting for this class. + void AddRef() const override {} + rtc::RefCountReleaseStatus Release() const override { + return rtc::RefCountReleaseStatus::kDroppedLastRef; + } private: int32_t RegisterAudioCallback(AudioTransport* audioCallback) override { diff --git a/modules/audio_device/include/mock_audio_device.h b/modules/audio_device/include/mock_audio_device.h index 2c3602ddcf..a98d2aca82 100644 --- a/modules/audio_device/include/mock_audio_device.h +++ b/modules/audio_device/include/mock_audio_device.h @@ -22,8 +22,8 @@ namespace test { class MockAudioDeviceModule : public AudioDeviceModule { public: // RefCounted - MOCK_CONST_METHOD0(AddRef, int32_t()); - MOCK_CONST_METHOD0(Release, int32_t()); + MOCK_CONST_METHOD0(AddRef, void()); + MOCK_CONST_METHOD0(Release, rtc::RefCountReleaseStatus()); // AudioDeviceModule. MOCK_CONST_METHOD1(ActiveAudioLayer, int32_t(AudioLayer* audioLayer)); MOCK_CONST_METHOD0(LastError, ErrorCode()); diff --git a/modules/audio_processing/audio_processing_impl_unittest.cc b/modules/audio_processing/audio_processing_impl_unittest.cc index 107d905642..e152befc5c 100644 --- a/modules/audio_processing/audio_processing_impl_unittest.cc +++ b/modules/audio_processing/audio_processing_impl_unittest.cc @@ -30,8 +30,8 @@ class MockInitialize : public AudioProcessingImpl { return AudioProcessingImpl::InitializeLocked(); } - MOCK_CONST_METHOD0(AddRef, int()); - MOCK_CONST_METHOD0(Release, int()); + MOCK_CONST_METHOD0(AddRef, void()); + MOCK_CONST_METHOD0(Release, rtc::RefCountReleaseStatus()); }; } // namespace diff --git a/pc/transportcontroller.cc b/pc/transportcontroller.cc index 80a444ca5f..e2fdc62e43 100644 --- a/pc/transportcontroller.cc +++ b/pc/transportcontroller.cc @@ -310,7 +310,10 @@ void TransportController::DestroyDtlsTransport_n( << ", which doesn't exist."; return; } - if ((*it)->Release() > 0) { + // Release one reference to the RefCountedChannel, and do additional cleanup + // only if it was the last one. Matches the AddRef logic in + // CreateDtlsTransport_n. + if ((*it)->Release() == rtc::RefCountReleaseStatus::kOtherRefsRemained) { return; } channels_.erase(it); @@ -471,11 +474,14 @@ JsepTransport* TransportController::GetOrCreateJsepTransport( void TransportController::DestroyAllChannels_n() { RTC_DCHECK(network_thread_->IsCurrent()); transports_.clear(); + // TODO(nisse): If |channels_| were a vector of scoped_refptr, we + // wouldn't need this strange hack. for (RefCountedChannel* channel : channels_) { // Even though these objects are normally ref-counted, if // TransportController is deleted while they still have references, just // remove all references. - while (channel->Release() > 0) { + while (channel->Release() == + rtc::RefCountReleaseStatus::kOtherRefsRemained) { } } channels_.clear(); diff --git a/rtc_base/callback_unittest.cc b/rtc_base/callback_unittest.cc index fb6950741b..26bfd5daf4 100644 --- a/rtc_base/callback_unittest.cc +++ b/rtc_base/callback_unittest.cc @@ -31,11 +31,11 @@ struct BindTester { class RefCountedBindTester : public RefCountInterface { public: RefCountedBindTester() : count_(0) {} - int AddRef() const override { - return ++count_; - } - int Release() const override { - return --count_; + void AddRef() const override { ++count_; } + RefCountReleaseStatus Release() const override { + --count_; + return count_ == 0 ? RefCountReleaseStatus::kDroppedLastRef + : RefCountReleaseStatus::kOtherRefsRemained; } int RefCount() const { return count_; } diff --git a/rtc_base/refcount.h b/rtc_base/refcount.h index f29d27990d..fb0971ce01 100644 --- a/rtc_base/refcount.h +++ b/rtc_base/refcount.h @@ -12,12 +12,52 @@ namespace rtc { -// Reference count interface. +// Refcounted objects should implement the following informal interface: +// +// void AddRef() const ; +// RefCountReleaseStatus Release() const; +// +// You may access members of a reference-counted object, including the AddRef() +// and Release() methods, only if you already own a reference to it, or if +// you're borrowing someone else's reference. (A newly created object is a +// special case: the reference count is zero on construction, and the code that +// creates the object should immediately call AddRef(), bringing the reference +// count from zero to one, e.g., by constructing an rtc::scoped_refptr). +// +// AddRef() creates a new reference to the object. +// +// Release() releases a reference to the object; the caller now has one less +// reference than before the call. Returns kDroppedLastRef if the number of +// references dropped to zero because of this (in which case the object destroys +// itself). Otherwise, returns kOtherRefsRemained, to signal that at the precise +// time the caller's reference was dropped, other references still remained (but +// if other threads own references, this may of course have changed by the time +// Release() returns). +// +// The caller of Release() must treat it in the same way as a delete operation: +// Regardless of the return value from Release(), the caller mustn't access the +// object. The object might still be alive, due to references held by other +// users of the object, but the object can go away at any time, e.g., as the +// result of another thread calling Release(). +// +// Calling AddRef() and Release() manually is discouraged. It's recommended to +// use rtc::scoped_refptr to manage all pointers to reference counted objects. +// Note that rtc::scoped_refptr depends on compile-time duck-typing; formally +// implementing the below RefCountInterface is not required. + +enum class RefCountReleaseStatus { kDroppedLastRef, kOtherRefsRemained }; + +// Interfaces where refcounting is part of the public api should +// inherit this abstract interface. The implementation of these +// methods is usually provided by the RefCountedObject template class, +// applied as a leaf in the inheritance tree. class RefCountInterface { public: - virtual int AddRef() const = 0; - virtual int Release() const = 0; + virtual void AddRef() const = 0; + virtual RefCountReleaseStatus Release() const = 0; + // Non-public destructor, because Release() has exclusive responsibility for + // destroying the object. protected: virtual ~RefCountInterface() {} }; diff --git a/rtc_base/refcountedobject.h b/rtc_base/refcountedobject.h index 182e027b92..f04b7f974c 100644 --- a/rtc_base/refcountedobject.h +++ b/rtc_base/refcountedobject.h @@ -13,6 +13,7 @@ #include #include "rtc_base/atomicops.h" +#include "rtc_base/refcount.h" namespace rtc { @@ -30,14 +31,14 @@ class RefCountedObject : public T { std::forward(p1), std::forward(args)...) {} - virtual int AddRef() const { return AtomicOps::Increment(&ref_count_); } + virtual void AddRef() const { AtomicOps::Increment(&ref_count_); } - virtual int Release() const { - int count = AtomicOps::Decrement(&ref_count_); - if (!count) { + virtual RefCountReleaseStatus Release() const { + if (AtomicOps::Decrement(&ref_count_) == 0) { delete this; + return RefCountReleaseStatus::kDroppedLastRef; } - return count; + return RefCountReleaseStatus::kOtherRefsRemained; } // Return whether the reference count is one. If the reference count is used diff --git a/rtc_base/refcountedobject_unittest.cc b/rtc_base/refcountedobject_unittest.cc index 143ca85c44..4744525af6 100644 --- a/rtc_base/refcountedobject_unittest.cc +++ b/rtc_base/refcountedobject_unittest.cc @@ -61,12 +61,14 @@ class RefClassWithMixedValues : public RefCountInterface { } // namespace -TEST(RefCountedObject, Basic) { +TEST(RefCountedObject, HasOneRef) { scoped_refptr> aref( new RefCountedObject()); EXPECT_TRUE(aref->HasOneRef()); - EXPECT_EQ(2, aref->AddRef()); - EXPECT_EQ(1, aref->Release()); + aref->AddRef(); + EXPECT_FALSE(aref->HasOneRef()); + EXPECT_EQ(aref->Release(), RefCountReleaseStatus::kOtherRefsRemained); + EXPECT_TRUE(aref->HasOneRef()); } TEST(RefCountedObject, SupportRValuesInCtor) { diff --git a/sdk/android/src/jni/jni_helpers.h b/sdk/android/src/jni/jni_helpers.h index 8c02e3b6df..920bf4691b 100644 --- a/sdk/android/src/jni/jni_helpers.h +++ b/sdk/android/src/jni/jni_helpers.h @@ -21,6 +21,7 @@ #include "rtc_base/checks.h" #include "rtc_base/constructormagic.h" +#include "rtc_base/refcount.h" #include "rtc_base/thread_checker.h" // Abort the process if |jni| has a Java exception pending. @@ -33,8 +34,9 @@ // Helper that calls ptr->Release() and aborts the process with a useful // message if that didn't actually delete *ptr because of extra refcounts. -#define CHECK_RELEASE(ptr) \ - RTC_CHECK_EQ(0, (ptr)->Release()) << "Unexpected refcount." +#define CHECK_RELEASE(ptr) \ + RTC_CHECK((ptr)->Release() == rtc::RefCountReleaseStatus::kDroppedLastRef) \ + << "Unexpected refcount." // Convenience macro defining JNI-accessible methods in the org.webrtc package. // Eliminates unnecessary boilerplate and line-wraps, reducing visual clutter. diff --git a/sdk/android/src/jni/pc/peerconnection_jni.cc b/sdk/android/src/jni/pc/peerconnection_jni.cc index 8bc8b1f083..a542c28e48 100644 --- a/sdk/android/src/jni/pc/peerconnection_jni.cc +++ b/sdk/android/src/jni/pc/peerconnection_jni.cc @@ -110,8 +110,7 @@ JNI_FUNCTION_DECLARATION(jobject, nativeChannelPtr); CHECK_EXCEPTION(jni) << "error during NewObject"; // Channel is now owned by Java object, and will be freed from there. - int bumped_count = channel->AddRef(); - RTC_CHECK(bumped_count == 2) << "Unexpected refcount"; + channel->AddRef(); return j_channel; } diff --git a/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc b/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc index 4160f5acec..55d7b3108a 100644 --- a/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc +++ b/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc @@ -304,8 +304,7 @@ void PeerConnectionObserverJni::OnDataChannel( // DataChannel.dispose(). Important that this be done _after_ the // CallVoidMethod above as Java code might call back into native code and be // surprised to see a refcount of 2. - int bumped_count = channel->AddRef(); - RTC_CHECK(bumped_count == 2) << "Unexpected refcount OnDataChannel"; + channel->AddRef(); CHECK_EXCEPTION(jni()) << "error during CallVoidMethod"; }