Skip to content

CCCT-1867 Connect Message Fragment Crash #3395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
conroy-ricketts merged 1 commit into from
Nov 4, 2025
Merged

CCCT-1867 Connect Message Fragment Crash #3395

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.commcare.connect.database.ConnectMessagingDatabaseHelper;
import org.commcare.dalvik.R;
import org.commcare.dalvik.databinding.FragmentConnectMessageBinding;

import org.commcare.google.services.analytics.FirebaseAnalyticsUtil;
import org.commcare.utils.FirebaseMessagingUtil;

Expand Down Expand Up @@ -109,7 +108,14 @@ private void fetchMessagesFromNetwork() {
if (success) {
refreshUi();
} else {
Toast.makeText(requireContext(), getString(R.string.connect_messaging_retrieve_messages_fail), Toast.LENGTH_SHORT).show();
Context context = getContext();
Copy link
Contributor

@Jignesh-dimagi Jignesh-dimagi Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will actually not solve the real problem but suppress it.

I think issue is here, it has delay of 30 minutes before calling API, which is causing this issue.

Copy link
Contributor Author

@conroy-ricketts conroy-ricketts Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a real problem to suppress though is what I mean.

The API is being called every 30 seconds while the user is on the Connect Message Fragment screen. The current crash happens when the API is called, and then the user moves to a different screen before the work calling the API finishes, and that's why the context would be null.

I feel like we should not want to update the UI if the current screen the user is looking at is irrelevant right?

Looking at this code, we are already following this same philosophy in the function refreshUi() in this class:

        Context context = getContext();
        if (context != null) {
            List<ConnectMessagingMessageRecord> messages = ConnectMessagingDatabaseHelper.getMessagingMessagesForChannel(context, channelId);

            List<ConnectMessageChatData> chats = new ArrayList<>();
            for (ConnectMessagingMessageRecord message : messages) {

                chats.add(fromMessage(message));

                if (!message.getUserViewed()) {
                    message.setUserViewed(true);
                    ConnectMessagingDatabaseHelper.storeMessagingMessage(context, message);
                }
            }

            adapter.updateData(chats);
            scrollToLatestMessage();

        }

Checking if the context is not null makes more sense to me because why would we want to update the UI if the fragment is detached from its activity?

I'm open for discussion though

Copy link
Contributor

@Jignesh-dimagi Jignesh-dimagi Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@conroy-ricketts I feel that 30 seconds is too high, may be some one wanted to put 3 seconds but added extra zero there.
If user is not on the screen, I think it should not call the API only. Can we cancel the handler in onDestroyView?

Copy link
Contributor Author

@conroy-ricketts conroy-ricketts Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something, but the size of the interval does not cause the crash, and cancelling the handler in onDestroyView will not solve the issue. The crash happens because the coroutine in PushNotificationApiHelper's retrieveLatestPushNotificationsWithCallback() is launched while the user is on the Connect Message Fragment, but finishes and calls a listener when the user is no longer on the fragment:

object PushNotificationApiHelper {

    fun retrieveLatestPushNotificationsWithCallback(
        context: Context,
        listener: ConnectActivityCompleteListener
    ) {
        CoroutineScope(Dispatchers.IO).launch {
            retrieveLatestPushNotifications(context).onSuccess {
                withContext(Dispatchers.Main) { //  switching to main to touch views
                    listener.connectActivityComplete(true)
                }
            }.onFailure {
                withContext(Dispatchers.Main) { //  switching to main to touch views
                    listener.connectActivityComplete(false)
                }
            }
        }
    }

This coroutine runs very fast normally.

For example, the crash will not happen if you wait a few seconds before hitting the back button on the Connect Message Fragment screen. The crash only happens when you hit the back button while the coroutine is doing its work

Copy link
Contributor

@Jignesh-dimagi Jignesh-dimagi Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that this line is calling the Runnable again after 30 seconds and as user not on this fragment, causing issues.

Copy link
Contributor Author

@conroy-ricketts conroy-ricketts Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I noticed too is that we are already cancelling the handler here

Copy link
Contributor

@OrangeAndGreen OrangeAndGreen Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 30 second refresh rate was chosen intentionally as a way to have messages refresh somewhat frequently without constantly hitting the network. It was originally 60 seconds but we cut it in half earlier this year.

Also, it looks like the handler is already cleaned up during onPause so the call stops happening when the user leaves the page.

All in all, I think Conroy's fix is what we need here since the call can start while the user is on the page but take a while to complete such that the user may have navigated away by the time the call completes.

Copy link
Contributor

@Jignesh-dimagi Jignesh-dimagi Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it looks like Conroy's fix is what we need. This is strange as we need to handle manually the context. We need ViewModel scope here soon.

I was also just trying to understand why app is not crashing whenever it has valid response but only when it fails. Answer is here.

Copy link
Contributor

@OrangeAndGreen OrangeAndGreen Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! I was worried about the success case and verified the same. Explains why all the crash logs were for failed calls.

Copy link
Contributor

@shubham1g5 shubham1g5 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am good with the solution here until we correct lifecyle management of network calls from UI. The alternative here would be to bind the callback to fragment lifecyle so that it never gets called if fragment is no longer alive. Although that might require a lot more changes (like doing this call from a view model scoped to fragment lifecycle). I do want to call out the downside of current solution that it will not hard crash in cases when context is null here due to some bad code (for example calling fetchMessagesFromNetwork from onDestroy, It will never pass our code reviews though :D).

The 30 second refresh rate was chosen intentionally as a way to have messages refresh somewhat frequently without constantly hitting the network. It was originally 60 seconds but we cut it in half earlier this year.

Is there context on that decision somewhere ? 30 seconds does still seem high to me.

if (context != null) {
Toast.makeText(
context,
getString(R.string.connect_messaging_retrieve_messages_fail),
Toast.LENGTH_SHORT
).show();
}
}
});
}
Expand Down