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: wait for logout to be completed when handling user events [WPB-9689] [WPB-10310] #2914

Merged

Conversation

saleniuk
Copy link
Contributor

@saleniuk saleniuk commented Jul 29, 2024

BugWPB-9689 [Android] App crashing and stuck on splash screen when deleting device for second account from another client


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

When the app is in the background and the current client is removed from other client, then when launching the app there's a crash.

Causes (Optional)

ProteusPreKeyRefiller always launches when sync completes (when incremental sync state is changed to Live) and it
requires core crypto to generate new pre keys, but when the app received User.ClientRemove event and started handling it as an "offline event" (before sync is Live) then core crypto may be removed before or during executing the refiller.
Generally I noticed three possible outcomes:

  • fatal signal error (generating new prekeys already started and core crypto got removed during that)
  • ProteusException: Job was cancelled (probably userscope got removed and cancelled all ongoing coroutines)
  • ProteusException: CoreCrypto object has already been destroyed (core crypto got removed before some prekeys started being generated)
    Just the first one ends up crashing the app, ProteusExceptions are handled quietly and don't crash the app, so depending on what happens first, it can crash or handle it without a crash.

Solutions

We shouldn't start refilling pre keys if the core crypto / client is already removed.
Even more - we shouldn't execute any action after sync or handle any other event after receiving User.ClientRemove or User.UserDelete event.
The solution is easy - add waitUntilCompletes = true when when executing logout during handling User.ClientRemove or User.UserDelete event. This way, when the app starts the logout, it should actually wait for the logout to be completed before any other event is handled, so it will close the user session and related coroutine scope and prevent the app from handling subsequent events and doing any actions after the sync is completed.

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

How to Test

  • login on client A
  • login as the same user on client B
  • put client A into background
  • remove client A on client B
  • open the app on client A by clicking on the app icon
  • user should be logged out on client A without any crash

PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@echoes-hq echoes-hq bot added the echoes: unplanned Any work item that isn’t part of the product or technical roadmap. label Jul 29, 2024
Copy link
Contributor

github-actions bot commented Jul 29, 2024

Test Results

2 966 tests   2 949 ✔️  1h 2m 53s ⏱️
       5 suites       17 💤
       5 files           0

Results for commit 210b9b1.

♻️ This comment has been updated with latest results.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Jul 29, 2024

Datadog Report

Branch report: fix/wait-for-logout-when-handling-user-events
Commit report: 800b4e0
Test service: kalium-jvm

✅ 0 Failed, 3029 Passed, 105 Skipped, 14.19s Total Time

@saleniuk saleniuk requested review from a team, typfel, yamilmedina, MohamadJaara, mchenani and Garzas and removed request for a team July 30, 2024 07:59
@MohamadJaara MohamadJaara added this pull request to the merge queue Jul 30, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 30, 2024
@saleniuk saleniuk added this pull request to the merge queue Jul 30, 2024
@MohamadJaara MohamadJaara removed this pull request from the merge queue due to a manual request Jul 30, 2024
@MohamadJaara MohamadJaara merged commit 8a02f96 into release/candidate Jul 30, 2024
10 checks passed
@MohamadJaara MohamadJaara deleted the fix/wait-for-logout-when-handling-user-events branch July 30, 2024 12:03
Copy link

github-actions bot pushed a commit that referenced this pull request Jul 30, 2024
…689] [WPB-10310] (#2914)

Co-authored-by: Vitor Hugo Schwaab <[email protected]>
Co-authored-by: Mohamad Jaara <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Jul 30, 2024
…689] [WPB-10310] 🍒 (#2919)

* fix: wait for logout to be completed when handling user events [WPB-9689] [WPB-10310] (#2914)

Co-authored-by: Vitor Hugo Schwaab <[email protected]>
Co-authored-by: Mohamad Jaara <[email protected]>

* trigger build

---------

Co-authored-by: Michał Saleniuk <[email protected]>
Co-authored-by: Vitor Hugo Schwaab <[email protected]>
Co-authored-by: Mohamad Jaara <[email protected]>
Co-authored-by: Michał Saleniuk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: unplanned Any work item that isn’t part of the product or technical roadmap. 👕 size: XS type: bug / fix 🐞
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants