Prevent iOS 15 crashes by not polling isLowPowerModeEnabled #613

earltedly · 2021-09-28T14:31:10Z

What and why?

As described in #609, iOS 15 has introduced a race condition around ProcessInfo.isLowPowerModeEnabled:

The conditions under which this can occur are:

Register to observer: NSProcessInfoPowerStateDidChangeNotification
In the observation block, query ProcessInfo.isLowPowerModeEnabled
Later on another piece of code calls ProcessInfo.isLowPowerModeEnabled
Sometimes that will trigger a change in the power state which will be broadcast to observers before returning
The code in (2) is called which re-enters isLowPowerModeEnabled
Thread deadlock over an os_unfair_lock occurs and the app crashes

How?

We introduce a LowPowerModeMonitor which:

Listens to NSProcessInfoPowerStateDidChangeNotification
On any updates it queries ProcessInfo.isLowPowerModeEnabled and caches the return value

MobileDevice retains an instance of this in the local scope and queries it instead of ProcessInfo locally.

This prevents us from constantly polling for new values and thus takes us out of the call stack for re-entry.

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference

Notes

This is a change in the implementation details rather than behaviour, so I've not added any additional tests

buranmert

hey @earltedly 👋
thanks so much for the PR, we are really happy to have your contribution! 🥳

switching from polling to listening to notifications seems to be a step in the right direction, but i guess there is still a problem with isLowPowerModeEnabled property. i left a comment below 👇

Sources/Datadog/Core/System/MobileDevice.swift

Asavarkhul · 2021-09-29T08:47:39Z

Sources/Datadog/Core/System/MobileDevice.swift

@@ -99,3 +100,29 @@ internal class MobileDevice {
        }
    }
 }
+
+private class LowPowerModeMonitor {


Hey guys 👋

Quick question, does this class needs to be open to subclassing? Why not putting it as final?

I'm fairly sure this is implicit for a private class, however always best to be certain so I've added final too 👍

… they change rather than polling for them on every log submission

earltedly · 2021-09-29T14:51:05Z

Thanks for the review 🎉 . I think I've addressed everything, but please let me know if anything else is needed.

buranmert

once again thanks @earltedly 🙏
unfortunately CI failed due to linter errors, i left suggestions to fix them.
once these trivial errors are fixed, CI will probably pass and we can merge the PR 🚀

note: you can see linter output as warnings in Xcode and/or by running ./tools/lint/run_linter.sh in Terminal

Sources/Datadog/Core/System/MobileDevice.swift

yousefhamza · 2021-09-30T12:02:30Z

Sources/Datadog/Core/System/MobileDevice.swift

+                guard let processInfo = notification.object as? ProcessInfo else {
+                    return
+                }
+                self?.publisher.publishAsync(processInfo.isLowPowerModeEnabled)


Hi @earltedly 👋

Can you help me understand how accessing processInfo like this, prevents against calling the deadlock?

I'm confused because the object that posts the notification will be the same object that we query for isLowPowerModeEnabled which should cause the same issue.

hi @yousefhamza 👋

DatadogSDK currently polls processInfo.isLowPowerModeEnabled regularly, once in every 5 seconds. this PR switches from polling to notification-based readings: SDK will read processInfo.isLowPowerModeEnabled only when its value changes.
needless to say, as the author feel free to explain further @earltedly :)

