Change return types of refcount methods.
AddRef() now returns void, and Release() returns an enum
RefCountReleaseStatus, to indicate whether or not this Release
call implied deletion.
Bug: webrtc:8270
Change-Id: If2fb77f26118b61751b51c856af187c72112c630
Reviewed-on: https://webrtc-review.googlesource.com/3320
Commit-Queue: Niels Moller <nisse@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20366}diff --git a/audio/audio_state.cc b/audio/audio_state.cc
index 92f05ad..2a84f5c 100644
--- a/audio/audio_state.cc
+++ b/audio/audio_state.cc
@@ -60,17 +60,17 @@
}
// 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 c9c2cc5..bdec529 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 @@
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 1c81f33..c8a452a 100644
--- a/media/engine/webrtcvoiceengine_unittest.cc
+++ b/media/engine/webrtcvoiceengine_unittest.cc
@@ -84,8 +84,9 @@
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 @@
// Tests that reference counting on the external ADM is correct.
TEST(WebRtcVoiceEngineTest, StartupShutdownWithExternalADM) {
testing::NiceMock<webrtc::test::MockAudioDeviceModule> 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<webrtc::AudioProcessing> 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 94cb919..c49e9fe 100644
--- a/modules/audio_device/include/fake_audio_device.h
+++ b/modules/audio_device/include/fake_audio_device.h
@@ -19,8 +19,13 @@
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 2c3602d..a98d2ac 100644
--- a/modules/audio_device/include/mock_audio_device.h
+++ b/modules/audio_device/include/mock_audio_device.h
@@ -22,8 +22,8 @@
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 107d905..e152bef 100644
--- a/modules/audio_processing/audio_processing_impl_unittest.cc
+++ b/modules/audio_processing/audio_processing_impl_unittest.cc
@@ -30,8 +30,8 @@
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 80a444c..e2fdc62 100644
--- a/pc/transportcontroller.cc
+++ b/pc/transportcontroller.cc
@@ -310,7 +310,10 @@
<< ", 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 @@
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 fb69507..26bfd5d 100644
--- a/rtc_base/callback_unittest.cc
+++ b/rtc_base/callback_unittest.cc
@@ -31,11 +31,11 @@
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 f29d279..fb0971c 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 182e027..f04b7f9 100644
--- a/rtc_base/refcountedobject.h
+++ b/rtc_base/refcountedobject.h
@@ -13,6 +13,7 @@
#include <utility>
#include "rtc_base/atomicops.h"
+#include "rtc_base/refcount.h"
namespace rtc {
@@ -30,14 +31,14 @@
std::forward<P1>(p1),
std::forward<Args>(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 143ca85..4744525 100644
--- a/rtc_base/refcountedobject_unittest.cc
+++ b/rtc_base/refcountedobject_unittest.cc
@@ -61,12 +61,14 @@
} // namespace
-TEST(RefCountedObject, Basic) {
+TEST(RefCountedObject, HasOneRef) {
scoped_refptr<RefCountedObject<RefClass>> aref(
new RefCountedObject<RefClass>());
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 8c02e3b..920bf46 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 8bc8b1f..a542c28 100644
--- a/sdk/android/src/jni/pc/peerconnection_jni.cc
+++ b/sdk/android/src/jni/pc/peerconnection_jni.cc
@@ -110,8 +110,7 @@
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 4160f5a..55d7b31 100644
--- a/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc
+++ b/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc
@@ -304,8 +304,7 @@
// 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";
}