Skip to content
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

Attach screenshot #2362

Closed
Tracked by #65
bruno-garcia opened this issue Jul 9, 2022 · 3 comments · Fixed by #2610
Closed
Tracked by #65

Attach screenshot #2362

bruno-garcia opened this issue Jul 9, 2022 · 3 comments · Fixed by #2610

Comments

@bruno-garcia
Copy link
Member

Add option attachScreenshot that when opt in, attached a screenshot on error events.

@marandaneto
Copy link
Contributor

marandaneto commented Jul 25, 2022

It's not just adding the attachScreenshot flag.

On iOS, native hard crashes would work, but the JS errors (even if it crashes the app) won't contain the screenshot, because we call captureEnvelope or storeEnvelope and those functions won't attach the screenshot before sending it over to Sentry.

On Android, We always store the envelope on the disk, and the file observer will pick it up and enrich the event with device context, that's already too late and the view might not be up to date anymore, leading to a false positive.
There are more 2 issues.
The first issue is that the currentActivity is null because the Android SDK is inited after the very first Activity is created.
https://github.com/getsentry/sentry-java/blob/121d465b31546c7d98af7b4b260dc120db902f79/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java#L70
If you put the app in the background and restore to the foreground, it'd work because the currentActivity won't be null anymore.
The second issue is the attachScreenshot flag, if you enable it via the manifest, it works, but if you use the manual init as the RN does, the event processor is created before the user flips the attach screenshot flag and its always false, and it does not install the activity observer, https://github.com/getsentry/sentry-java/blob/121d465b31546c7d98af7b4b260dc120db902f79/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java#L50-L64
I've raised an issue for that.

I feel that getsentry/team-mobile#11 should be addressed first with this issue in mind.
cc @bruno-garcia

@bruno-garcia
Copy link
Member Author

If we're triggering the screenshot from JS through the bridge it's possible the view will change, no? So possibly not exactly what was on the screen at that time? It's fine if that's the case, just good to understand and document it.

I feel that getsentry/team-mobile#11 should be addressed first with this issue in mind.

Not sure this one affects things as much. We can take care of screenshots on each layer: In JS if there's an error, we get a screenshot through the bridge and pass it down to the native layers in the envelope. It doesn't need to live in the JS-land either, we could do all of that in the bridge if that makes things easier.
For the native errors we just pass the flag down so they add screenshots when they capture errors on those layers.

Would that work?

So we need to figure out:

@marandaneto
Copy link
Contributor

If we're triggering the screenshot from JS through the bridge it's possible the view will change, no? So possibly not exactly what was on the screen at that time? It's fine if that's the case, just good to understand and document it.

If we await the calls, technically the view is the same because we are in the calling thread.

The first issue is that the currentActivity is null because the Android SDK is inited after the very first Activity is created.

This is still an issue, we'd need to find a way to set the currentActivity via the RN SDK, maybe the RN plugin should implement its own logic for screenshots, and the EventProcessor just exposes the methods to not duplicate the same logic, similar to Slow/frozen frames.

@marandaneto marandaneto moved this from In Progress to Blocked in Mobile & Cross Platform SDK Aug 18, 2022
@marandaneto marandaneto moved this from Blocked to Needs Discussion in Mobile & Cross Platform SDK Aug 18, 2022
@marandaneto marandaneto moved this from Needs Discussion to Backlog in Mobile & Cross Platform SDK Aug 19, 2022
@zoesyc zoesyc moved this to Research / Discussion 📖 in Mobile Planning Sep 21, 2022
@zoesyc zoesyc moved this from Research / Discussion 📖 to In Progress 📈 in Mobile Planning Nov 4, 2022
@zoesyc zoesyc moved this from In Progress 📈 to Planned ✅ in Mobile Planning Nov 4, 2022
@krystofwoldrich krystofwoldrich moved this from Planned ✅ to In Progress 📈 in Mobile Planning Nov 9, 2022
@krystofwoldrich krystofwoldrich moved this from Backlog to In Progress in Mobile & Cross Platform SDK Nov 21, 2022
Repository owner moved this from In Progress to Done in Mobile & Cross Platform SDK Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Status: In Progress 📈
3 participants