-
Notifications
You must be signed in to change notification settings - Fork 1.2k
PIR: Fix PIR foreground service types usage #7596
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.
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
base: develop
Are you sure you want to change the base?
Conversation
| android:process=":pir"> | ||
| <property | ||
| android:name="android.app.PROPERTY_SPECIAL_USE_FGS_SUBTYPE" | ||
| android:value="Executing operations for the Personal Information Removal feature" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a question regarding this on https://app.asana.com/1/137249556945/project/1209454604567606/task/1212961144938313?focus=true
pir/pir-impl/src/main/java/com/duckduckgo/pir/impl/scan/PirForegroundScanService.kt
Show resolved
Hide resolved
pir/pir-impl/src/main/java/com/duckduckgo/pir/impl/optout/PirForegroundOptOutService.kt
Show resolved
Hide resolved
| } | ||
|
|
||
| override fun reportManualScanStartFailed() { | ||
| enqueueFire(PIR_FOREGROUND_RUN_START_FAILED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karlenDimla I've had to use enqueueFire here instead of regular fire because we call stopSelf() right after we fire the pixel and for some reason it's not sent. It seems the service gets destroyed and along with it likely other dependencies.
I've confirmed that with enqueueFire the pixel gets fired on next app launch.
|
Privacy Review task: https://app.asana.com/0/69071770703008/1213011073757604 |
| try { | ||
| ServiceCompat.startForeground( | ||
| this, | ||
| 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been a placeholder id since the PoC. Is now the best time to replace this? At least let’s assign it to variable.
karlenDimla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment and also pending privacy triage. Works as expected.
pir/pir-impl/src/main/java/com/duckduckgo/pir/impl/scan/PirForegroundScanService.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Task/Issue URL: https://app.asana.com/1/137249556945/project/488551667048375/task/1212998192770911?focus=true
Description
Declares the missing FGS permissions and provide foreground service type in
startForeground()call.Steps to test this PR
Smoke test PIR initial scanning
UI changes
No UI changes
Note
Aligns PIR foreground services with Android FGS requirements and adds diagnostics.
FOREGROUND_SERVICEandFOREGROUND_SERVICE_SPECIAL_USE; services useforegroundServiceType="specialUse"withPROPERTY_SPECIAL_USE_FGS_SUBTYPEPirForegroundScanService,PirForegroundOptOutService): switch toServiceCompat.startForegroundwithFOREGROUND_SERVICE_TYPE_SPECIAL_USEon SDK >= 34, add try/catch; on failure stop service and, for scans, emitm_dbp_foreground-run_start-failedm_dbp_foreground-run_start-failedandm_dbp_foreground-run_low-memory; plumb throughPirPixel,PirPixelSender(newreportManualScanStartFailed/LowMemory,enqueueFire), and allowlist inPirPixelInterceptor(addsmanparam)PirForegroundScanServiceemits low-memory pixel inonLowMemoryWritten by Cursor Bugbot for commit 8ffc74c. This will update automatically on new commits. Configure here.