From c84d0805ec7d149c7f8fc7b7ee5a322ba685b976 Mon Sep 17 00:00:00 2001 From: Jonathan Almeida Date: Sat, 3 Dec 2022 23:31:16 -0500 Subject: [PATCH] Bug 1799996 - Speculative fix checks CFRPopup.anchor is attached to window We're seeing crashes in the Fenix copy of the `CFRPopup` that are happening when our activity has already been destroyed, and then we are trying to add a view to it with the WindowManager. Our guess from looking at the traces are that the `BrowserToolbarCFRPresenter` is receiving the `collect call on the main thread, but further down the event loop. As well as the Runnable in `CFRPopup#show` that is posting to the end event loop - both of these calls are putting us in a state where the activity we want has already been destroyed before we get to the moment where we want to use it. This is a crash that is difficult to synthesize or write a test for. As a result, our fix is rather speculative but still straight-forward and safe to uplift, should it work. There are three remaining follow-ups to do when fix is confirmed: 1. There is a CFRPopup that was upstreamed which is based on the Fenix implementation. We should upstream this patch to that component as well. 2. We should remove this copy of the CFRPopup and use the upstreamed version to avoid confusion between the two copies. 3. With this change, we are now gracefully failing when our users get into this state, however our telemtry and onboarding flows are unaware that the CFR was never shown. We need to to correct telemetry for `TrackingProtection.tcpCfrShown` to stop incorrectly reporting false positives and reset our `shouldShowTotalCookieProtectionCFR` pref to false so that we can try again at the appropriate time. Co-authored-by: Christian Sadilek (cherry picked from commit 7a8b0a91c0657340c6a5d191099c22644c49c80a) --- .../java/org/mozilla/fenix/compose/cfr/CFRPopup.kt | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/app/src/main/java/org/mozilla/fenix/compose/cfr/CFRPopup.kt b/app/src/main/java/org/mozilla/fenix/compose/cfr/CFRPopup.kt index a2ed1e048..1c1951f02 100644 --- a/app/src/main/java/org/mozilla/fenix/compose/cfr/CFRPopup.kt +++ b/app/src/main/java/org/mozilla/fenix/compose/cfr/CFRPopup.kt @@ -72,6 +72,19 @@ class CFRPopup( */ fun show() { anchor.post { + // When we're in this Runnable, the 'show' method might have been called right before + // the activity is no longer attached to the WindowManager. When we get to calling + // the CFRPopupFullscreenLayout#show method below, we are now trying to attach the View + // with the WindowManager that has an unusable Activity. + // + // To protect against this, within this same Runnable, we check if the anchor view is + // safe to use before continuing. + // + // See: https://bugzilla.mozilla.org/show_bug.cgi?id=1799996 + if (anchor.context == null || !anchor.isAttachedToWindow) { + return@post + } + CFRPopupFullscreenLayout(text, anchor, properties, onDismiss, action).apply { this.show() popup = WeakReference(this)