attempt to fix data race in call initiation

Author: or-else

app/src/main/java/co/tinode/tindroid/Cache.java

@@ -44,8 +44,8 @@ public class Cache {
     // Currently active topic.
     private String mTopicSelected = null;
 
-    // Current video call.
-    private CallInProgress mCallInProgress = null;
+    // Current video call. Volatile for visibility across threads.
+    private volatile CallInProgress mCallInProgress = null;
 
     private boolean mFCMTokenRequested = false;
 
@@ -187,47 +187,57 @@ static void invalidate() {
     }
 
     public static CallInProgress getCallInProgress() {
-        return sInstance.mCallInProgress;
+        synchronized (sInstance) {
+            return sInstance.mCallInProgress;
+        }
     }
 
     public static void prepareNewCall(@NonNull String topic, int seq, @Nullable CallConnection conn) {
-        if (sInstance.mCallInProgress == null) {
-            sInstance.mCallInProgress = new CallInProgress(topic, seq, conn);
-        } else if (!sInstance.mCallInProgress.equals(topic, seq)) {
-            // Include stacktrace to identify the caller which attempted to start a conflicting call.
-            throw new IllegalStateException("prepareNewCall called while another call is in progress"  +
-                    "\n\tNew: " + topic + ":" + seq);
+        synchronized (sInstance) {
+            if (sInstance.mCallInProgress == null) {
+                sInstance.mCallInProgress = new CallInProgress(topic, seq, conn);
+            } else if (!sInstance.mCallInProgress.equals(topic, seq)) {
+                // Include stacktrace to identify the caller which attempted to start a conflicting call.
+                throw new IllegalStateException("prepareNewCall called while another call is in progress" +
+                        "\n\tNew: " + topic + ":" + seq);
+            }
         }
     }
 
     public static void setCallActive(String topic, int seqId) {
-        if (sInstance.mCallInProgress != null) {
-            try {
-                sInstance.mCallInProgress.setCallActive(topic, seqId);
-            } catch (IllegalArgumentException iae) {
-                // Preserve fatal behavior but add context about the cache state.
-                throw new IllegalArgumentException("setCallActive failed. Existing=" +
-                        sInstance.mCallInProgress +
-                        " New=" + topic + ":" + seqId + "; err=" + iae.getMessage());
+        synchronized (sInstance) {
+            if (sInstance.mCallInProgress != null) {
+                try {
+                    sInstance.mCallInProgress.setCallActive(topic, seqId);
+                } catch (IllegalArgumentException iae) {
+                    // Preserve fatal behavior but add context about the cache state.
+                    throw new IllegalArgumentException("setCallActive failed. Existing=" +
+                            sInstance.mCallInProgress +
+                            " New=" + topic + ":" + seqId + "; err=" + iae.getMessage());
+                }
+            } else {
+                throw new IllegalStateException("setCallActive without prepareNewCall, New="
+                        + topic + ":" + seqId);
             }
-        } else {
-            throw new IllegalStateException("setCallActive without prepareNewCall, New="
-                    + topic + ":" + seqId);
         }
     }
 
     public static void setCallConnected() {
-        if (sInstance.mCallInProgress != null) {
-            sInstance.mCallInProgress.setCallConnected();
-        } else {
-            Log.e(TAG, "Attempt to mark call connected with no configured call");
+        synchronized (sInstance) {
+            if (sInstance.mCallInProgress != null) {
+                sInstance.mCallInProgress.setCallConnected();
+            } else {
+                Log.e(TAG, "Attempt to mark call connected with no configured call");
+            }
         }
     }
 
     public static void endCallInProgress() {
-        if (sInstance.mCallInProgress != null) {
-            sInstance.mCallInProgress.endCall();
-            sInstance.mCallInProgress = null;
+        synchronized (sInstance) {
+            if (sInstance.mCallInProgress != null) {
+                sInstance.mCallInProgress.endCall();
+                sInstance.mCallInProgress = null;
+            }
         }
     }
 

app/src/main/java/co/tinode/tindroid/CallManager.java

@@ -233,6 +233,22 @@ public static void acceptIncomingCall(Context context, String caller, int seq, b
 
     public static void showOutgoingCallUi(Context context, String topicName,
                                           boolean audioOnly, CallConnection conn) {
+        CallInProgress existingCall = Cache.getCallInProgress();
+        if (existingCall != null) {
+            if (existingCall.isConnected() || existingCall.isConnectionUseful()) {
+                // A real active call is in progress (ringing or connected). Don't clobber it.
+                Log.w(TAG, "Cannot start outgoing call: another call is active: " + existingCall);
+                if (conn != null) {
+                    conn.setDisconnected(new android.telecom.DisconnectCause(
+                            android.telecom.DisconnectCause.BUSY));
+                    conn.destroy();
+                }
+                return;
+            }
+            // Stale/zombie call left over from a previous session. Clean it up.
+            Log.w(TAG, "Ending stale call before starting new outgoing call: " + existingCall);
+            Cache.endCallInProgress();
+        }
         Cache.prepareNewCall(topicName, 0, conn);
 
         Intent intent = new Intent(context, CallActivity.class);

app/src/main/java/co/tinode/tindroid/services/CallConnectionService.java

@@ -73,6 +73,8 @@ public Connection onCreateIncomingConnection(@Nullable PhoneAccountHandle connec
 
         conn.setConnectionProperties(Connection.PROPERTY_SELF_MANAGED);
 
+        // This throws but shouldn't. We are not catching here to be able to find
+        // the root cause of the problem (data race).
         Cache.prepareNewCall(callerUri.getSchemeSpecificPart(), seq, conn);
 
         conn.setConnectionCapabilities(Connection.CAPABILITY_MUTE);