Catch NPE when removing activity from frameMetricsAggregator #4184
Conversation
Coverage Report 1Affected Products
Test Logs |
Unit Test Results 51 files + 36 51 suites +36 1m 35s ⏱️ +42s Results for commit 8d45513. ± Comparison against base commit 6df2784. This pull request removes 152 and adds 974 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Size Report 1Affected Products
Test Logs |
| } catch (NullPointerException err) { | ||
| frameMetricsAggregator.reset(); | ||
| logger.warn( | ||
| "NullPointerException. %s", err.toString()); |
There was a problem hiding this comment.
nit: Does the err.toString() have enough context to understand where the issue came from? Otherwise you can be more clear and say "NullPointerException when calling FrameMetricsAggregator.remove()"
There was a problem hiding this comment.
Recategorized the error, please see updated description
|
Update: I took a second look at the crash log provided in the issue and it seems like the error is happening with So my previous comment (bellow) is not entirely true. @jeremyjiang-dev the underlying issue seems to be the fact that the The pre-24 implementation of Instead of catching this Exception, should we maybe add that condition to static boolean isFrameMetricsRecordingSupported() {
try {
Class<?> initializerClass = Class.forName(FRAME_METRICS_AGGREGATOR_CLASSNAME);
return Build.VERSION.SDK_INT >= 24;
} catch (ClassNotFoundException e) {
return false;
}
} |
firebase-perf/src/main/java/com/google/firebase/perf/application/FrameMetricsRecorder.java
Outdated
Show resolved
Hide resolved
|
Does this mean we are not capture screen rendering when a device has API < 24? |
Thanks Rosário for the investigation! I thought about API 24 could be the issue, but the issue also happens to Android 7, 8, and 9 according to the developer. Android 7, 8, and 9 are mapped to API level 24+, so I am not sure if API 24 was the cause. |
|
@visumickey @raymondlam I updated this PR to categorize this NPE into the same error message "View no hardware accelerated", and a lengthy comment to explain why. I also updated the PR description with more detail explaining with Android Source Code why this is the case. |
|
@leotianlizhan amazing job on the investigation!! Thanks for sharing the results here (publicly instead of a internal source) :) |
leotianlizhan
force-pushed
the
perf-fix-fma
branch
from
10f003f to
e5d8365
Compare
|
PR is moved to #4284 due to CLA issues (PR author leaving Google) blocking merge on this PR. |
Potential fix of #4146
I was not able to reproduce the issue locally. I tried with Android version 7.1 API 25 and turned off hardware acceleration, but could not reproduce the issue.
It could be a bug from older Android versions.
Reference:
Update by @leotianlizhan
Let's take a look at Android's source code, in android.view.View.
The exception was thrown from
findFrameMetricsObserver, insideremoveFrameMetricsListenerSince this null check didn't exists, this triggers NPE when
mFrameMetricsObserveris uninitialized. As Jeremy listed in his reference, this was fixed in later Android versions. Why then, ismFrameMetricsObserveruninitialized? It's becauseaddFrameMetricsListenerfails silently!Conclusion
So overall, this is the same issue caused by the view not being hardware-accelerated. The correct error message is hidden away because
addFrameMetricsListenerfails silently, that's why we should catch them both and use the same error message. The developer's provided crash versions also lines up with when Android fixed it: the above reference fix was made in 2019, and Android 7, 8, 9 are all before 2019, with Android 10 releasing in 2019 thus fixing the issue. There is no crash before Android 7 because as Rosario pointed out, FrameMetrics was added in Android 7 (API 24).