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 stopping OngoingCallService [WPB-2320] [WPB-1836] [WPB-3457] #2084

Merged

Conversation

github-actions[bot]
Copy link
Contributor

Cherry pick from the original PR:


⚠️ Conflicts during cherry-pick:


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?

The PR is big, but 80% of that are tests. 😅

Issues

There are still multiple crashes related to stopping OngoingCallService: ForegroundServiceDidNotStartInTimeException or Context.startForegroundService() did not then call Service.startForeground(), depending on Android version.
Also, we have one service for all logged in users so there's a race condition because one account can close the service even if another one has an ongoing call.

Causes (Optional)

The problem is that if we start service as a foreground one, then it has to call startForeground and show notification, if we stop such service before it does that, then the app crashes.

Solutions

We already tried to call startForeground with some placeholder notification when creating service, before onStartCommand is called, but it's not enough and crashes still happen quite commonly, so to fix that the debounce is added to take 200 milliseconds to check whether starting this service is really necessary - if "stop service" happens within that time then it won't even start and prevent us from the crash. Also, stopping the service, if already started but before it's in the foreground, is done directly inside the service, so even if we call it to stop too soon, then it will still execute onStartCommand, show a placeholder notification for a split second and then safely close the service.
For the issue with multiple accounts, a common SharedFlow is provided which collects data for all accounts and then starts or stops the service if any of them has an ongoing call. The same is implemented for incoming call notification (we also show a single incoming call notification for all accounts). The service is started and then it handles keeping itself alive as long as there is an ongoing call for any of valid accounts.

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

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.

…B-3457] (#2057)

Co-authored-by: Tommaso Piazza <196761+tmspzz@users.noreply.github.com>
@github-actions github-actions bot added the cherry-pick PR is cherry-picking changes from another banch label Aug 11, 2023
@AndroidBob
Copy link
Collaborator

Build 1017 failed.

@github-actions
Copy link
Contributor Author

github-actions bot commented Aug 11, 2023

Test Results

533 tests  +24   532 ✔️ +24   5m 28s ⏱️ -41s
  79 suites +  2       1 💤 ±  0 
  79 files   +  2       0 ±  0 

Results for commit 040fe16. ± Comparison against base commit cbd5ae1.

This pull request removes 1 and adds 25 tests. Note that renamed tests count towards both.
com.wire.android.notification.WireNotificationManagerTest ‑ givenSomeEstablishedCalls_whenAppIsNotVisible_thenOngoingCallServiceRun()
com.wire.android.notification.CallNotificationManagerTest ‑ given an incoming call for one user, then show notification for that call()
com.wire.android.notification.CallNotificationManagerTest ‑ given incoming call, when end call comes instantly after start, then do not even show notification()
com.wire.android.notification.CallNotificationManagerTest ‑ given incoming call, when end call comes some time after start, then first show notification and then hide()
com.wire.android.notification.CallNotificationManagerTest ‑ given incoming calls for two users, then show notification for the first call()
com.wire.android.notification.CallNotificationManagerTest ‑ given incoming calls for two users, when both call ends, then hide notification()
com.wire.android.notification.CallNotificationManagerTest ‑ given incoming calls for two users, when one call ends, then show notification for another call()
com.wire.android.notification.CallNotificationManagerTest ‑ given no incoming calls, then hide notification()
com.wire.android.notification.WireNotificationManagerTest ‑ givenAppInBackground_withInvalidCurrentAccountAndNoOngoingCall_whenObserving_thenStopOngoingCallService()
com.wire.android.notification.WireNotificationManagerTest ‑ givenAppInBackground_withInvalidCurrentAccountAndOngoingCall_whenObserving_thenStopOngoingCallService()
com.wire.android.notification.WireNotificationManagerTest ‑ givenAppInBackground_withTwoValidAccountsAndOngoingCallForCurrentOne_whenCurrentAccountChanges_thenStopOngoingCallService()
…

♻️ This comment has been updated with latest results.

@AndroidBob
Copy link
Collaborator

Build 1026 failed.

@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Merging #2084 (040fe16) into develop (cbd5ae1) will increase coverage by 0.43%.
The diff coverage is 53.54%.

@@              Coverage Diff              @@
##             develop    #2084      +/-   ##
=============================================
+ Coverage      39.14%   39.58%   +0.43%     
- Complexity       920      930      +10     
=============================================
  Files            305      305              
  Lines          11194    11254      +60     
  Branches        1491     1499       +8     
=============================================
+ Hits            4382     4455      +73     
+ Misses          6390     6370      -20     
- Partials         422      429       +7     
Files Changed Coverage Δ
...ndroid/notification/NotificationChannelsManager.kt 1.20% <0.00%> (ø)
...in/com/wire/android/services/OngoingCallService.kt 1.75% <3.03%> (+1.75%) ⬆️
...re/android/notification/WireNotificationManager.kt 79.60% <58.62%> (-0.30%) ⬇️
...re/android/notification/CallNotificationManager.kt 30.39% <67.74%> (+29.15%) ⬆️
...otlin/com/wire/android/services/ServicesManager.kt 73.91% <90.62%> (+73.91%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbd5ae1...040fe16. Read the comment docs.

@github-actions
Copy link
Contributor Author

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 1032 succeeded.

The build produced the following APK's:

@ohassine ohassine added this pull request to the merge queue Aug 11, 2023
Merged via the queue into develop with commit db45542 Aug 11, 2023
13 checks passed
@ohassine ohassine deleted the fix/crash_when_stopping_ongoingcallservice-cherry-pick branch August 11, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick PR is cherry-picking changes from another banch size/XXL
Projects
None yet
4 participants