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

Fix crash when leaving WireGuard Key screen #1701

Merged
merged 4 commits into from
Apr 30, 2020

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Apr 30, 2020

Some crash reports contains an IllegalStateException in the WireGuard Key screen. When checking the backtrace, it looked like the view was being updated after the fragment was detached (i.e., after the user left the screen). This shouldn't have happened because the background jobs should be cancelled when leaving the screen.

However, it turns out there was a bug in the JobTracker. It was cancelling a wrapper "reaper" job instead of cancelling the actual job. This PR fixes that by making sure both jobs are cancelled.

Since the crash happened when calling getResources() in the fragment for retrieving a color, the PR also takes a second mitigation of caching the color value so that the call isn't necessary anymore.

Git checklist:


This change is Reviewable

@jvff jvff requested a review from pinkisemils April 30, 2020 13:37
Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Janito Vaqueiro Ferreira Filho added 4 commits April 30, 2020 15:33
@jvff jvff force-pushed the fix-crash-on-wireguard-key-screen branch from 006dccf to f02ebb5 Compare April 30, 2020 15:34
@jvff jvff merged commit bdae032 into master Apr 30, 2020
@jvff jvff deleted the fix-crash-on-wireguard-key-screen branch April 30, 2020 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants