Fix ConcurrentModificationException due to FrameMetricsAggregator manipulation

[adinauer]

androidx.core.app.FrameMetricsAggregator is not thread safe, calling .remove() from a background thread may cause a
ConcurrentModificationException

📜 Description

Ensure calling .remove() is done on the UI thread.

💡 Motivation and Context

SentryTracer uses a Timer and TimerTask to finish open Transactions on a background thread. Finishing a transaction also triggers the calculation of the frame metrics as well as unregistering the activity from metric collection (FrameMetricsAggregator.remove(activity)), FrameMetricsAggregator is not thread safe, causing sporadic ConcurrentModificationExceptions.

💚 How did you test it?

Ensured current tests are not breaking, otherwise it's hard to test a ConcurrentModificationException.

📝 Checklist

• I reviewed the submitted code
• I added tests to verify the changes
• I updated the docs if needed
• No breaking changes

🔮 Next steps

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	301.41 ms	376.38 ms	74.97 ms
Size	1.74 MiB	2.33 MiB	606.15 KiB

Previous results on branch: 6.4.x

Startup times

Revision	Plain	With Sentry	Diff
5d9a6b1	372.04 ms	429.63 ms	57.59 ms
5a92bc6	352.02 ms	378.21 ms	26.19 ms
efa513a	322.63 ms	362.73 ms	40.10 ms
222b94c	338.71 ms	374.32 ms	35.61 ms
9a87290	318.37 ms	369.65 ms	51.27 ms
f48be76	316.26 ms	363.22 ms	46.96 ms
255f89e	320.54 ms	357.53 ms	36.99 ms

App size

Revision	Plain	With Sentry	Diff
5d9a6b1	1.73 MiB	2.29 MiB	579.50 KiB
5a92bc6	1.74 MiB	2.33 MiB	606.15 KiB
efa513a	1.73 MiB	2.29 MiB	579.50 KiB
222b94c	1.74 MiB	2.33 MiB	606.32 KiB
9a87290	1.73 MiB	2.29 MiB	579.49 KiB
f48be76	1.74 MiB	2.33 MiB	606.32 KiB
255f89e	1.74 MiB	2.33 MiB	606.32 KiB

[marandaneto]

This does not touch OnFrameMetricsAvailableListener.
We don't need a try/catch, if so, what is the concern?

[markushi]

I agree, I still added it in order to make all frameMetricsAggregator.x calls safer. Yes, it's safe now but in theory it could change with a future update to the androidx lib. But I'm fine with removing it 👍

[marandaneto]

Your call, upgrades to the androidx lib can cause new issues, I agree.

[codecov-commenter]

Codecov Report

Base: 80.62% // Head: 80.62% // No change to project coverage 👍

Coverage data is based on head (497457f) compared to base (e63148d).
Patch has no changes to coverable lines.

Additional details and impacted files

@@               Coverage Diff               @@
##             6.4.x-hotfix    #2282   +/-   ##
===============================================
  Coverage           80.62%   80.62%           
  Complexity           3368     3368           
===============================================
  Files                 240      240           
  Lines               12388    12388           
  Branches             1646     1646           
===============================================
  Hits                 9988     9988           
  Misses               1791     1791           
  Partials              609      609           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[markushi]

Seeing this finally getting ready for merge, the changes are functional but definitely not very readable/maintainable.
I was wondering if we should introduce a helper function do deal with the try/catch and ensure being run on the main thread.

E.g.

void runSafelyOnMainThread(Runnable runnable, String tag) {
    try {
        if (MainThreadChecker.isMainThread()) {
            runnable.run();
        } else {
            handler.post(
                () -> {
                try {
                    runnable.run();
                } catch (Throwable t) {
                    if (logger != null) {
                    logger.log(SentryLevel.ERROR, "Failed to run " + tag, t);
                    }
                }
                });
        }
    } catch (Throwable t) {
        if (logger != null) {
            logger.log(SentryLevel.ERROR, "Failed to run" + tag, t);
        }
    }
}

...
runSafelyOnMainThread(() -> { frameMetricsAggregator.stop(); }, "frameMetricsAggregator.stop");

[adinauer]

Feel free to merge. If we can extract some of the duplicated code to make it easier to read / maintain I'd vote for that.

[adinauer]

Can't approve since I created the PR

[philipphofmann]

@adinauer, is there a good reason why this PR doesn't have a description? Can we please add one?

[philipphofmann]

LGTM

[markushi]

I've updated the description (copied it over from the initial PR)

[marandaneto]

Seeing this finally getting ready for merge, the changes are functional but definitely not very readable/maintainable. I was wondering if we should introduce a helper function do deal with the try/catch and ensure being run on the main thread.

E.g.

void runSafelyOnMainThread(Runnable runnable, String tag) {
    try {
        if (MainThreadChecker.isMainThread()) {
            runnable.run();
        } else {
            handler.post(
                () -> {
                try {
                    runnable.run();
                } catch (Throwable t) {
                    if (logger != null) {
                    logger.log(SentryLevel.ERROR, "Failed to run " + tag, t);
                    }
                }
                });
        }
    } catch (Throwable t) {
        if (logger != null) {
            logger.log(SentryLevel.ERROR, "Failed to run" + tag, t);
        }
    }
}

...
runSafelyOnMainThread(() -> { frameMetricsAggregator.stop(); }, "frameMetricsAggregator.stop");

Makes sense, and I'd reuse where we already do something similar.

[adinauer]

Makes sense, and I'd reuse where we already do something similar.

Can we do that as a separate issue / PR as not to block this from being released?