Use scoped_refptr for On(Add|Remove)Stream and OnDataChannel.

This will make it much less likely for application developers to not
realize the object is reference counted.

It also fixes a bug in the Java PeerConnection binding, by allowing a
reference to be transferred in the OnRemoveStream call via std::move.

BUG=webrtc:5128
R=pthatcher@webrtc.org, tkchin@webrtc.org

Review URL: https://codereview.webrtc.org/1972793003 .

Cr-Commit-Position: refs/heads/master@{#12976}

webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java

@@ -746,9 +746,7 @@ public class PeerConnectionTest extends ActivityTestCase {
     offeringExpectations.dataChannel.close();
     getMetrics();
 
-    // Free the Java-land objects, collect them, and sleep a bit to make sure we
-    // don't get late-arrival crashes after the Java-land objects have been
-    // freed.
+    // Free the Java-land objects and collect them.
     shutdownPC(offeringPC, offeringExpectations);
     offeringPC = null;
     shutdownPC(answeringPC, answeringExpectations);
@@ -959,14 +957,67 @@ public class PeerConnectionTest extends ActivityTestCase {
     MediaStream aRMS = answeringExpectations.gotRemoteStreams.iterator().next();
     assertEquals(aRMS.videoTracks.get(0).state(), MediaStreamTrack.State.ENDED);
 
-    // Free the Java-land objects, collect them, and sleep a bit to make sure we
-    // don't get late-arrival crashes after the Java-land objects have been
-    // freed.
+    // Finally, remove the audio track as well, which should completely remove
+    // the remote stream. This used to trigger an assert.
+    // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5128
+    AudioTrack offererAudioTrack = oLMS.get().audioTracks.get(0);
+    oLMS.get().removeTrack(offererAudioTrack);
+
+    // Create offer.
+    sdpLatch = new SdpObserverLatch();
+    offeringPC.createOffer(sdpLatch, new MediaConstraints());
+    assertTrue(sdpLatch.await());
+    offerSdp = sdpLatch.getSdp();
+    assertEquals(offerSdp.type, SessionDescription.Type.OFFER);
+    assertFalse(offerSdp.description.isEmpty());
+
+    // Set local description for offerer.
+    sdpLatch = new SdpObserverLatch();
+    offeringExpectations.expectSignalingChange(SignalingState.HAVE_LOCAL_OFFER);
+    offeringPC.setLocalDescription(sdpLatch, offerSdp);
+    assertTrue(sdpLatch.await());
+    assertNull(sdpLatch.getSdp());
+
+    // Set remote description for answerer.
+    sdpLatch = new SdpObserverLatch();
+    answeringExpectations.expectSignalingChange(SignalingState.HAVE_REMOTE_OFFER);
+    answeringExpectations.expectRemoveStream("offeredMediaStream");
+    answeringPC.setRemoteDescription(sdpLatch, offerSdp);
+    assertTrue(sdpLatch.await());
+    assertNull(sdpLatch.getSdp());
+
+    // Create answer.
+    sdpLatch = new SdpObserverLatch();
+    answeringPC.createAnswer(sdpLatch, new MediaConstraints());
+    assertTrue(sdpLatch.await());
+    answerSdp = sdpLatch.getSdp();
+    assertEquals(answerSdp.type, SessionDescription.Type.ANSWER);
+    assertFalse(answerSdp.description.isEmpty());
+
+    // Set local description for answerer.
+    sdpLatch = new SdpObserverLatch();
+    answeringExpectations.expectSignalingChange(SignalingState.STABLE);
+    answeringPC.setLocalDescription(sdpLatch, answerSdp);
+    assertTrue(sdpLatch.await());
+    assertNull(sdpLatch.getSdp());
+
+    // Set remote description for offerer.
+    sdpLatch = new SdpObserverLatch();
+    offeringExpectations.expectSignalingChange(SignalingState.STABLE);
+    offeringPC.setRemoteDescription(sdpLatch, answerSdp);
+    assertTrue(sdpLatch.await());
+    assertNull(sdpLatch.getSdp());
+
+    // Make sure the stream was really removed.
+    assertTrue(answeringExpectations.gotRemoteStreams.isEmpty());
+
+    // Free the Java-land objects and collect them.
     shutdownPC(offeringPC, offeringExpectations);
     offeringPC = null;
     shutdownPC(answeringPC, answeringExpectations);
     answeringPC = null;
     offererVideoTrack.dispose();
+    offererAudioTrack.dispose();
     videoSource.dispose();
     factory.dispose();
     System.gc();

webrtc/api/java/jni/peerconnection_jni.cc

@@ -256,14 +256,14 @@ class PCOJava : public PeerConnectionObserver {
     CHECK_EXCEPTION(jni()) << "error during CallVoidMethod";
   }
 
-  void OnAddStream(MediaStreamInterface* stream) override {
+  void OnAddStream(rtc::scoped_refptr<MediaStreamInterface> stream) override {
     ScopedLocalRefFrame local_ref_frame(jni());
     // Java MediaStream holds one reference. Corresponding Release() is in
     // MediaStream_free, triggered by MediaStream.dispose().
     stream->AddRef();
     jobject j_stream =
         jni()->NewObject(*j_media_stream_class_, j_media_stream_ctor_,
-                         reinterpret_cast<jlong>(stream));
+                         reinterpret_cast<jlong>(stream.get()));
     CHECK_EXCEPTION(jni()) << "error during NewObject";
 
     for (const auto& track : stream->GetAudioTracks()) {
@@ -319,7 +319,8 @@ class PCOJava : public PeerConnectionObserver {
     CHECK_EXCEPTION(jni()) << "error during CallVoidMethod";
   }
 
-  void OnRemoveStream(MediaStreamInterface* stream) override {
+  void OnRemoveStream(
+      rtc::scoped_refptr<MediaStreamInterface> stream) override {
     ScopedLocalRefFrame local_ref_frame(jni());
     NativeToJavaStreamsMap::iterator it = remote_streams_.find(stream);
     RTC_CHECK(it != remote_streams_.end()) << "unexpected stream: " << std::hex
@@ -329,13 +330,17 @@ class PCOJava : public PeerConnectionObserver {
                               "(Lorg/webrtc/MediaStream;)V");
     jni()->CallVoidMethod(*j_observer_global_, m, j_stream);
     CHECK_EXCEPTION(jni()) << "error during CallVoidMethod";
+    // Release the refptr reference so that DisposeRemoteStream can assert
+    // it removes the final reference.
+    stream = nullptr;
     DisposeRemoteStream(it);
   }
 
-  void OnDataChannel(DataChannelInterface* channel) override {
+  void OnDataChannel(
+      rtc::scoped_refptr<DataChannelInterface> channel) override {
     ScopedLocalRefFrame local_ref_frame(jni());
     jobject j_channel = jni()->NewObject(
-        *j_data_channel_class_, j_data_channel_ctor_, (jlong)channel);
+        *j_data_channel_class_, j_data_channel_ctor_, (jlong)channel.get());
     CHECK_EXCEPTION(jni()) << "error during NewObject";
 
     jmethodID m = GetMethodID(jni(), *j_observer_class_, "onDataChannel",

webrtc/api/peerconnection.cc

@@ -1152,7 +1152,11 @@ void PeerConnection::SetRemoteDescription(
   for (size_t i = 0; i < new_streams->count(); ++i) {
     MediaStreamInterface* new_stream = new_streams->at(i);
     stats_->AddStream(new_stream);
+    // Call both the raw pointer and scoped_refptr versions of the method
+    // for compatibility.
     observer_->OnAddStream(new_stream);
+    observer_->OnAddStream(
+        rtc::scoped_refptr<MediaStreamInterface>(new_stream));
   }
 
   UpdateEndedRemoteMediaStreams();
@@ -1715,9 +1719,12 @@ void PeerConnection::UpdateEndedRemoteMediaStreams() {
     }
   }
 
-  for (const auto& stream : streams_to_remove) {
+  for (auto& stream : streams_to_remove) {
     remote_streams_->RemoveStream(stream);
-    observer_->OnRemoveStream(stream);
+    // Call both the raw pointer and scoped_refptr versions of the method
+    // for compatibility.
+    observer_->OnRemoveStream(stream.get());
+    observer_->OnRemoveStream(std::move(stream));
   }
 }
 
@@ -1888,8 +1895,11 @@ void PeerConnection::CreateRemoteRtpDataChannel(const std::string& label,
     return;
   }
   channel->SetReceiveSsrc(remote_ssrc);
-  observer_->OnDataChannel(
-      DataChannelProxy::Create(signaling_thread(), channel));
+  auto proxy_channel = DataChannelProxy::Create(signaling_thread(), channel);
+  // Call both the raw pointer and scoped_refptr versions of the method
+  // for compatibility.
+  observer_->OnDataChannel(proxy_channel.get());
+  observer_->OnDataChannel(std::move(proxy_channel));
 }
 
 rtc::scoped_refptr<DataChannel> PeerConnection::InternalCreateDataChannel(
@@ -2019,8 +2029,11 @@ void PeerConnection::OnDataChannelOpenMessage(
     return;
   }
 
-  observer_->OnDataChannel(
-      DataChannelProxy::Create(signaling_thread(), channel));
+  auto proxy_channel = DataChannelProxy::Create(signaling_thread(), channel);
+  // Call both the raw pointer and scoped_refptr versions of the method
+  // for compatibility.
+  observer_->OnDataChannel(proxy_channel.get());
+  observer_->OnDataChannel(std::move(proxy_channel));
 }
 
 RtpSenderInterface* PeerConnection::FindSenderById(const std::string& id) {

webrtc/api/peerconnection_unittest.cc

@@ -244,7 +244,8 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver,
       webrtc::PeerConnectionInterface::SignalingState new_state) override {
     EXPECT_EQ(pc()->signaling_state(), new_state);
   }
-  void OnAddStream(MediaStreamInterface* media_stream) override {
+  void OnAddStream(
+      rtc::scoped_refptr<MediaStreamInterface> media_stream) override {
     media_stream->RegisterObserver(this);
     for (size_t i = 0; i < media_stream->GetVideoTracks().size(); ++i) {
       const std::string id = media_stream->GetVideoTracks()[i]->id();
@@ -254,7 +255,8 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver,
           media_stream->GetVideoTracks()[i]));
     }
   }
-  void OnRemoveStream(MediaStreamInterface* media_stream) override {}
+  void OnRemoveStream(
+      rtc::scoped_refptr<MediaStreamInterface> media_stream) override {}
   void OnRenegotiationNeeded() override {}
   void OnIceConnectionChange(
       webrtc::PeerConnectionInterface::IceConnectionState new_state) override {
@@ -428,7 +430,8 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver,
                PeerConnectionInterface::RTCOfferAnswerOptions::kUndefined;
   }
 
-  void OnDataChannel(DataChannelInterface* data_channel) override {
+  void OnDataChannel(
+      rtc::scoped_refptr<DataChannelInterface> data_channel) override {
     LOG(INFO) << id_ << "OnDataChannel";
     data_channel_ = data_channel;
     data_observer_.reset(new MockDataChannelObserver(data_channel));

webrtc/api/peerconnectionfactory_unittest.cc

@@ -69,9 +69,11 @@ class NullPeerConnectionObserver : public PeerConnectionObserver {
   virtual void OnSignalingMessage(const std::string& msg) {}
   virtual void OnSignalingChange(
       PeerConnectionInterface::SignalingState new_state) {}
-  virtual void OnAddStream(MediaStreamInterface* stream) {}
-  virtual void OnRemoveStream(MediaStreamInterface* stream) {}
-  virtual void OnDataChannel(DataChannelInterface* data_channel) {}
+  virtual void OnAddStream(rtc::scoped_refptr<MediaStreamInterface> stream) {}
+  virtual void OnRemoveStream(rtc::scoped_refptr<MediaStreamInterface> stream) {
+  }
+  virtual void OnDataChannel(
+      rtc::scoped_refptr<DataChannelInterface> data_channel) {}
   virtual void OnRenegotiationNeeded() {}
   virtual void OnIceConnectionChange(
       PeerConnectionInterface::IceConnectionState new_state) {}

webrtc/api/peerconnectioninterface.h

@@ -503,27 +503,40 @@ class PeerConnectionObserver {
   virtual void OnSignalingChange(
       PeerConnectionInterface::SignalingState new_state) = 0;
 
+  // TODO(deadbeef): Once all subclasses override the scoped_refptr versions
+  // of the below three methods, make them pure virtual and remove the raw
+  // pointer version.
+
   // Triggered when media is received on a new stream from remote peer.
-  virtual void OnAddStream(MediaStreamInterface* stream) = 0;
+  virtual void OnAddStream(rtc::scoped_refptr<MediaStreamInterface> stream) {}
+  // Deprecated; please use the version that uses a scoped_refptr.
+  virtual void OnAddStream(MediaStreamInterface* stream) {}
 
   // Triggered when a remote peer close a stream.
-  virtual void OnRemoveStream(MediaStreamInterface* stream) = 0;
+  virtual void OnRemoveStream(rtc::scoped_refptr<MediaStreamInterface> stream) {
+  }
+  // Deprecated; please use the version that uses a scoped_refptr.
+  virtual void OnRemoveStream(MediaStreamInterface* stream) {}
 
-  // Triggered when a remote peer open a data channel.
-  virtual void OnDataChannel(DataChannelInterface* data_channel) = 0;
+  // Triggered when a remote peer opens a data channel.
+  virtual void OnDataChannel(
+      rtc::scoped_refptr<DataChannelInterface> data_channel){};
+  // Deprecated; please use the version that uses a scoped_refptr.
+  virtual void OnDataChannel(DataChannelInterface* data_channel) {}
 
-  // Triggered when renegotiation is needed, for example the ICE has restarted.
+  // Triggered when renegotiation is needed. For example, an ICE restart
+  // has begun.
   virtual void OnRenegotiationNeeded() = 0;
 
-  // Called any time the IceConnectionState changes
+  // Called any time the IceConnectionState changes.
   virtual void OnIceConnectionChange(
       PeerConnectionInterface::IceConnectionState new_state) = 0;
 
-  // Called any time the IceGatheringState changes
+  // Called any time the IceGatheringState changes.
   virtual void OnIceGatheringChange(
       PeerConnectionInterface::IceGatheringState new_state) = 0;
 
-  // New Ice candidate have been found.
+  // A new ICE candidate has been gathered.
   virtual void OnIceCandidate(const IceCandidateInterface* candidate) = 0;
 
   // Ice candidates have been removed.

webrtc/api/peerconnectioninterface_unittest.cc

@@ -473,16 +473,18 @@ class MockPeerConnectionObserver : public PeerConnectionObserver {
     return remote_streams_->find(label);
   }
   StreamCollectionInterface* remote_streams() const { return remote_streams_; }
-  void OnAddStream(MediaStreamInterface* stream) override {
+  void OnAddStream(rtc::scoped_refptr<MediaStreamInterface> stream) override {
     last_added_stream_ = stream;
     remote_streams_->AddStream(stream);
   }
-  void OnRemoveStream(MediaStreamInterface* stream) override {
+  void OnRemoveStream(
+      rtc::scoped_refptr<MediaStreamInterface> stream) override {
     last_removed_stream_ = stream;
     remote_streams_->RemoveStream(stream);
   }
   void OnRenegotiationNeeded() override { renegotiation_needed_ = true; }
-  void OnDataChannel(DataChannelInterface* data_channel) override {
+  void OnDataChannel(
+      rtc::scoped_refptr<DataChannelInterface> data_channel) override {
     last_datachannel_ = data_channel;
   }
 

webrtc/api/test/peerconnectiontestwrapper.cc

@@ -96,7 +96,8 @@ PeerConnectionTestWrapper::CreateDataChannel(
   return peer_connection_->CreateDataChannel(label, &init);
 }
 
-void PeerConnectionTestWrapper::OnAddStream(MediaStreamInterface* stream) {
+void PeerConnectionTestWrapper::OnAddStream(
+    rtc::scoped_refptr<MediaStreamInterface> stream) {
   LOG(LS_INFO) << "PeerConnectionTestWrapper " << name_
                << ": OnAddStream";
   // TODO(ronghuawu): support multiple streams.
@@ -116,7 +117,7 @@ void PeerConnectionTestWrapper::OnIceCandidate(
 }
 
 void PeerConnectionTestWrapper::OnDataChannel(
-    webrtc::DataChannelInterface* data_channel) {
+    rtc::scoped_refptr<webrtc::DataChannelInterface> data_channel) {
   SignalOnDataChannel(data_channel);
 }
 

webrtc/api/test/peerconnectiontestwrapper.h

@@ -43,9 +43,12 @@ class PeerConnectionTestWrapper
      webrtc::PeerConnectionInterface::SignalingState new_state) {}
   virtual void OnStateChange(
       webrtc::PeerConnectionObserver::StateType state_changed) {}
-  virtual void OnAddStream(webrtc::MediaStreamInterface* stream);
-  virtual void OnRemoveStream(webrtc::MediaStreamInterface* stream) {}
-  virtual void OnDataChannel(webrtc::DataChannelInterface* data_channel);
+  virtual void OnAddStream(
+      rtc::scoped_refptr<webrtc::MediaStreamInterface> stream);
+  virtual void OnRemoveStream(
+      rtc::scoped_refptr<webrtc::MediaStreamInterface> stream) {}
+  virtual void OnDataChannel(
+      rtc::scoped_refptr<webrtc::DataChannelInterface> data_channel);
   virtual void OnRenegotiationNeeded() {}
   virtual void OnIceConnectionChange(
       webrtc::PeerConnectionInterface::IceConnectionState new_state) {}

webrtc/base/scoped_ref_ptr.h

@@ -89,6 +89,12 @@ class scoped_refptr {
       ptr_->AddRef();
   }
 
+  // Move constructors.
+  scoped_refptr(scoped_refptr<T>&& r) : ptr_(r.release()) {}
+
+  template <typename U>
+  scoped_refptr(scoped_refptr<U>&& r) : ptr_(r.release()) {}
+
   ~scoped_refptr() {
     if (ptr_)
       ptr_->Release();

webrtc/examples/peerconnection/client/conductor.cc

@@ -156,19 +156,16 @@ void Conductor::EnsureStreamingUI() {
 //
 
 // Called when a remote stream is added
-void Conductor::OnAddStream(webrtc::MediaStreamInterface* stream) {
+void Conductor::OnAddStream(
+    rtc::scoped_refptr<webrtc::MediaStreamInterface> stream) {
   LOG(INFO) << __FUNCTION__ << " " << stream->label();
-
-  stream->AddRef();
-  main_wnd_->QueueUIThreadCallback(NEW_STREAM_ADDED,
-                                   stream);
+  main_wnd_->QueueUIThreadCallback(NEW_STREAM_ADDED, stream.release());
 }
 
-void Conductor::OnRemoveStream(webrtc::MediaStreamInterface* stream) {
+void Conductor::OnRemoveStream(
+    rtc::scoped_refptr<webrtc::MediaStreamInterface> stream) {
   LOG(INFO) << __FUNCTION__ << " " << stream->label();
-  stream->AddRef();
-  main_wnd_->QueueUIThreadCallback(STREAM_REMOVED,
-                                   stream);
+  main_wnd_->QueueUIThreadCallback(STREAM_REMOVED, stream.release());
 }
 
 void Conductor::OnIceCandidate(const webrtc::IceCandidateInterface* candidate) {

webrtc/examples/peerconnection/client/conductor.h

@@ -66,9 +66,12 @@ class Conductor
 
   void OnSignalingChange(
       webrtc::PeerConnectionInterface::SignalingState new_state) override{};
-  void OnAddStream(webrtc::MediaStreamInterface* stream) override;
-  void OnRemoveStream(webrtc::MediaStreamInterface* stream) override;
-  void OnDataChannel(webrtc::DataChannelInterface* channel) override {}
+  void OnAddStream(
+      rtc::scoped_refptr<webrtc::MediaStreamInterface> stream) override;
+  void OnRemoveStream(
+      rtc::scoped_refptr<webrtc::MediaStreamInterface> stream) override;
+  void OnDataChannel(
+      rtc::scoped_refptr<webrtc::DataChannelInterface> channel) override {}
   void OnRenegotiationNeeded() override {}
   void OnIceConnectionChange(
       webrtc::PeerConnectionInterface::IceConnectionState new_state) override{};