From 192bd5cc20756f3d2ea11320988171d6801adec8 Mon Sep 17 00:00:00 2001 From: Optio Agent Date: Mon, 13 Jul 2026 15:19:35 +0000 Subject: [PATCH] Fix cross-thread crash on the shared transmitMessages calling list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GeneralVariables.transmitMessages (the calling-UI "followed entries" list) was a plain ArrayList mutated concurrently with no external lock from three threads: * decode thread — MainViewModel.findIncludedCallsigns appends matches then trims the front via deleteArrayListMore (remove(0)), and clearTransmittingMessage clears it every cycle; * TX-sequencer — FT8TransmitSignal.doComplete reverse-scans it for the QSO signal reports (size()-1 .. 0, get(i)), onBeforeTransmit appends; * UI thread — GridTrackerMainActivity / GridMarkerInfoWindow append. Under that contention the ArrayList's backing array is corrupted and the reverse size()-then-get(i) scan on the TX thread throws IndexOutOfBounds when a concurrent remove(0)/clear shrinks the list mid-scan — an uncaught crash on the TX-sequencer thread at QSO completion. Root cause: an unsynchronised mutable collection shared across threads. Fix: * make transmitMessages a CopyOnWriteArrayList so every add/remove/clear is atomic (widen deleteArrayListMore + CallingListAdapter to List, both source-compatible; ft8Messages is untouched); * snapshot the list once in FT8TransmitSignal.doComplete before the two reverse index scans so size()/get() cannot race a concurrent shrink. The private copy preserves the exact most-recent-first semantics. Happy path (single-threaded) is byte-identical. Adds TransmitMessagesConcurrencyTest: the field is a CopyOnWriteArrayList, the snapshot reverse scan survives 20k iterations of concurrent decode-thread mutation, and a deterministic case showing a live size()/get() throws on a shrunk list while a snapshot is immune. The first two fail pre-fix. Verified: :app:testDebugUnitTest and :app:assembleDebug (4 ABIs) both green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../java/com/k1af/ft8af/GeneralVariables.java | 16 ++- .../ft8af/ft8transmit/FT8TransmitSignal.java | 15 +- .../com/k1af/ft8af/ui/CallingListAdapter.java | 6 +- .../TransmitMessagesConcurrencyTest.java | 130 ++++++++++++++++++ 4 files changed, 158 insertions(+), 9 deletions(-) create mode 100644 ft8af/app/src/test/java/com/k1af/ft8af/TransmitMessagesConcurrencyTest.java diff --git a/ft8af/app/src/main/java/com/k1af/ft8af/GeneralVariables.java b/ft8af/app/src/main/java/com/k1af/ft8af/GeneralVariables.java index 19ce4ee7..2b9559ec 100644 --- a/ft8af/app/src/main/java/com/k1af/ft8af/GeneralVariables.java +++ b/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 followCallsign = new ArrayList<>();//Followed callsigns - public static ArrayList 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 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 list) { + public static synchronized void deleteArrayListMore(List list) { if (list.size() > GeneralVariables.MESSAGE_COUNT) { while (list.size() > GeneralVariables.MESSAGE_COUNT) { list.remove(0); diff --git a/ft8af/app/src/main/java/com/k1af/ft8af/ft8transmit/FT8TransmitSignal.java b/ft8af/app/src/main/java/com/k1af/ft8af/ft8transmit/FT8TransmitSignal.java index ee7c43aa..7aec4088 100644 --- a/ft8af/app/src/main/java/com/k1af/ft8af/ft8transmit/FT8TransmitSignal.java +++ b/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 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) diff --git a/ft8af/app/src/main/java/com/k1af/ft8af/ui/CallingListAdapter.java b/ft8af/app/src/main/java/com/k1af/ft8af/ui/CallingListAdapter.java index e4471513..06d81e76 100644 --- a/ft8af/app/src/main/java/com/k1af/ft8af/ui/CallingListAdapter.java +++ b/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 { public enum ShowMode{CALLING_LIST,MY_CALLING,TRACKER} private static final String TAG = "CallingListAdapter"; private final MainViewModel mainViewModel; - private final ArrayList ft8MessageArrayList; + private final List 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 messages, ShowMode showMode) { + , List messages, ShowMode showMode) { this.mainViewModel = mainViewModel; this.context = context; this.showMode=showMode; diff --git a/ft8af/app/src/test/java/com/k1af/ft8af/TransmitMessagesConcurrencyTest.java b/ft8af/app/src/test/java/com/k1af/ft8af/TransmitMessagesConcurrencyTest.java new file mode 100644 index 00000000..189fadb6 --- /dev/null +++ b/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. + * + *

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. + * + *

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 messages(int n) { + List 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 shared) { + List 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 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 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 live2 = new ArrayList<>(messages(3)); + List 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 + } + } +}