Fixing bug in MediaStream.java that caused double disposal of track.

talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java

Index: talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java
diff --git a/talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java b/talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java
index 50f4d7317f33a11e146ef18af96d63164f833956..bd72fac32220622f0ef825c9bddbb61cdbc72ec2 100644
--- a/talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java
+++ b/talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java
@@ -39,6 +39,7 @@ import java.util.Arrays;
 import java.util.IdentityHashMap;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.TreeSet;
 import java.util.concurrent.CountDownLatch;
@@ -88,6 +89,8 @@ public class PeerConnectionTest {
     private int expectedStatsCallbacks = 0;
     private LinkedList<StatsReport[]> gotStatsReports =
         new LinkedList<StatsReport[]>();
+    private final HashSet<MediaStream> gotRemoteStreams =
+        new HashSet<MediaStream>();
 
     public ObserverExpectations(String name) {
       this.name = name;
@@ -213,6 +216,7 @@ public class PeerConnectionTest {
       stream.videoTracks.get(0).addRenderer(renderer);
       assertNull(renderers.put(
           stream, new WeakReference<VideoRenderer>(renderer)));
+      gotRemoteStreams.add(stream);
     }
 
     public synchronized void expectRemoveStream(String label) {
@@ -227,6 +231,7 @@ public class PeerConnectionTest {
       assertNotNull(renderer.get());
       assertEquals(1, stream.videoTracks.size());
       stream.videoTracks.get(0).removeRenderer(renderer.get());
+      gotRemoteStreams.remove(stream);
     }
 
     public synchronized void expectDataChannel(String label) {
@@ -716,6 +721,66 @@ public class PeerConnectionTest {
       }
     }
 
+    // Do another negotiation, removing the video track from one peer.

[AlexG, 2016/01/26 22:03:36]

I think it should be under the boolean flag in doTest() and moved to separate
function - excersiceTrackRemoval()?
doTest() is already too big.

[Taylor Brandstetter, 2016/01/26 22:43:10]

I just went ahead and created a separate test method.

+    // This previously caused a crash on pc.dispose().
+    // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5128
+    VideoTrack offererVideoTrack = oLMS.get().videoTracks.get(0);
+    // Note that when we call removeTrack, we regain responsibility for
+    // disposing of the track.
+    oLMS.get().removeTrack(offererVideoTrack);
+    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 remote offer.
+    sdpLatch = new SdpObserverLatch();
+    answeringExpectations.expectSignalingChange(
+        SignalingState.HAVE_REMOTE_OFFER);
+    answeringPC.setRemoteDescription(sdpLatch, offerSdp);
+    assertEquals(
+        PeerConnection.SignalingState.STABLE, offeringPC.signalingState());
+    assertTrue(sdpLatch.await());
+    assertNull(sdpLatch.getSdp());
+
+    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 answer.
+    sdpLatch = new SdpObserverLatch();
+    answeringExpectations.expectSignalingChange(SignalingState.STABLE);
+    answeringPC.setLocalDescription(sdpLatch, answerSdp);
+    assertTrue(sdpLatch.await());
+    assertNull(sdpLatch.getSdp());
+
+    // Set local offer.
+    sdpLatch = new SdpObserverLatch();
+    offeringExpectations.expectSignalingChange(SignalingState.HAVE_LOCAL_OFFER);
+    offeringPC.setLocalDescription(sdpLatch, offerSdp);
+    assertTrue(sdpLatch.await());
+    assertNull(sdpLatch.getSdp());
+
+    // Set remote answer.
+    sdpLatch = new SdpObserverLatch();
+    offeringExpectations.expectSignalingChange(SignalingState.STABLE);
+    offeringPC.setRemoteDescription(sdpLatch, answerSdp);
+    assertTrue(sdpLatch.await());
+    assertNull(sdpLatch.getSdp());
+
+    // Make sure the track was really removed.
+    // TODO(deadbeef): Currently the expectation is that the video track's
+    // state will be set to "ended". However, in the future, it's likely that
+    // the video track will be completely removed from the remote stream
+    // (as it is on the C++ level).
+    MediaStream aRMS = answeringExpectations.gotRemoteStreams.iterator().next();
+    assertEquals(aRMS.videoTracks.get(0).state(), MediaStreamTrack.State.ENDED);
+
     // TODO(fischman) MOAR test ideas:
     // - Test that PC.removeStream() works; requires a second
     //   createOffer/createAnswer dance.
@@ -734,6 +799,7 @@ public class PeerConnectionTest {
     offeringPC = null;
     shutdownPC(answeringPC, answeringExpectations);
     answeringPC = null;
+    offererVideoTrack.dispose();
     videoSource.dispose();
     factory.dispose();
     System.gc();