Phillip Davis created
- ConversationGetMucOptionsRaceTest is in its own file to make the
instrumentation barriers a little easier to see and enforce
- It simulates basically the following situation:
i. On app startup, all accounts connect to all MUCs, which
eventually triggers joinMuc, which calls
Conversation.resetMucOptions.
This happens on account's individual XMPP connection Thread.
ii. At the same time, on the UI thread, we bind
ConversationsOverviewFragment, which eventually leads to
ConversationAdapter.onBindViewHolder, which calls getMucOptions.
iii. Conversation.resetMucOptions is not synchronized, so it can
run after the null-check in getMucOptions, resulting in a null
return value.
- Used AtomicReference instead of synchronizing resetMucOptions
because joinMuc is run on a directExecutor associated with the
XmppConnection thread. While the synchronized getMucOptions was
called right after this in the existing code, the documentation for
directExecutor has this to say:
When a ListenableFuture listener is registered to run under
directExecutor, the listener can execute in any of three possible
threads:
- When a thread attaches a listener to a ListenableFuture that's
already complete, the listener runs immediately in that thread.
- When a thread attaches a listener to a ListenableFuture that's
incomplete and the ListenableFuture later completes normally,
the listener runs in the thread that completes the ListenableFuture.
- When a listener is attached to a ListenableFuture and the
ListenableFuture gets cancelled, the listener runs immediately
in the thread that cancelled the Future.
- (This is applicable, since directExecutor uses in
XmppConnectionService are extensively attached to ListenableFuture)
The docs continue:
A specific warning about locking: Code that executes user-supplied
tasks, such as ListenableFuture listeners, should take care not to
do so while holding a lock. Additionally, as a further line of
defense, prefer not to perform any locking inside a task that will
be run under directExecutor: Not only might the wait for a lock
be long, but if the running thread was holding a lock, the
listener may deadlock or break lock isolation.
- Altogether, AtomicReference seems much more aligned with how Google
intends directExecutor to be used.
Anyway, here's the stacktrace that reported this error:
```
2026-02-20 10:41:09.089 7610-7610 AndroidRuntime com.cheogram.android E FATAL EXCEPTION: main (Ask Gemini)
Process: com.cheogram.android, PID: 7610
java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String eu.siacs.conversations.entities.MucOptions.getName()' on a null object reference
at eu.siacs.conversations.entities.Conversation.getName(Conversation.java:1068)
at eu.siacs.conversations.entities.Conversation.getAvatarName(Conversation.java:1784)
at eu.siacs.conversations.ui.util.AvatarWorkerTask.setContentDescription(AvatarWorkerTask.java:135)
at eu.siacs.conversations.ui.util.AvatarWorkerTask.loadAvatar(AvatarWorkerTask.java:105)
at eu.siacs.conversations.ui.adapter.ConversationAdapter.onBindViewHolder(ConversationAdapter.java:255)
at eu.siacs.conversations.ui.adapter.ConversationAdapter.onBindViewHolder(ConversationAdapter.java:33)
at androidx.recyclerview.widget.RecyclerView$Adapter.onBindViewHolder(RecyclerView.java:7846)
at androidx.recyclerview.widget.RecyclerView$Adapter.bindViewHolder(RecyclerView.java:7953)
at androidx.recyclerview.widget.RecyclerView$Recycler.tryBindViewHolderByDeadline(RecyclerView.java:6742)
at androidx.recyclerview.widget.RecyclerView$Recycler.tryGetViewHolderForPositionByDeadline(RecyclerView.java:7013)
at androidx.recyclerview.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:6853)
at androidx.recyclerview.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:6849)
at androidx.recyclerview.widget.LinearLayoutManager$LayoutState.next(LinearLayoutManager.java:2422)
at androidx.recyclerview.widget.LinearLayoutManager.layoutChunk(LinearLayoutManager.java:1722)
at androidx.recyclerview.widget.LinearLayoutManager.fill(LinearLayoutManager.java:1682)
at androidx.recyclerview.widget.LinearLayoutManager.onLayoutChildren(LinearLayoutManager.java:747)
at androidx.recyclerview.widget.RecyclerView.dispatchLayoutStep2(RecyclerView.java:4737)
at androidx.recyclerview.widget.RecyclerView.dispatchLayout(RecyclerView.java:4459)
at androidx.recyclerview.widget.RecyclerView.onLayout(RecyclerView.java:5011)
at android.view.View.layout(View.java:25626)
at android.view.ViewGroup.layout(ViewGroup.java:6460)
at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1891)
at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1729)
at android.widget.LinearLayout.onLayout(LinearLayout.java:1638)
at android.view.View.layout(View.java:25626)
at android.view.ViewGroup.layout(ViewGroup.java:6460)
at androidx.coordinatorlayout.widget.CoordinatorLayout.layoutChild(CoordinatorLayout.java:1213)
at androidx.coordinatorlayout.widget.CoordinatorLayout.onLayoutChild(CoordinatorLayout.java:899)
at androidx.coordinatorlayout.widget.CoordinatorLayout.onLayout(CoordinatorLayout.java:919)
at android.view.View.layout(View.java:25626)
at android.view.ViewGroup.layout(ViewGroup.java:6460)
at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332)
at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
at android.view.View.layout(View.java:25626)
at android.view.ViewGroup.layout(ViewGroup.java:6460)
at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1891)
at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1729)
at android.widget.LinearLayout.onLayout(LinearLayout.java:1638)
at android.view.View.layout(View.java:25626)
at android.view.ViewGroup.layout(ViewGroup.java:6460)
at androidx.drawerlayout.widget.DrawerLayout.onLayout(DrawerLayout.java:1273)
at android.view.View.layout(View.java:25626)
at android.view.ViewGroup.layout(ViewGroup.java:6460)
at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332)
at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
at android.view.View.layout(View.java:25626)
at android.view.ViewGroup.layout(ViewGroup.java:6460)
at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332)
at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
at android.view.View.layout(View.java:25626)
at android.view.ViewGroup.layout(ViewGroup.java:6460)
```build.gradle | 3
src/main/java/eu/siacs/conversations/entities/Conversation.java | 20
src/main/java/eu/siacs/conversations/xmpp/manager/MultiUserChatManager.java | 3
src/test/java/eu/siacs/conversations/entities/ConversationGetMucOptionsRaceTest.java | 183
4 files changed, 198 insertions(+), 11 deletions(-)
@@ -134,6 +134,9 @@ dependencies {
testImplementation 'junit:junit:4.13.2'
testImplementation 'org.robolectric:robolectric:4.14.1'
+ testImplementation 'org.mockito:mockito-core:5.14.2'
+ testImplementation 'net.bytebuddy:byte-buddy:1.15.11'
+ testImplementation 'net.bytebuddy:byte-buddy-agent:1.15.11'
androidTestImplementation 'androidx.test.ext:junit:1.2.1'
}
@@ -110,6 +110,7 @@ import java.util.HashSet;
import java.util.List;
import java.util.ListIterator;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Collectors;
import java.util.Set;
import java.util.Timer;
@@ -218,7 +219,7 @@ public class Conversation extends AbstractEntity
private int mode;
private final JSONObject attributes;
private Jid nextCounterpart;
- private transient MucOptions mucOptions = null;
+ private final transient AtomicReference<MucOptions> mucOptions = new AtomicReference<>();
private boolean messagesLeftOnServer = true;
private ChatState mOutgoingChatState = Config.DEFAULT_CHAT_STATE;
private ChatState mIncomingChatState = Config.DEFAULT_CHAT_STATE;
@@ -1141,15 +1142,16 @@ public class Conversation extends AbstractEntity
return getMucOptions().isPrivateAndNonAnonymous();
}
- public synchronized MucOptions getMucOptions() {
- if (this.mucOptions == null) {
- this.mucOptions = new MucOptions(this);
- }
- return this.mucOptions;
+ public @NonNull MucOptions getMucOptions() {
+ return this.mucOptions.updateAndGet(
+ existing -> existing != null ? existing : new MucOptions(this)
+ );
}
- public void resetMucOptions() {
- this.mucOptions = null;
+ public @NonNull MucOptions resetMucOptions() {
+ return this.mucOptions.updateAndGet(
+ ignoredExisting -> new MucOptions(this)
+ );
}
public void setContactJid(final Jid jid) {
@@ -1344,7 +1346,7 @@ public class Conversation extends AbstractEntity
public boolean storeInCache() {
if ("cache".equals(getAttribute("storeMedia"))) return true;
if ("shared".equals(getAttribute("storeMedia"))) return false;
- if (mode == Conversation.MODE_MULTI && !mucOptions.isPrivateAndNonAnonymous()) return true;
+ if (mode == Conversation.MODE_MULTI && !getMucOptions().isPrivateAndNonAnonymous()) return true;
return false;
}
@@ -87,8 +87,7 @@ public class MultiUserChatManager extends AbstractManager {
if (Config.MUC_LEAVE_BEFORE_JOIN) {
unavailable(conversation);
}
- conversation.resetMucOptions();
- conversation.getMucOptions().setAutoPushConfiguration(autoPushConfiguration);
+ conversation.resetMucOptions().setAutoPushConfiguration(autoPushConfiguration);
conversation.setHasMessagesLeftOnServer(false);
final var disco = fetchDiscoInfo(conversation);
@@ -0,0 +1,183 @@
+package eu.siacs.conversations.entities;
+
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.RobolectricTestRunner;
+import org.robolectric.annotation.Config;
+import org.robolectric.annotation.ConscryptMode;
+
+import android.os.Build;
+import eu.siacs.conversations.Conversations;
+import eu.siacs.conversations.xmpp.Jid;
+import junit.framework.Assert;
+import net.bytebuddy.ByteBuddy;
+import net.bytebuddy.asm.AsmVisitorWrapper;
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+import net.bytebuddy.implementation.Implementation;
+import net.bytebuddy.jar.asm.MethodVisitor;
+import net.bytebuddy.jar.asm.Opcodes;
+import net.bytebuddy.jar.asm.Type;
+import net.bytebuddy.pool.TypePool;
+
+@RunWith(RobolectricTestRunner.class)
+@Config(sdk = Build.VERSION_CODES.TIRAMISU, application = Conversations.class)
+@ConscryptMode(ConscryptMode.Mode.OFF)
+public class ConversationGetMucOptionsRaceTest {
+
+ static int fieldReadCount;
+ static volatile CountDownLatch remainingReads;
+ static volatile CountDownLatch resetDone;
+
+ public static void gate() {
+ final var reads = remainingReads;
+ final var reset = resetDone;
+ if (reads == null || reset == null) return;
+ final boolean lastRead = reads.getCount() == 1;
+ reads.countDown();
+ if (lastRead) {
+ try {
+ reset.await();
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ }
+
+ static class GetMucOptionsInstrumentor extends MethodVisitor {
+ private int count = 0;
+
+ GetMucOptionsInstrumentor(MethodVisitor delegate) {
+ super(Opcodes.ASM9, delegate);
+ }
+
+ @Override
+ public void visitFieldInsn(
+ int opcode, String owner, String name, String descriptor
+ ) {
+ if (opcode == Opcodes.GETFIELD && "mucOptions".equals(name)) {
+ count++;
+ super.visitMethodInsn(
+ Opcodes.INVOKESTATIC,
+ Type.getInternalName(
+ ConversationGetMucOptionsRaceTest.class),
+ "gate",
+ "()V",
+ false
+ );
+ }
+ super.visitFieldInsn(opcode, owner, name, descriptor);
+ }
+
+ @Override
+ public void visitEnd() {
+ fieldReadCount = count;
+ super.visitEnd();
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ @BeforeClass
+ public static void instrumentConversation() throws Exception {
+ Class.forName("net.bytebuddy.agent.ByteBuddyAgent")
+ .getMethod("install")
+ .invoke(null);
+
+ final var strategy = (ClassLoadingStrategy<ClassLoader>)
+ Class.forName(
+ "net.bytebuddy.dynamic.loading.ClassReloadingStrategy")
+ .getMethod("fromInstalledAgent")
+ .invoke(null);
+
+ new ByteBuddy()
+ .redefine(Conversation.class)
+ .visit(new AsmVisitorWrapper.ForDeclaredMethods()
+ .method(
+ named("getMucOptions"),
+ new AsmVisitorWrapper.ForDeclaredMethods
+ .MethodVisitorWrapper() {
+ @Override
+ public MethodVisitor wrap(
+ TypeDescription instrumentedType,
+ MethodDescription instrumentedMethod,
+ MethodVisitor methodVisitor,
+ Implementation.Context implementationContext,
+ TypePool typePool,
+ int writerFlags,
+ int readerFlags
+ ) {
+ return new GetMucOptionsInstrumentor(
+ methodVisitor);
+ }
+ }
+ )
+ )
+ .make()
+ .load(Conversation.class.getClassLoader(), strategy);
+ }
+
+ @Test
+ public void testGetMucOptionsNeverReturnsNull() throws Throwable {
+ final var account = mock(Account.class);
+ when(account.getJid()).thenReturn(
+ Jid.ofLocalAndDomain("testAccount", "example.org"));
+
+ final var conversation = new Conversation(
+ "Test MUC",
+ account,
+ Jid.ofLocalAndDomain("testMuc", "example.org"),
+ Conversation.MODE_MULTI
+ );
+ conversation.getMucOptions();
+
+ remainingReads = new CountDownLatch(fieldReadCount);
+ resetDone = new CountDownLatch(1);
+
+ final var result = new AtomicReference<MucOptions>();
+ final var error = new AtomicReference<Throwable>();
+
+ Thread reader = new Thread(() -> {
+ try {
+ result.set(conversation.getMucOptions());
+ } catch (Throwable t) {
+ error.set(t);
+ }
+ });
+
+ Thread resetter = new Thread(() -> {
+ try {
+ remainingReads.await();
+ conversation.resetMucOptions();
+ resetDone.countDown();
+ } catch (Throwable t) {
+ error.set(t);
+ }
+ });
+
+ reader.start();
+ resetter.start();
+
+ reader.join(10_000);
+ resetter.join(10_000);
+
+ remainingReads = null;
+ resetDone = null;
+
+ if (error.get() != null) throw error.get();
+
+ Assert.assertNotNull(
+ "getMucOptions() returned null"
+ + " — the field must not be re-read after the null check",
+ result.get()
+ );
+ }
+}