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

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

 /*
  * libjingle
  * Copyright 2013 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 21 matching lines @@
 import org.webrtc.PeerConnection.SignalingState;
 
 import java.io.File;
 import java.lang.ref.WeakReference;
 import java.nio.ByteBuffer;
 import java.nio.charset.Charset;
 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;
 import java.util.concurrent.TimeUnit;
 
 import static junit.framework.Assert.*;
 
 /** End-to-end tests for PeerConnection.java. */
 public class PeerConnectionTest {
   // Set to true to render video.
@@ skipping 29 matching lines @@
     private DataChannel dataChannel;
     private LinkedList<DataChannel.Buffer> expectedBuffers =
         new LinkedList<DataChannel.Buffer>();
     private LinkedList<DataChannel.State> expectedStateChanges =
         new LinkedList<DataChannel.State>();
     private LinkedList<String> expectedRemoteDataChannelLabels =
         new LinkedList<String>();
     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;
     }
 
     public synchronized void setDataChannel(DataChannel dataChannel) {
       assertNull(this.dataChannel);
       this.dataChannel = dataChannel;
       this.dataChannel.registerObserver(this);
       assertNotNull(this.dataChannel);
@@ skipping 105 matching lines @@
       assertEquals(1, stream.videoTracks.size());
       assertEquals(1, stream.audioTracks.size());
       assertTrue(stream.videoTracks.get(0).id().endsWith("VideoTrack"));
       assertTrue(stream.audioTracks.get(0).id().endsWith("AudioTrack"));
       assertEquals("video", stream.videoTracks.get(0).kind());
       assertEquals("audio", stream.audioTracks.get(0).kind());
       VideoRenderer renderer = createVideoRenderer(this);
       stream.videoTracks.get(0).addRenderer(renderer);
       assertNull(renderers.put(
           stream, new WeakReference<VideoRenderer>(renderer)));
+      gotRemoteStreams.add(stream);
     }
 
     public synchronized void expectRemoveStream(String label) {
       expectedRemoveStreamLabels.add(label);
     }
 
     @Override
     public synchronized void onRemoveStream(MediaStream stream) {
       assertEquals(expectedRemoveStreamLabels.removeFirst(), stream.label());
       WeakReference<VideoRenderer> renderer = renderers.remove(stream);
       assertNotNull(renderer);
       assertNotNull(renderer.get());
       assertEquals(1, stream.videoTracks.size());
       stream.videoTracks.get(0).removeRenderer(renderer.get());
+      gotRemoteStreams.remove(stream);
     }
 
     public synchronized void expectDataChannel(String label) {
       expectedRemoteDataChannelLabels.add(label);
     }
 
     @Override
     public synchronized void onDataChannel(DataChannel remoteDataChannel) {
       assertEquals(expectedRemoteDataChannelLabels.removeFirst(),
                    remoteDataChannel.label());
@@ skipping 469 matching lines @@
     offeringExpectations.dataChannel.close();
 
     if (RENDER_TO_GUI) {
       try {
         Thread.sleep(3000);
       } catch (Throwable t) {
         throw new RuntimeException(t);
       }
     }
 
+    // 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.
     // - audit each place that uses |constraints| for specifying non-trivial
     //   constraints (and ensure they're honored).
     // - test error cases
     // - ensure reasonable coverage of _jni.cc is achieved.  Coverage is
     //   extra-important because of all the free-text (class/method names, etc)
     //   in JNI-style programming; make sure no typos!
     // - Test that shutdown mid-interaction is crash-free.
 
     // 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.
     shutdownPC(offeringPC, offeringExpectations);
     offeringPC = null;
     shutdownPC(answeringPC, answeringExpectations);
     answeringPC = null;
+    offererVideoTrack.dispose();
     videoSource.dispose();
     factory.dispose();
     System.gc();
   }
 
   private static void shutdownPC(
       PeerConnection pc, ObserverExpectations expectations) {
     expectations.dataChannel.unregisterObserver();
     expectations.dataChannel.dispose();
     expectations.expectStatsCallback();
@@ skipping 26 matching lines @@
     TreeSet<String> threads = new TreeSet<String>();
     // This pokes at /proc instead of using the Java APIs because we're also
     // looking for libjingle/webrtc native threads, most of which won't have
     // attached to the JVM.
     for (String threadId : (new File("/proc/self/task")).list()) {
       threads.add(threadId);
     }
     return threads;
   }
 }