assuming that it changes less often than once in every 5 seconds, this should reduce the reads and number of crashes if not solves the root cause (it's very likely that Apple needs to fix the root cause).

publishAsync is just to make this property thread-safe as it's written in main and is read in a background thread.

if your app experiences similar crashes too, it'd be very helpful to hear your stats if possible 👂
how often does this happen in your case? do you have the exact same stack traces? etc.

Hi @yousefhamza 👋

It's just as @buranmert, at a worst case this will greatly reduce the number of times DatadogSDK access processInfo and thus reduces the potential for hitting this bug.

We can also hope that processInfo itself only notifies when the state has recently changed and that immediately querying it will be unlikely to cause processInfo to immediately find it has changed again (and thus re-broadcast within the same callstack).

Also, it would be great to hear if you're experiencing this too and in what kind of numbers.

To add a bit more context - in worst case scenario, Datadog SDK might read processInfo.isLowPowerModeEnabled from 6 simultaneous threads. Given that we suspect it for not being thread-safe on iOS 15 it could lead to to a crash.

With the proposed change, the value of processInfo.isLowPowerModeEnabled would be read and published from a single thread and with the use of ValuePublisher it can be later accessed from any number of simultaneous threads in a thread-safe manner.

This should solve the crash originating from our SDK. It doesn't though address the problem of some other code (e.g. application code) reading shared ProcessInfo.processInfo.isLowPowerModeEnabled at the same time as we do. I think we can't do anything with this as the problem seems to be on the Apple side. ~~I will fill radar when I manage to reproduce this issue with minimal code on iOS 15 device~~ (@earltedly already did 🏅).

Here's the radar: FB9661108. Filing a duplicate would be great as it increases the priority within Apple's triage system.

@buranmert @earltedly @ncreated Thanks for the very thorough responses 🙏

What I understand is it will help with this fix because:

It reduces how many times isLowPowerModeenabled is queried

Querying it after it has changed will give a much lower chance that it will change again while querying

Reading it from a single thread will reduce the chances of multiple reads (which can cause firing a notification) at the same time, while making it thread safe.

I'm working on Instabug SDK and this issue came into my radar because we use ProcessInfo.processInfo.isLowPowerModeEnabled, we are also prone to this crash too. however, we didn't get any reports of it yet.

@yousefhamza yes, your reasoning about this change is correct 👍 with the additional note that our MobileDevice is a shared component, instantiated only once from SDK init - so we will have only one active subscriber to LPM-change notification.

Thanks for more details, and I hope you will manage to prevent this crash in Instabug SDK 🙂🤞!

Co-authored-by: Mert Buran <buranmert@gmail.com>

earltedly · 2021-09-30T14:31:54Z

@buranmert Thanks for the the CR and fix it suggestions, I've applied them all so 🤞 it will pass now :)

maxep

👌

…S15-crash Prevent iOS 15 crashes by not polling isLowPowerModeEnabled (cherry picked from commit f44f351)

earltedly requested a review from a team as a code owner September 28, 2021 14:31

buranmert reviewed Sep 28, 2021

View reviewed changes

Sources/Datadog/Core/System/MobileDevice.swift Outdated Show resolved Hide resolved

Asavarkhul reviewed Sep 29, 2021

View reviewed changes

DataDog#609 observe changes to isLowPowerMode and cache the values as…

65f6ae9

… they change rather than polling for them on every log submission

earltedly force-pushed the V5IOSCS-1929-isLowPowerMode-iOS15-crash branch from 34b2d76 to 65f6ae9 Compare September 29, 2021 13:31

DataDog#609 - CR feedback (final class and use private dispatch queue)

e0c880a

buranmert suggested changes Sep 30, 2021

View reviewed changes

buranmert reviewed Sep 30, 2021

View reviewed changes

Sources/Datadog/Core/System/MobileDevice.swift Outdated Show resolved Hide resolved

yousefhamza reviewed Sep 30, 2021

View reviewed changes

DataDog#609 Apply suggestions from code review

dae198b

Co-authored-by: Mert Buran <buranmert@gmail.com>

buranmert approved these changes Sep 30, 2021

View reviewed changes

ncreated approved these changes Oct 1, 2021

View reviewed changes

ncreated mentioned this pull request Oct 1, 2021

Crash in CarrierInfoProvider.swift #619

Closed

maxep approved these changes Oct 4, 2021

View reviewed changes

ncreated merged commit f44f351 into DataDog:master Oct 4, 2021

ncreated added a commit that referenced this pull request Oct 4, 2021

Merge pull request #613 from GoodNotes/V5IOSCS-1929-isLowPowerMode-iO…

fffd5c6

…S15-crash Prevent iOS 15 crashes by not polling isLowPowerModeEnabled (cherry picked from commit f44f351)

This was referenced Oct 4, 2021

RUMM-1688 Add tests to "Low Power Mode" setting changes #621

Merged

Release 1.7.1 #622

Merged

pBoelck mentioned this pull request Oct 5, 2021

iOS15 Crash with isLowPowerModeEnabled Tealium/tealium-swift#227

Closed

earltedly deleted the V5IOSCS-1929-isLowPowerMode-iOS15-crash branch October 5, 2021 14:48

earltedly restored the V5IOSCS-1929-isLowPowerMode-iOS15-crash branch October 5, 2021 14:48

This was referenced Oct 7, 2021

RUMM-1691 Reduce utilization of CTTelephonyNetworkInfo to mitigate CarrierInfoProvider crash #627

Merged

Dogfood recent changes #632

Merged

kitoko552 mentioned this pull request Oct 25, 2021

iOS 15 crash on calling isLowPowerModeEnabled react-native-device-info/react-native-device-info#1330

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent iOS 15 crashes by not polling isLowPowerModeEnabled #613

Prevent iOS 15 crashes by not polling isLowPowerModeEnabled #613

earltedly commented Sep 28, 2021 •

edited

Loading

buranmert left a comment

Asavarkhul Sep 29, 2021

earltedly Sep 29, 2021

earltedly commented Sep 29, 2021

buranmert left a comment

yousefhamza Sep 30, 2021 •

edited

Loading

buranmert Sep 30, 2021

earltedly Sep 30, 2021

ncreated Sep 30, 2021 •

edited

Loading

earltedly Oct 1, 2021

yousefhamza Oct 2, 2021

ncreated Oct 3, 2021

earltedly commented Sep 30, 2021

maxep left a comment

Prevent iOS 15 crashes by not polling isLowPowerModeEnabled #613

Prevent iOS 15 crashes by not polling isLowPowerModeEnabled #613

Conversation

earltedly commented Sep 28, 2021 • edited Loading

What and why?

How?

Review checklist

Notes

buranmert left a comment

Choose a reason for hiding this comment

Asavarkhul Sep 29, 2021

Choose a reason for hiding this comment

earltedly Sep 29, 2021

Choose a reason for hiding this comment

earltedly commented Sep 29, 2021

buranmert left a comment

Choose a reason for hiding this comment

yousefhamza Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

buranmert Sep 30, 2021

Choose a reason for hiding this comment

earltedly Sep 30, 2021

Choose a reason for hiding this comment

ncreated Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

earltedly Oct 1, 2021

Choose a reason for hiding this comment

yousefhamza Oct 2, 2021

Choose a reason for hiding this comment

ncreated Oct 3, 2021

Choose a reason for hiding this comment

earltedly commented Sep 30, 2021

maxep left a comment

Choose a reason for hiding this comment

earltedly commented Sep 28, 2021 •

edited

Loading

yousefhamza Sep 30, 2021 •

edited

Loading

ncreated Sep 30, 2021 •

edited

Loading