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

Window manager BadTokenException / WindowLeaked #2208

Merged
merged 4 commits into from
Nov 4, 2024

Conversation

jinliu9508
Copy link
Contributor

@jinliu9508 jinliu9508 commented Oct 30, 2024

Description

One Line Summary

Fix the BadTokenException and WindowLeaked exception caused by showing a dialog on a finishing or destroyed activity.

Details

Motivation

The exception exists since User Model and now becomes the most occurring exception according to the data from Google Play SDK Console. This exception is relatively less impactful to user experience as it normal happens during app closing or backgrounding phrase, but we want to reduce the amount of reported exception as it may potentially affect app's stability rating over the Play Store.

Scope

The call to show a permission dialog has no effect if the activity is finishing, backgrounded, or destroyed, and it will not be retried unless user is explicitly asking to do so. There is no behavior change because the current version would just crash and will not retrying showing the dialog upon app restart.

OPTIONAL - Other

This PR contains 3 commits: the first commit simulates calling a permission dialog on a finished activity; the second commit shows that the fix is effective in removing the exception in such activity; the third commit cleans up the simulation code and leave the fix code change only.
Related issue: #2014

Testing

Manual testing

By checking out the first commit, I am able to reproduce a WindowLeaked exception by running the sample app.
image
After the second commit, the exception is no longer reproducible.
BadTokenException can be caused by showing the dialog in a "finishing" activity but I am unable to simulate the situation other than a "finished" activity. Since this fix checks for both states of the activity, I believe this can fixes both BadTokenException and WindowLeaked exceptions.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@jinliu9508 jinliu9508 force-pushed the windowManager-BadTokenException branch from 7f1523c to df65bae Compare October 31, 2024 21:54
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

While this might be a 1 in million, and may never happen, I think it's possible the activity state to change between when we check and when show() is called., but probably only if we are trying to display from a background thread. The thread note is only a theory, but all Android APIs only do work on the main thread from what I have seen..

Do you have any thoughts on maybe doing a try catching on calling show instead of checking before calling show? We could specifically catch android.view.WindowLeaked, and then do our error handling logic there.

@jinliu9508 jinliu9508 merged commit a7d5dee into main Nov 4, 2024
2 checks passed
@jinliu9508 jinliu9508 deleted the windowManager-BadTokenException branch November 4, 2024 19:09
@jinliu9508 jinliu9508 mentioned this pull request Nov 4, 2024
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