Closed Bug 1550291 Opened 5 months ago Closed 4 months ago

Fennec crash reporter triggers Android Q warning about background activity starts

Categories

(Firefox for Android :: General, defect, P1)

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 69
Tracking Status
geckoview66 --- wontfix
firefox-esr60 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 --- verified
firefox69 --- verified

People

(Reporter: kbrosnan, Assigned: petru)

References

(Blocks 1 open bug)

Details

(Whiteboard: [bcs:p1])

Attachments

(3 files)

When GeckoView or Fennec crashes on Android Q a toast notification appears warning of a background activity start. Looks like we might need to use the notification shade to preserve the crash reporter functions. https://developer.android.com/preview/privacy/background-activity-starts

No longer depends on: 1535168

Here's the Google bug about this new background activity restriction:
https://issuetracker.google.com/issues/128511873

Many "me too" comments and duplicate bugs, but no signal from Google that they might back off this new restriction in Android Q's final release.

Whiteboard: [geckoview:fenix:m6] → [geckoview:fenix:m7]

Chris, would the GV team help determine the right approach here? Stefan mentioned they are starting to talk about it re: Fenix too.

Please let me know how the Android team can help support this.

Depends on: 1529082

(In reply to Devin Reams (dreams) from comment #2)

Chris, would the GV team help determine the right approach here? Stefan mentioned they are starting to talk about it re: Fenix too.

Sure. I'll send this bug back to GV triage so the GV team can discuss next steps.

No longer depends on: 1529082
Priority: P1 → --
Whiteboard: [geckoview:fenix:m7] → [geckoview:fenix]

Devin, to be compatible with the upcoming Android Q release (expected in ~Q3), Fennec's crash reporter needs to start a foreground service instead of an Activity.

Fenix/A-C will need to do the same, but it has different code. I filed a new bug for Fenix/A-C: https://github.com/mozilla-mobile/android-components/issues/3097

Flags: needinfo?(dreams)
Priority: -- → P1
Product: GeckoView → Firefox for Android
Summary: Background activity start message when GeckoView/Fennec crashes on Android Q → Fennec crash reporter triggers Android Q warning about background activity starts
Whiteboard: [geckoview:fenix] → [bcs:p1]

Thanks. We'll go with that.

Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Flags: needinfo?(dreams)

Thanks! btw, Sebastian had some comments (in the Fenix GitHub issue) about Android's startForegroundService API on Android API level 26+. I don't know if this is relevant for Fennec, too.

https://github.com/mozilla-mobile/android-components/issues/3097#issuecomment-495349717

Android Q doesn't allow starting Activities from background so we'll use a
foreground notification from where users could start CrashReporterActivity.

Depends on D33028

Google released Q beta 4 with the finalized API changes and the background execution change is still part of Q. https://android-developers.googleblog.com/2019/06/android-q-beta-4-and-final-apis.html

But according to https://issuetracker.google.com/issues/128511873#comment81, activities can be launched from the background if you hold the SYSTEM_ALERT_WINDOW permission, which we already require for the tab queue in any case. Though we still have to handle the case where we haven't (yet) granted that permission by the user, too...

Attachment #9068425 - Attachment description: Bug 1550291 - Android Q: Post a notification for submitting a crash report; r?snorp → Bug 1550291 - Android Q: Use the overlay permission or a foreground notification to start the crash handler; r?VladBaicu
Keywords: checkin-needed

Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7f44087b9a5
Add new "Crash handler" notification channel; r=JanH
https://hg.mozilla.org/integration/autoland/rev/53931ee8bb19
Android Q: Use the overlay permission or a foreground notification to start the crash handler; r=VladBaicu

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
Keywords: checkin-needed
Flags: qe-verify+

We're planning to spin a Fennec 68b10 build on Thursday, so it would be great if we could get an uplift request in time for that.

Flags: needinfo?(petru.lingurar)

Prerequisites for testing this:

  • latest Android Q installed (>= beta 4)
  • force a crash using about:crashparent

There are two scenarios with slightly different behaviors that needs to be tested:

  1. The crash reporter starts right away
  • Display over other apps permission is given to our app.
    User would easiest do this when activating Tab queue.
  1. The crash reporter can be started from a notification posted upon a crash
  • If Tab queue is disabled (that Display over other apps special permission is not allowed for us) a notification would be posted.
  • Tapping the notification would open the crash reporter.
  • Tapping the Ignore button will dismiss the notification, users loosing so the ability to submit a crash report for the recent crash.

Once started, the crash reporter should offer the same experience as previously, nothing should be changed.

Tested with Google Pixel (Android Q- latest) on 69.0a1 from Treeherder (CI of mozilla-central) and here are my findings following the steps provided by Petru:

  • everything worked as expected: the crash reporter starts right away if "display over other apps" is on
  • a notification is displayed if "display over other apps" is off.

Found this scenario where the tabs aren't restored after the crash:

  1. Go to Settings -> Advanced -> Restore tabs -> set to "don't restore after quitting Nightly".
  2. Force a crash using about:crashparent.
  3. Tap on "Ignore" from the notification.
  4. Open Fennec.

Results: the tabs aren't restored after the crash due to the fact that no crash report was submitted.

Note: on older Android versions since we don't have notification we can't test this but the behavior is to restore the tabs after a crash with "Restore tabs - don't restore after quitting Nightly" on.
Should I file a new bug? Or the issue will be fixed here? @Petru

Thanks Sorina!
Will investigate this issue in this ticket.

This is the bit that let's GeckoApp know we've crashed the next time it starts:
https://dxr.mozilla.org/mozilla-central/rev/43b2d57b25cc16d0571e6bef6a414abe24457154/mobile/android/base/java/org/mozilla/gecko/CrashReporterActivity.java#207-213

Maybe moving this to the point where we decide to either directly launch the Crash Reporter UI, respectively alternatively only show the notification is sufficient?

Default app behavior for when a crash occurs involves starting an Activity that
lets users submit a crash report and maybe provide additional info.
In the event of a crash we want to restore for the user all previous tabs, this
Activity being the component that saves the CRASHED status that will trigger
the restore process.

On Android Q, when a crash notification is posted but ignored by the user, upon
starting the app would not know it crashed and it would not try restoring tabs.
Executing the "save CRASHED status" logic whenever a crash occurs, irrespective
of if we'll show the Activity or just post a notification ensures users will
always have their previous tabs restored.

I'm reopening this bug because Petru is landing some follow-up fixes.

68=affected because we'll want to uplift all these Android Q fixes to Fennec 68 Beta.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

I've added a new patch. Please also land D34826.

Keywords: checkin-needed

Comment on attachment 9068424 [details]
Bug 1550291 - Add new "Crash handler" notification channel; r?snorp

Beta/Release Uplift Approval Request

  • User impact if declined: Crash Reporter is not working on Android Q devices.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: The steps from comment #14 and comment #15.
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): We change how the crash reporter starts and saves the "CRASHED" status so that the app would know to restore tabs after a crash.
  • String changes made/needed: crash_handler_notifications_channel,crash_notification_title, crash_notification_message, crash_notification_negative_button_text
