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.

Sign up for GitHub

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

Jump to bottom

Feature/crash loop detection #80

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Feature/crash loop detection #80

wants to merge 14 commits into from

Conversation

vlussenburg
Copy link
Contributor

Reopening #71

@konraddysput konraddysput added the enhancement New feature or request label Nov 30, 2022
KrzaQ
KrzaQ suggested changes Dec 8, 2022
View reviewed changes
Copy link
Contributor

@KrzaQ KrzaQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the change looks well overall, but some things should be more fleshed out before we release it to the public.

backtrace-library/src/main/cpp/backends/backend.cpp Show resolved Hide resolved
backtrace-library/src/main/cpp/backends/crashpad-backend.cpp Outdated Show resolved Hide resolved
backtrace-library/src/main/cpp/backends/crashpad-backend.cpp Outdated
@@ -120,9 +123,12 @@ bool InitializeCrashpad(jstring url,

// Start crash handler
client = new crashpad::CrashpadClient();
client->EnableCrashLoopDetection();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we want to make this configurable for the end-users.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should - imo we should pass it and don't execute this path if we don't need it. By default we want have features opt-in

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it - it is called from Example's MainActivity now.

backtrace-library/src/main/cpp/backends/crashpad-backend.cpp Show resolved Hide resolved
backtrace-library/src/main/cpp/backends/crashpad-backend.cpp Show resolved Hide resolved
KrzaQ
KrzaQ suggested changes Feb 10, 2023
View reviewed changes
Copy link
Contributor

@KrzaQ KrzaQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to update the crashpad dependency for a fix that has yet to be done. I'm on it, but this must wait.

example-app/src/main/java/backtraceio/backtraceio/MainActivity.java Outdated Show resolved Hide resolved
@konraddysput
Backtrace client api improvements (#89)
a156236 
* Backtrace client api improvements

* Bring back backtrace credentials definition
@KrzaQ
Copy link
Contributor

KrzaQ commented Feb 14, 2023

Please rebase the branch on the latest master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konst-sauce konst-sauce konst-sauce left review comments

@BartoszLitwiniuk BartoszLitwiniuk BartoszLitwiniuk left review comments

@KrzaQ KrzaQ KrzaQ requested changes

@konraddysput konraddysput konraddysput approved these changes

Assignees

@konst-sauce konst-sauce

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vlussenburg @KrzaQ @konraddysput @BartoszLitwiniuk @konst-sauce