IllegalStateException: Fragment already added: PrivacyBannerFragment{a553dee} (7aaca395-a9ff-4b3b-8b2e-2109e8ff6b7f tag=...

[sentry-io]

Sentry Issue: JETPACK-ANDROID-9BC

IllegalStateException: Fragment already added: PrivacyBannerFragment{a553dee} (7aaca395-a9ff-4b3b-8b2e-2109e8ff6b7f tag=PRIVACY_BANNER_FRAGMENT)
    at androidx.fragment.app.FragmentStore.addFragment(FragmentStore.java:92)
    at androidx.fragment.app.FragmentManager.addFragment(FragmentManager.java:1483)
    at androidx.fragment.app.BackStackRecord.executeOps(BackStackRecord.java:387)
    at androidx.fragment.app.FragmentManager.executeOps(FragmentManager.java:1967)
    at androidx.fragment.app.FragmentManager.executeOpsTogether(FragmentManager.java:1875)
...
(11 additional frame(s) were not displayed)

[develric]

At this point in time numbers of impacted users are on the low side (8 out of 10700 at the current rollout phase; making it 0.07% impacted users). This is on the 22.7 release, but to be fair, a similar issue (on the DomainSuggestionsFragment fragment) already exists since 22.4 release at least (you can use this query in the Discover section in Sentry "androidx.fragment.app.FragmentStore in addFragment" error.type:IllegalStateException).

I'm giving a priority of Medium for now, but worth following along with the rollout progression.

Hey @mkevins 👋 , since you worked on this PR that introduced the mentioned fragment, wondering if you could have a look at it 🙇

PS: from a super quick check (so don't quote me and feel free to ignore 😄 ), I wonder if it could be related to the observed live data emitting multiple times; in that case we could add an isAdded() check on the fragment but maybe there is more to understand there 🤔

[mkevins]

Hey @mkevins 👋 , since you worked on this PR that introduced the mentioned fragment, wondering if you could have a look at it 🙇

Thanks for the ping Riccardo! I've taken a quick look, and I agree this could be related with the fragment for the privacy dialog.

PS: from a super quick check (so don't quote me and feel free to ignore 😄 ), I wonder if it could be related to the observed live data emitting multiple times; in that case we could add an isAdded() check on the fragment but maybe there is more to understand there 🤔

I'm not sure why this is added twice 🤔 . The underlying MutableLiveData is a SingleLiveEvent, which is supposed to swallow extraneous emissions from orientation changes, etc., but it seems that this expectation is not being completely met (or maybe there is some other reason that the emission is repeated). Either way, I think it would be safe to add the isAdded() check to condition showing the dialog (when it is already shown). I.e. something like this:

diff --git a/WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java b/WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java
index accc2e1a2e..0ee94a8062 100644
--- a/WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java
+++ b/WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java
@@ -1885,7 +1885,9 @@ public class WPMainActivity extends LocaleAwareActivity implements
         if (privacyBannerFragment == null) {
             privacyBannerFragment = new PrivacyBannerFragment();
         }
-        privacyBannerFragment.show(getSupportFragmentManager(), PrivacyBannerFragment.TAG);
+        if (!privacyBannerFragment.isAdded()) {
+            privacyBannerFragment.show(getSupportFragmentManager(), PrivacyBannerFragment.TAG);
+        }
     }
 
     private void showPrivacySettingsScreen(@Nullable Boolean requestedAnalyticsValue) {

I have not attempted reproducing this yet, but I'll assign myself to this issue and tackle this with a PR soon.

[develric]

Thanks for the follow up and analysis @mkevins 🙇 ; I agree with the tentative placement of the isAdded check logic 👍 . I also noticed the live data being a SingleLiveEvent and could not find a reason at hand for a potential re-emission (nor could I reproduce the crash as well tbh 🤔 ).

Also thanks for planning on taking care of it, happy to help testing/reviewing when you come to it 🙇

For the sake of sharing, the numbers seem to be stable for now (at the current rollout 16/20054 ~ 0.07% impacted users); even so, and since we have a few occurrences of a pretty similar crash with DomainSuggestionsFragment from 22.4 release (not sure if there can be other cases), it would be interesting for us to check if we are missing any underline mechanism around the Live data emission that could bite us in future scenarios I guess 🤔.

[mkevins]

I've added a PR for this: #18786

[mkevins]

Closing as resolved by: #18786.