Detect false-positive ANRs report and prevent ANR watchdog from sending them #66

konraddysput · 2021-03-09T12:43:04Z

Why

When the user closed an application just after the ANR watchdog ANR check, the next time when an application goes from background to foreground, the ANR watchdog might report a false-positive ANR. To prevent this situation we need to also detect when an application goes to background/foreground and prevent/allow ANR checks depends on the application state.

Technical background

Game objects update method works only when an application is in the foreground, however, our ANR watchdog background thread is able to compare main/background thread time even when an application goes to background. In this situation when an application goes to the background just after ANR check and before any update method, we will report an ANR situation. When an application goes to foreground/background the nativeClient instance sets _preventAnr flag to false/true.
This patch also contains few improvements
- we need to be sure that ANR watchdog uses the most recent time available in the main thread - because of that we changed Update to LateUpdate
- to avoid ThreadAbortExecption we started comparing thread state instead of just aborting thread - long story short, this answer contains all needed resources to read about it: https://stackoverflow.com/a/19048316/4818730
- to prevent reporting ANR when an application goes from background to thread, the nativeClient instance set lastUpdatedTime to 0 - which algorithm will treat like a starting point for detecting ANRs.

Testing strategy

Move application to background on Android/iOS just after ANR detection method and wait more than 5 seconds

previously this use case report an ANR
now it doesn't
tested only manually on Android device/iOS simulator

No unit tests.

Commercialization

This diff will be merged to branch release/3.3.4. After merging all technical changes we will work on the documentation/changelog improvements.

…ng them + anr thread improvements

sbahra · 2021-03-09T13:51:45Z

Runtime/Native/Android/NativeClient.cs

+        /// Determine if the ANR background thread should be disabled or not 
+        /// for some time of period.
+        /// This option will be used by the native client implementation
+        /// once applicaiton goes to background/foreground


done - thanks

Runtime/Native/INativeClient.cs

sbahra · 2021-03-09T13:53:24Z

Runtime/Native/iOS/NativeClient.cs

+        /// Determine if the ANR background thread should be disabled or not 
+        /// for some time of period.
+        /// This option will be used by the native client implementation
+        /// once applicaiton goes to background/foreground


done - thanks

sbahra · 2021-03-09T13:53:33Z

Runtime/Native/iOS/NativeClient.cs

+
+        /// <summary>
+        /// Determine if the ANR background thread should be disabled or not 
+        /// for some time of period.


period of time

done - thanks.

sbahra · 2021-03-09T13:54:28Z

Runtime/Native/iOS/NativeClient.cs

+                        lastUpdatedCache = _lastUpdateTime;
                    }
-                    else
+                    // make sure when ANR happened just after going to foreground


These comments blocks are a bit hard to follow. I would suggest putting braces on same line or put the comment inside the block.

done - thanks.

sbahra · 2021-03-09T13:54:52Z

Runtime/Native/iOS/NativeClient.cs

+        /// <summary>
+        /// Pause ANR detection
+        /// </summary>
+        /// <param name="stopAnr">True - if native client should pause an ANR detection</param>


"should pause ANR detection"

done - thanks

rqbacktrace

As you mentioned previously, we might want to see if we can unify Android/iOS ANR algorithm used by Unity here. Otherwise looks good to me!

vlussenburg

This requires a (unit) test.
a) to prove the problem exists (before the code change)
b) to verify the problem is fixed
and of course c) to show all scenarios we support.

konraddysput · 2021-03-11T23:52:49Z

This requires a (unit) test.
a) to prove the problem exists (before the code change)
b) to verify the problem is fixed
and of course c) to show all scenarios we support.

I wish Unity can provide an easy tool to mock moving device to background and watch background thread time to synchronize them somehow - you were able to reproduce it by just closing an app in the right moment, because timer didn't stop working before main method did and timer continue counting. I wish we can test it but I can't imagine different tests than manual with current test suite and with test tools that we use.

vlussenburg · 2021-03-15T16:04:39Z

This requires a (unit) test.
a) to prove the problem exists (before the code change)
b) to verify the problem is fixed
and of course c) to show all scenarios we support.

I wish Unity can provide an easy tool to mock moving device to background and watch background thread time to synchronize them somehow - you were able to reproduce it by just closing an app in the right moment, because timer didn't stop working before main method did and timer continue counting. I wish we can test it but I can't imagine different tests than manual with current test suite and with test tools that we use.

Check. Makes sense.

…se-positive anrs (#69) * Background thread support (#67) * Background thread support * Fixed exception flow * Removed renamed file * Detect false-positive ANRs report and prevent ANR watchdog from sending them (#66) * Detect false-positive ANRs report and prevent ANR watchdog from sending them + anr thread improvements * Fixed typos + better documentation - added Samy's advices * Attachment support (#68) * Attachment support on Android - iOS shouldn't compile * Updated the latest version of backtrace-android libraries and also adjust iOS integration code - code should now compile on iOS * Native attachment support * Attachment improvements * Updated label * Native client updates * Android libraries * Version update * 3.4.0-rc1 * Prevents from reading empty attachments * Arabic language support (#70) * Fixed invalid date in log manager when calendar is unsupported * Use single method to generate UTC timestamps + move it from extension code to static class to avoid unnecessary allocation * Fixed line endings * Updated native library * Removed debug logs from native library * Updated library version * Added invariant culture to rest of the code * native library update * Fixed issue in the readme file * Correct attachment name (#73) * Correct attachment name * Moved constants to const variables * Prepare for final release * Fixed typo in function name * Changelog change

Detect false-positive ANRs report and prevent ANR watchdog from sendi…

c9ba4da

…ng them + anr thread improvements

konraddysput requested review from Dautery91, rqbacktrace and vlussenburg March 9, 2021 12:43

konraddysput self-assigned this Mar 9, 2021

konraddysput added the bug Something isn't working label Mar 9, 2021

sbahra suggested changes Mar 9, 2021

View reviewed changes

Fixed typos + better documentation - added Samy's advices

f75540c

rqbacktrace approved these changes Mar 11, 2021

View reviewed changes

sbahra approved these changes Mar 11, 2021

View reviewed changes

vlussenburg suggested changes Mar 11, 2021

View reviewed changes

vlussenburg approved these changes Mar 15, 2021

View reviewed changes

Base automatically changed from release/3.3.4 to release/3.4.0 March 15, 2021 18:20

Merge branch 'release/3.4.0' into bugfix/false-positive-anr

5ace489

konraddysput merged commit 55c6efd into release/3.4.0 Mar 15, 2021

konraddysput deleted the bugfix/false-positive-anr branch April 14, 2021 13:31

Detect false-positive ANRs report and prevent ANR watchdog from sending them #66

Detect false-positive ANRs report and prevent ANR watchdog from sending them #66

Uh oh!

Conversation

konraddysput commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Technical background

Testing strategy

Commercialization

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rqbacktrace left a comment

Choose a reason for hiding this comment

Uh oh!

vlussenburg left a comment

Choose a reason for hiding this comment

Uh oh!

konraddysput commented Mar 11, 2021

Uh oh!

vlussenburg commented Mar 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

konraddysput commented Mar 9, 2021 •

edited

Loading