Fix cross-thread crash on the shared transmitMessages calling list

ft8af/app/src/main/java/com/k1af/ft8af/GeneralVariables.java

@@ -27,8 +27,10 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 
 public class GeneralVariables {
@@ -624,7 +626,17 @@ public static synchronized boolean directionalCQIsForMe(String callsignTo) {
 
     public static final ArrayList<String> followCallsign = new ArrayList<>();//Followed callsigns
 
-    public static ArrayList<Ft8Message> transmitMessages = new ArrayList<>();//List for the calling UI, followed entries
+    // The calling-UI "followed entries" list. Mutated concurrently from three
+    // threads with no external lock: the decode thread (findIncludedCallsigns
+    // add + deleteArrayListMore remove(0) + clear), the TX-sequencer thread
+    // (FT8TransmitSignal.doComplete reverse scans, onBeforeTransmit add) and the
+    // UI thread (GridTracker / GridMarkerInfoWindow add, clearTransmittingMessage).
+    // A plain ArrayList corrupts its backing array / throws
+    // IndexOutOfBounds under that contention, so this is a CopyOnWriteArrayList:
+    // every add/remove/clear is atomic. Index scans must still snapshot the list
+    // first (size() then get(i) can otherwise race a concurrent remove) — see
+    // FT8TransmitSignal.doComplete.
+    public static List<Ft8Message> transmitMessages = new CopyOnWriteArrayList<>();//List for the calling UI, followed entries
 
     public static void setMyMaidenheadGrid(String grid) {
         myMaidenheadGrid = grid;
@@ -1239,7 +1251,7 @@ public static String getCallsignAndGridToHTML() {
         return result.toString();
     }
 
-    public static synchronized void deleteArrayListMore(ArrayList<Ft8Message> list) {
+    public static synchronized void deleteArrayListMore(List<Ft8Message> list) {
         if (list.size() > GeneralVariables.MESSAGE_COUNT) {
             while (list.size() > GeneralVariables.MESSAGE_COUNT) {
                 list.remove(0);

ft8af/app/src/main/java/com/k1af/ft8af/ft8transmit/FT8TransmitSignal.java

@@ -38,6 +38,7 @@
 import com.k1af.ft8af.ui.ToastMessage;
 
 import java.util.ArrayList;
+import java.util.List;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 
@@ -1035,9 +1036,15 @@ private void doComplete() {
 
         // look up signal report from history
         // processing signal reports here because saved reports often differ from actual QSO reports
+        // Snapshot the shared transmitMessages list once: it is mutated concurrently
+        // by the decode thread (add / deleteArrayListMore remove(0) / clear), so a live
+        // size()-then-get(i) scan can throw IndexOutOfBounds when the list shrinks
+        // mid-scan. Iterating a private copy is race-free and preserves the
+        // most-recent-first (reverse index) semantics.
+        List<Ft8Message> transmitMessagesSnapshot = new ArrayList<>(GeneralVariables.transmitMessages);
         // iterate through received signal reports from the other party
-        for (int i = GeneralVariables.transmitMessages.size() - 1; i >= 0; i--) {
-            Ft8Message message = GeneralVariables.transmitMessages.get(i);
+        for (int i = transmitMessagesSnapshot.size() - 1; i >= 0; i--) {
+            Ft8Message message = transmitMessagesSnapshot.get(i);
             if ((GeneralVariables.checkFun3(message.extraInfo)
                     || GeneralVariables.checkFun2(message.extraInfo))
                     && (message.callsignFrom.equals(toCallsign.callsign)
@@ -1048,8 +1055,8 @@ private void doComplete() {
             }
         }
         // iterate through signal reports I sent to the other party
-        for (int i = GeneralVariables.transmitMessages.size() - 1; i >= 0; i--) {
-            Ft8Message message = GeneralVariables.transmitMessages.get(i);
+        for (int i = transmitMessagesSnapshot.size() - 1; i >= 0; i--) {
+            Ft8Message message = transmitMessagesSnapshot.get(i);
             if ((GeneralVariables.checkFun3(message.extraInfo)
                     || GeneralVariables.checkFun2(message.extraInfo))
                     && (message.callsignTo.equals(toCallsign.callsign)

ft8af/app/src/main/java/com/k1af/ft8af/ui/CallingListAdapter.java

@@ -29,13 +29,13 @@
 import com.k1af.ft8af.rigs.BaseRigOperation;
 import com.k1af.ft8af.timer.UtcTimer;
 
-import java.util.ArrayList;
+import java.util.List;
 
 public class CallingListAdapter extends RecyclerView.Adapter<CallingListAdapter.CallingListItemHolder> {
     public enum ShowMode{CALLING_LIST,MY_CALLING,TRACKER}
     private static final String TAG = "CallingListAdapter";
     private final MainViewModel mainViewModel;
-    private final ArrayList<Ft8Message> ft8MessageArrayList;
+    private final List<Ft8Message> ft8MessageArrayList;
     private final Context context;
 
     private final ShowMode showMode;
@@ -126,7 +126,7 @@ public void onCreateContextMenu(ContextMenu contextMenu, View view, ContextMenu.
 
 
     public CallingListAdapter(Context context, MainViewModel mainViewModel
-            , ArrayList<Ft8Message> messages, ShowMode showMode) {
+            , List<Ft8Message> messages, ShowMode showMode) {
         this.mainViewModel = mainViewModel;
         this.context = context;
         this.showMode=showMode;

ft8af/app/src/test/java/com/k1af/ft8af/TransmitMessagesConcurrencyTest.java

@@ -0,0 +1,130 @@
+package com.k1af.ft8af;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.RobolectricTestRunner;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Coverage for the thread-safety of {@link GeneralVariables#transmitMessages}, the
+ * calling-UI "followed entries" list.
+ *
+ * <p>That list is mutated with no external lock from three threads at once: the decode
+ * thread ({@code MainViewModel.findIncludedCallsigns} appends matches then trims the
+ * front via {@link GeneralVariables#deleteArrayListMore} {@code remove(0)}, and
+ * {@code clearTransmittingMessage} clears it), the TX-sequencer thread
+ * ({@code FT8TransmitSignal.doComplete} reverse-scans it for the QSO signal reports,
+ * {@code onBeforeTransmit} appends), and the UI thread ({@code GridTrackerMainActivity}
+ * / {@code GridMarkerInfoWindow} append). When it was a plain {@code ArrayList} that
+ * contention corrupted its backing array and the reverse {@code size()}-then-{@code get(i)}
+ * scan threw {@link IndexOutOfBoundsException} on the TX thread. It is now a
+ * {@link CopyOnWriteArrayList} (atomic add/remove/clear) and the scan snapshots the list
+ * before indexing it.
+ *
+ * <p>Robolectric is only needed to construct real {@link Ft8Message} rows (their ctor
+ * touches {@code android.util.Log}); the list operations under test touch no Android types.
+ */
+@RunWith(RobolectricTestRunner.class)
+public class TransmitMessagesConcurrencyTest {
+
+    private static List<Ft8Message> messages(int n) {
+        List<Ft8Message> list = new ArrayList<>();
+        for (int i = 0; i < n; i++) {
+            list.add(new Ft8Message(FT8Common.FT8_MODE));
+        }
+        return list;
+    }
+
+    /** Reproduce the production reverse scan from {@code FT8TransmitSignal.doComplete}. */
+    private static void reverseScanLikeDoComplete(List<Ft8Message> shared) {
+        List<Ft8Message> snapshot = new ArrayList<>(shared);
+        for (int i = snapshot.size() - 1; i >= 0; i--) {
+            Ft8Message message = snapshot.get(i);
+            // touch a field, exactly as doComplete dereferences message.extraInfo etc.
+            if (message == null) {
+                throw new AssertionError("snapshot yielded a null element");
+            }
+        }
+    }
+
+    @Before
+    public void resetList() {
+        GeneralVariables.transmitMessages.clear();
+    }
+
+    @After
+    public void clearList() {
+        GeneralVariables.transmitMessages.clear();
+    }
+
+    @Test
+    public void transmitMessages_isCopyOnWriteList() {
+        // A plain ArrayList cannot be shared across the decode / TX / UI threads without
+        // external locking; the field must be a thread-safe list.
+        assertThat(GeneralVariables.transmitMessages).isInstanceOf(CopyOnWriteArrayList.class);
+    }
+
+    @Test
+    public void reverseScanSnapshot_survivesConcurrentDecodeMutation() throws InterruptedException {
+        GeneralVariables.transmitMessages.addAll(messages(64));
+        final AtomicReference<Throwable> failure = new AtomicReference<>();
+        final int iterations = 20_000;
+
+        // Mirror the decode / UI writers: append, trim the front, and occasionally clear.
+        Thread mutator = new Thread(() -> {
+            try {
+                for (int i = 0; i < iterations; i++) {
+                    GeneralVariables.transmitMessages.add(new Ft8Message(FT8Common.FT8_MODE));
+                    GeneralVariables.deleteArrayListMore(GeneralVariables.transmitMessages);
+                    if (i % 128 == 0) {
+                        GeneralVariables.transmitMessages.clear();
+                    }
+                }
+            } catch (Throwable t) {
+                failure.compareAndSet(null, t);
+            }
+        });
+
+        mutator.start();
+        try {
+            // Mirror the TX-sequencer thread reading the same shared list.
+            for (int i = 0; i < iterations; i++) {
+                reverseScanLikeDoComplete(GeneralVariables.transmitMessages);
+            }
+        } catch (Throwable t) {
+            failure.compareAndSet(null, t);
+        }
+        mutator.join();
+
+        assertThat(failure.get()).isNull();
+    }
+
+    @Test
+    public void directSizeThenGet_onShrunkList_throws_butSnapshotIsImmune() {
+        // The pre-fix hazard, made deterministic: the reverse scan captured size() and then
+        // indexed the live list; a concurrent trim/clear between the two shrank it, so the
+        // stale top index was out of range.
+        List<Ft8Message> live = new ArrayList<>(messages(3));
+        int staleSize = live.size();            // TX thread saw size == 3
+        live.clear();                           // decode thread cleared the list in the window
+        assertThrows(IndexOutOfBoundsException.class, () -> live.get(staleSize - 1));
+
+        // The fix snapshots the list first, decoupling the scan from later mutation.
+        List<Ft8Message> live2 = new ArrayList<>(messages(3));
+        List<Ft8Message> snapshot = new ArrayList<>(live2);
+        live2.clear();                          // mutate the shared list after the snapshot
+        assertThat(snapshot).hasSize(3);
+        for (int i = snapshot.size() - 1; i >= 0; i--) {
+            assertThat(snapshot.get(i)).isNotNull();   // never throws
+        }
+    }
+}