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

Android crash reports #9665

Merged
merged 6 commits into from
Aug 31, 2021
Merged

Android crash reports #9665

merged 6 commits into from
Aug 31, 2021

Conversation

AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Aug 5, 2021

Resolves brave/brave-browser#17563

This PR does introduces following changes:

  1. Creates additional option in Brave shields & privacy named Send crash reports automatically;
  2. Applies crash reporting settings from welcome screen message of Help improve Brave by sending crash reports and completely anonymized, private product analytics;
  3. Makes the crash report be sent to our crash reporting endpoint, https://cr.brave.com;
  4. Modifies the build script to generate breakpad symbols zip archive for Android libs ready for backtrace.io;

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one - requested review at https://github.com/brave/security/issues/554
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Test plan for developers:

  1. Build arm64 Mono Release apk:
    npm run create_dist -- Release --target_os=android --target_arch=arm64 --target_android_base=mono --target_android_output_format=apk;
  2. Upload symbols brave-browser/src/out/android_Release_arm64/dist/brave-v1.30.23-android-mono-arm64-symbols.zip to backtrace.io https://brave.sp.backtrace.io/p/brave/settings/symbol/upload
  3. Install arm64 Mono APK on the device
  4. Enable crash reporting in Settings => Brave shields & privacy => Send crash reports automatically;
  5. Make a crash by open brave://inducebrowsercrashforrealz/;
  6. Re-launch Brave;
  7. Open page brave://crashes and ensure there is a crash report with status Uploaded;
  8. Open https://brave.sp.backtrace.io/p/brave/explore?time=hour&filters=(plat%3DAndroid)&aggregations=((callstack%2Chead))& or use your search filters;
  9. Ensure you can see your crash and the symbolized stack trace.

Test plan for QA for the case when the symbols will be uploaded by CI

  1. Enable crash reporting in Settings => Brave shields & privacy => Send crash reports automatically;
  2. Make a crash by open brave://inducebrowsercrashforrealz/;
  3. Re-launch Brave;
  4. Open page brave://crashes and ensure there is a crash report with status Uploaded;
  5. Open https://brave.sp.backtrace.io/p/brave/explore?time=hour&filters=(plat%3DAndroid)&aggregations=((callstack%2Chead))&
  6. Ensure you can see the symbolized stack trace.

@AlexeyBarabash AlexeyBarabash self-assigned this Aug 5, 2021
@AlexeyBarabash AlexeyBarabash added the CI/skip Do not run CI builds (except noplatform) label Aug 5, 2021
@AlexeyBarabash AlexeyBarabash force-pushed the android_crash_reports branch 5 times, most recently from 1bc7b3c to e6babb2 Compare August 19, 2021 10:57
@AlexeyBarabash
Copy link
Contributor Author

Screenshot of welcome page which used to apply settings for crash reporting:

Screenshot_20210818-221804

@AlexeyBarabash
Copy link
Contributor Author

Screenshot of new setting in `Brave shields & privacy` settings:

Screenshot_20210819-182007

@AlexeyBarabash AlexeyBarabash added enhancement and removed CI/skip Do not run CI builds (except noplatform) labels Aug 19, 2021
@AlexeyBarabash AlexeyBarabash marked this pull request as ready for review August 19, 2021 15:33
@AlexeyBarabash AlexeyBarabash requested review from samartnik and a team as code owners August 19, 2021 15:33
@AlexeyBarabash
Copy link
Contributor Author

Added @jamesmudgett just to review the changes on screenshot at #9665 (comment)