Flags: needinfo?(petru.lingurar)
Attachment #9068424 - Flags: approval-mozilla-beta?
Attachment #9068425 - Flags: approval-mozilla-beta?
Attachment #9071798 - Flags: approval-mozilla-beta?

Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35ee61053c39
Ensure we save CRASHED status to be able to restore previous tabs; r=AndreiLazar

Keywords: checkin-needed

Comment on attachment 9068424 [details]
Bug 1550291 - Add new "Crash handler" notification channel; r?snorp

Fixes crash reporting on Android Q devices. Let's get this uplifted now so it's in tomorrow's Nightly builds for QA to verify.

Flags: needinfo?(sorina.florean)
Attachment #9068424 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9068425 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9071798 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: REOPENED → RESOLVED
Closed: 4 months ago4 months ago
Resolution: --- → FIXED
QA Whiteboard: [qa-triaged]

Hi!
I tested this on Beta Nightly 68.0a1 (2019-06-13), Nightly 68.0a1 (2019-06-14) with Google Pixel (Android Q) and I could not reproduce the issue. The tabs are restored when Restore tabs is set to "Don't restore after quitting Nightly" and the "Ignore" button is selected from the pop-up.
This is not fixed on Beta 68.0b10 and I will leave the qe-verify flag until we verify this on the next Beta build.
Due to my findings I will mark this issue as fixed on Firefox 69.
Thanks!

Flags: needinfo?(sorina.florean)

Thanks, Eliza!

I'll set the firefox68 status flag to affected as a reminder that we still need to uplift the fix for restoring tabs (comment 25).

Hi!
Verified as fixed on Beta 68.0b11 with Google Pixel (Android Q) and I could not reproduce the issue. The tabs are restored when Restore tabs is set to "Don't restore after quitting Nightly" and the "Ignore" button is selected from the pop-up.
Due to my findings I will mark this issue as fixed on Firefox 68.
Thanks!

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Regressions: 1561245
Regressions: 1562831
You need to log in before you can comment on or make changes to this bug.