Page MenuHomePhabricator
Differential D4573

Place call to completionHandler outside rescinding callback so that we avoid app being killed by the system
AbandonedPublic
Actions

Authored by marcin on Jul 19 2022, 2:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 10, 12:41 PM
Unknown Object (File)
Fri, Apr 26, 10:38 PM
Unknown Object (File)
Apr 12 2024, 10:15 AM
Unknown Object (File)
Apr 12 2024, 10:15 AM
Unknown Object (File)
Apr 12 2024, 10:15 AM
Unknown Object (File)
Apr 12 2024, 10:15 AM
Unknown Object (File)
Apr 12 2024, 10:15 AM
Unknown Object (File)
Apr 12 2024, 10:15 AM
View All 64 Files
Subscribers
• abosh
• adrian
ashoat

Details

Reviewers
tomek
atul
ashoat
Commits
rCOMM04107397098d: Revert "use dispatch on main queue"
rCOMMb9cf473baef9: Place call to completionHandler outside rescinding callback so that we avoid…
Summary

This diff moves call to completionHandler from rescinding callback in AppDelegate in handleBackgroundNotification. As established before rescinding callback is highly unlikely to ever be called, but failure to call completionHandler will result in app being terminated by the system

Test Plan

Launch the app from XCode and put it into background mode. Open two web clients one of which is the same as mobile one. Send messages and read them on web client several times waiting a couple of minutes between each trial. Ensure that app is still running.

Diff Detail

Repository
rCOMM Comm
Branch
marcin/new-D4576
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Jul 19 2022, 2:49 AM
Herald added subscribers: • abosh, • adrian, ashoat. · View Herald TranscriptJul 19 2022, 2:49 AM
Harbormaster completed remote builds in B10629: Diff 14622.Jul 19 2022, 3:02 AM
marcin requested review of this revision.Jul 19 2022, 3:02 AM
tomek requested changes to this revision.Jul 19 2022, 3:12 AM

This code doesn't work correctly. We need to wait for the loop to finish before calling the completion handler. The code starting with ^ is a block and can return before being completed (just like an async function).

This revision now requires changes to proceed.Jul 19 2022, 3:12 AM
marcin updated this revision to Diff 14635.Jul 19 2022, 5:07 AM

Wait for resdingin loop to complete

Harbormaster completed remote builds in B10636: Diff 14635.Jul 19 2022, 5:20 AM
marcin added a child revision: D4576: use dispatch on main queue.Jul 19 2022, 5:38 AM
marcin added a parent revision: D4572: change thread operations utilities paths in XCode so that they can be debugged.
ashoat added a comment.Jul 19 2022, 6:45 AM

So the idea on this diff is that we want to make sure that we call completionHandler before we return YES, as well as calling completionHandler on the main thread? Whereas the idea of D4576 is that we just need to call completionHandler on the main thread, and the order of return YES vs. calling completionHandler does not matter?

marcin added a comment.Jul 19 2022, 9:21 AM
In D4573#130520, @ashoat wrote:

So the idea on this diff is that we want to make sure that we call completionHandler before we return YES, as well as calling completionHandler on the main thread? Whereas the idea of D4576 is that we just need to call completionHandler on the main thread, and the order of return YES vs. calling completionHandler does not matter?

Yes, exactly. Your understanding is correct.

ashoat requested changes to this revision.Jul 19 2022, 9:43 AM

I don't think it matters when application:didReceiveRemoteNotification:fetchCompletionHandler: returns, so since both approached seemed to work let's go with D4576 for now. We can bring this one back if D4576 doesn't work

This revision now requires changes to proceed.Jul 19 2022, 9:43 AM
marcin updated this revision to Diff 14651.Jul 19 2022, 9:52 AM

Rebase master before landing

This revision was not accepted when it landed; it landed in state Needs Review.Jul 19 2022, 10:02 AM
This revision was landed with ongoing or failed builds.
Closed by commit rCOMMb9cf473baef9: Place call to completionHandler outside rescinding callback so that we avoid… (authored by marcin). · Explain Why
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMb9cf473baef9: Place call to completionHandler outside rescinding callback so that we avoid….
Harbormaster completed remote builds in B10651: Diff 14651.Jul 19 2022, 10:05 AM
marcin added a commit: rCOMM04107397098d: Revert "use dispatch on main queue".Jul 19 2022, 11:18 AM
marcin added a reverting change: rCOMM35fa0b5b9686: Revert "Place call to completionHandler outside rescinding callback so that we….
marcin removed a child revision: D4576: use dispatch on main queue.Jul 19 2022, 11:26 AM
marcin reopened this revision.Jul 19 2022, 11:30 AM
marcin added a parent revision: D4576: use dispatch on main queue.
marcin removed a parent revision: D4572: change thread operations utilities paths in XCode so that they can be debugged.
marcin updated this revision to Diff 14665.Jul 19 2022, 11:32 AM

Reorder

Harbormaster completed remote builds in B10655: Diff 14665.Jul 19 2022, 11:45 AM
ashoat requested changes to this revision.Jul 19 2022, 12:00 PM

Thanks for fixing things up!! Removing from my queue until we can confirm it's necessary (we're first testing just D4576 on its own)

This revision now requires changes to proceed.Jul 19 2022, 12:00 PM
marcin abandoned this revision.Apr 12 2024, 4:41 AM
marcin added a comment.Apr 12 2024, 4:51 AM

This diff was only used to create a release that was supposed to fix a crash: https://linear.app/comm/issue/ENG-1464/ios-build-137-crashes. The solution wasn't correct.

Revision Contents
Changeset List

PathSize
native/
ios/
Comm/
7 lines

Diff 14665

View Options

native/ios/Comm/AppDelegate.mm

Loading...