public class MinidumpUploader {
/* package */
- static final String CRASH_URL_STRING = "https://clients2.google.com/cr/report";
+ static final String CRASH_URL_STRING = "https://cr.brave.com";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to make it with bytecode asm, but it didn't work.
My failed attempt is here d0a3909 .
If there is a way how to make that without patch, I will be happy to re-do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does your attempt generate build errors?
I suppose we can add changeFieldOwner similar to changeMethodOwner, maybe as a follow up as it is not related to this task directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, my best attempt did not generate a build error, just the actual url where crash dump was sent was the one from Google, actual url substitution didn't happen. And besides all I had to add two more new patches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is a follow up issue with a brief description of the situation brave/brave-browser#17707
.

@AlexeyBarabash
Copy link
Contributor Author

Screenshot of brave://crashes page with uploaded dumps list:

Screenshot_20210819-185016

@anthonypkeane
Copy link

Screenshot of new setting in Brave shields & privacy settings:
@AlexeyBarabash

Header: Welcome to Brave
Body: Help improve Brave by automatically sending crash reports
Setting: Send crash reports and privacy-preserving product analytics
Button: Continue

cc @rmcfadden3

@anthonypkeane
Copy link

Screenshot of new setting in Brave shields & privacy settings:
Screenshot_20210819-182007

Header 1: Allow privacy-preserving product analytics (P3A)
Body 1: Anonymized P3A info helps Brave estimate overall usage, and ensure we’re improving popular features.
Header 2: Automatically send crash reports
Body 2: Crash reports help Brave improve product stability for all users.

cc @AlexeyBarabash & @rmcfadden3

Send crash reports automatically
</message>
<message name="IDS_SEND_CRASH_REPORTS_SUMMARY" desc="Summary for checkbox of send crash reports automatically.">
Crash reports helps Brave to improve stability of product.
Copy link
Member

Choose a reason for hiding this comment

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

cc @rmcfadden3 for wording suggestions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated words in
a635d47

Now it looks:

Screenshot_20210825-202423

@AlexeyBarabash
Copy link
Contributor Author

@samartnik , I have updated testcase for privacy settings at f839ab4 as we discussed in DM

@AlexeyBarabash AlexeyBarabash added the CI/skip Do not run CI builds (except noplatform) label Aug 25, 2021
@AlexeyBarabash
Copy link
Contributor Author

Will skip CI for now, because more changes / commits are required.

@AlexeyBarabash
Copy link
Contributor Author

AlexeyBarabash commented Aug 26, 2021

Here are the examples of symbol file names as per build config:

Package name Symbols file
Bravearm.apk brave-v1.30.33-android-classic-arm-symbols.zip
BraveMonoarm.apk brave-v1.30.33-android-mono-arm-symbols.zip
BraveMonoarm64.apk brave-v1.30.33-android-mono-arm64-symbols.zip
BraveMonox64.apk brave-v1.30.33-android-mono-x64-symbols.zip
BraveMonox86.apk brave-v1.30.33-android-mono-x86-symbols.zip
Bravex86.apk brave-v1.30.33-android-classic-x86-symbols.zip

@AlexeyBarabash
Copy link
Contributor Author

As per security review the decision for now is:

  1. Modify the welcome page
    Help improve Brave by sending crash reports and completely anonymized, private product analytics.
    =>
    Help improve Brave by sending completely anonymized, private product analytics.
    remove the mention of analytics, as right now in master this checkbox doesn't affect on the actual crash reporting

  2. In the Privacy preferences make option Automatically send crash reports be false by default.

Copy link
Contributor

@samartnik samartnik left a comment

Choose a reason for hiding this comment

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

lgtm

@AlexeyBarabash AlexeyBarabash removed the CI/skip Do not run CI builds (except noplatform) label Aug 27, 2021
@AlexeyBarabash
Copy link
Contributor Author

Rebased to the latest master and squashed some commits

@AlexeyBarabash
Copy link
Contributor Author

@anthonypkeane
Here is updated screens after privacy review comments:

Screenshot_20210827-142551

Screenshot_20210827-174710

@AlexeyBarabash
Copy link
Contributor Author

CI found pylint errors

@AlexeyBarabash
Copy link
Contributor Author

Fixed pylint errors

Copy link
Contributor

@mariospr mariospr left a comment

Choose a reason for hiding this comment

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

LGTM for chromium_src (nice removal!), and possibly for the .patch file, but I'd defer to @samartnik for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants