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

Reverts "Timing: Fixes timer when app get into background (#24649)" #27065

Merged
merged 1 commit into from
Nov 4, 2019

Conversation

radko93
Copy link
Contributor

@radko93 radko93 commented Oct 30, 2019

This reverts commit 3382984

Summary

This is related #26696 - an issue where the app would be closed immediately after going to background on iOS 13.1/13.2 and was investigated by @minhtc #26696 (comment). The commit that is being reverted is apparently causing the app to be closed immediately. This has to be reverted in master separately as the file differs there.

Changelog

[iOS] [Fixed] - Reverts commit causing a crash in the background

Test Plan

Create an empty app on 0.61.x with a timer, put it in the background.

@radko93 radko93 requested a review from shergin as a code owner October 30, 2019 20:46
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 30, 2019
@zhongwuzw
Copy link
Contributor

@radko93 Hey, seems iOS13 killed background apps much more aggressively, because we just use UIApplication background task, is this means we can't use it anymore? If we create a new iOS project(not integrate RN), just use UIApplication background task, is it also crashed?

@facebook facebook deleted a comment from pull-bot Oct 31, 2019
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Oct 31, 2019
@radko93
Copy link
Contributor Author

radko93 commented Oct 31, 2019

@zhongwuzw I think iOS 13.1 put some stricter limits on how this works. https://forums.developer.apple.com/thread/85066:

Failing to end a background task is the #1 cause of background task problems on iOS. This usually involves some easy-to-overlook error in bookkeeping that results in the app beginning a background task and not ending it.

Maybe we don't stop the background task in time, I'm not sure at this time, but we should investigate later on. You can try to experiment with iOS 13.1.

@ex3ndr
Copy link

ex3ndr commented Nov 1, 2019

Woah, starting background task and never stop it? Of course iOS will kill your app otherwise. It was the same logic like forever.

@radko93
Copy link
Contributor Author

radko93 commented Nov 1, 2019

@cpojer please look here and at #27073

@zhongwuzw
Copy link
Contributor

Woah, starting background task and never stop it? Of course iOS will kill your app otherwise. It was the same logic like forever.

@ex3ndr You can check the commit to see wether we stop it :).

@zhongwuzw
Copy link
Contributor

@zhongwuzw I think iOS 13.1 put some stricter limits on how this works. https://forums.developer.apple.com/thread/85066:

Failing to end a background task is the #1 cause of background task problems on iOS. This usually involves some easy-to-overlook error in bookkeeping that results in the app beginning a background task and not ending it.

Maybe we don't stop the background task in time, I'm not sure at this time, but we should investigate later on. You can try to experiment with iOS 13.1.

🤔 I don't have iOS13.0 or 13.1 now :), is this not a crash, right? it terminated by system after seconds, and I think we stop the background task in time, we can have two options to fix this:

  1. Just revert it, disable the background timer feature for all iOS system users.
  2. Provide an option for Timer module, developer can choose wether to enable/disable the background timer on different iOS version.

@radko93 What do you think?

@radko93
Copy link
Contributor Author

radko93 commented Nov 2, 2019

@zhongwuzw we should make the users safe now, then we can bring this back improved.

@grabbou grabbou changed the base branch from 0.61-stable to master November 4, 2019 17:15
@grabbou grabbou changed the base branch from master to 0.61-stable November 4, 2019 17:15
@grabbou
Copy link
Contributor

grabbou commented Nov 4, 2019

FYI let's keep the discussion under #27073 - that's the PR against master and will be easier to manage for other maintainers.

Meanwhile, I think it's fair to revert something that causes a major regression. We can always re-iterate again.

@grabbou grabbou merged commit d7c9173 into facebook:0.61-stable Nov 4, 2019
facebook-github-bot pushed a commit that referenced this pull request Nov 5, 2019
…27073)

Summary:
This PR reverts commit 3382984 that is causing #26696 #26995.
> app would be closed immediately after going to background on iOS 13.1/13.2 and was investigated by minhtc #26696 (comment). The commit that is being reverted is apparently causing the app to be closed immediately. This has to be reverted in master separately as the file differs there.

Similar PR for 0.61. branch #27065

## Changelog

[iOS] [Fixed] - Fix apps crashing on iOS 13.x when running timer in the background
Pull Request resolved: #27073

Test Plan: Try [this](3382984#commitcomment-35745287) snippet on iOS 13.1/13.2, the app should not crash anymore

Differential Revision: D18323679

Pulled By: cpojer

fbshipit-source-id: 3af7036a0e1d3811924e581c649b16e5a4667e83
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants