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}

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
 

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_;

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<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();

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 {

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());

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

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();

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_; }
 

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() {}
 };

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 @@ class RefCountedObject : public T {
           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

rtc_base/refcountedobject_unittest.cc

@@ -61,12 +61,14 @@ class RefClassWithMixedValues : public RefCountInterface {
 
 }  // 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) {

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.
@@ -34,7 +35,8 @@
 // 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."
+  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.

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;
 }
 

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";
 }