drgreen/android_frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Check mInputEventReceiver before sending timeline -- 2

Browse Source 
Try to submit the 'Check mInputEventReceiver' patch again.

This reverts commit 35fccc4535dd8eedd5e60af21651e20215ef4768.

Reason for revert: Added a check to ensure that only valid data is
getting sent. Now, the InputChannel will not be closed because all
InputMessages will always be valid.
Added logging to help investigate the reason why the present time is in
the past of the gpuCompletedTime.

Original commit message:

Previously, it was possible to receive a FrameMetrics callback after the view was already detached from window. In that situation, the mInputEventReceiver is set to null and the object is disposed.

But InputMetricsListener used to store another reference to mInputEventReceiver. So it's own object was never set to null. We would then try to send the timeline to native input receiver, and crash because the native object has already be deleted by the earlier dispose() call.

So the sequence of events was:

dispatchDetachedFromWindow
mInputEventReceiver.dispose()
native input receiver is deleted
InputMetricsListener::onFrameMetricsAvailable
mInputEventReceiver.reportTimeline
try to access a native object using a null pointer
crash

A few options to fix this were investigated:

1) Unregister the observer when mAttachInfo.mThreadedRenderer is set to null.

This is good to do, but it's not sufficient. The problem is that the native call to RenderProxy 'removeObserver' is not serviced immediately, but is posted to be completed sometime in the future. Therefore, the crash would not be fixed by it.
Still, we should always register the observer for the active threadedRenderer, which is done in this CL.

2) Keep a weak reference to mInputEventReceiver inside InputMetricsListener. This would allow InputMetricsListener to check on the status of mInputEventReceiver. When it's disposed, it would be also set to null, so the weak reference resolution would fail.

Unfortunately, 'mInputEventReceiver' is not the only reference to the object of WindowInputEventReceiver. It turns out that the receiver is also stored inside the queued events (see class QueuedInputEvent { private InputEventReceiver mReceiver }). From reviewing ag/153113, it should be OK to remove the receiver from QueuedInputEvent and simply keep track of whether the event is synthesized or not.
But, that change would be too significant to make in this CL. Also, weak references have performance impact, so this may not be desirable anyways.

3) Do not store mInputEventReceiver in InputMetricsListener
The chosen option is to simply use the variable mInputEventReceiver from the outer class. If the receiver is null, we don't notify about the metrics.

Bug: 169866723
Bug: 184255546
Bug: 184771626
Bug: 185015591
Bug: 186664409
Test: verified on fold 2 device, open/close the phone, touch remains
operational
Test: atest InputEventSenderAndReceiverTest
Change-Id: I01076d771e9432f08a3b5f8426c14759b56f3e12

Change-Id: Ie71c9ed38051e3e5ec9cb089951d759f52937fdd
This commit is contained in:
Siarhei Vishniakou 2021-04-23 07:13:02 +00:00
parent ff09af37dd
commit b3ea67fa42
3 changed files with 51 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
core/java/android/view/ViewRootImpl.java
View File

@ -1200,8 +1200,7 @@ public final class ViewRootImpl implements ViewParent,
Looper.myLooper());
if (mAttachInfo.mThreadedRenderer != null) {
InputMetricsListener listener =
new InputMetricsListener(mInputEventReceiver);
InputMetricsListener listener = new InputMetricsListener();
mHardwareRendererObserver = new HardwareRendererObserver(
listener, listener.data, mHandler, true /*waitForPresentTime*/);
mAttachInfo.mThreadedRenderer.addObserver(mHardwareRendererObserver);
@ -1408,6 +1407,9 @@ public final class ViewRootImpl implements ViewParent,
if (mAttachInfo.mThreadedRenderer != null) {
mAttachInfo.mHardwareAccelerated =
mAttachInfo.mHardwareAccelerationRequested = true;
if (mHardwareRendererObserver != null) {
mAttachInfo.mThreadedRenderer.addObserver(mHardwareRendererObserver);
}
}
}
}
@ -8110,6 +8112,9 @@ public final class ViewRootImpl implements ViewParent,
ThreadedRenderer hardwareRenderer = mAttachInfo.mThreadedRenderer;
if (hardwareRenderer != null) {
if (mHardwareRendererObserver != null) {
hardwareRenderer.removeObserver(mHardwareRendererObserver);
}
if (mView != null) {
hardwareRenderer.destroyHardwareResources(mView);
}
@ -8611,18 +8616,12 @@ public final class ViewRootImpl implements ViewParent,
super.dispose();
}
}
WindowInputEventReceiver mInputEventReceiver;
private WindowInputEventReceiver mInputEventReceiver;
final class InputMetricsListener
implements HardwareRendererObserver.OnFrameMetricsAvailableListener {
public long[] data = new long[FrameMetrics.Index.FRAME_STATS_COUNT];
private InputEventReceiver mReceiver;
InputMetricsListener(InputEventReceiver receiver) {
mReceiver = receiver;
}
@Override
public void onFrameMetricsAvailable(int dropCountSinceLastInvocation) {
final int inputEventId = (int) data[FrameMetrics.Index.INPUT_EVENT_ID];
@ -8635,6 +8634,20 @@ public final class ViewRootImpl implements ViewParent,
// available, we cannot compute end-to-end input latency metrics.
return;
}
final long gpuCompletedTime = data[FrameMetrics.Index.GPU_COMPLETED];
if (mInputEventReceiver == null) {
return;
}
if (gpuCompletedTime >= presentTime) {
final double discrepancyMs = (gpuCompletedTime - presentTime) * 1E-6;
final long vsyncId = data[FrameMetrics.Index.FRAME_TIMELINE_VSYNC_ID];
Log.w(TAG, "Not reporting timeline because gpuCompletedTime is " + discrepancyMs
+ "ms ahead of presentTime. FRAME_TIMELINE_VSYNC_ID=" + vsyncId
+ ", INPUT_EVENT_ID=" + inputEventId);
// TODO(b/186664409): figure out why this sometimes happens
return;
}
mInputEventReceiver.reportTimeline(inputEventId, gpuCompletedTime, presentTime);
}
}
HardwareRendererObserver mHardwareRendererObserver;

24
core/jni/android_view_InputEventReceiver.cpp
View File

@ -241,13 +241,13 @@ status_t NativeInputEventReceiver::processOutboundEvents() {
}
// Some other error. Give up
ALOGW("Failed to send outbound event on channel '%s'. status=%d",
getInputChannelName().c_str(), status);
ALOGW("Failed to send outbound event on channel '%s'. status=%s(%d)",
getInputChannelName().c_str(), statusToString(status).c_str(), status);
if (status != DEAD_OBJECT) {
JNIEnv* env = AndroidRuntime::getJNIEnv();
std::string message =
android::base::StringPrintf("Failed to send outbound event. status=%d",
status);
android::base::StringPrintf("Failed to send outbound event. status=%s(%d)",
statusToString(status).c_str(), status);
jniThrowRuntimeException(env, message.c_str());
mMessageQueue->raiseAndClearException(env, "finishInputEvent");
}
@ -319,8 +319,8 @@ status_t NativeInputEventReceiver::consumeEvents(JNIEnv* env,
status_t status = mInputConsumer.consume(&mInputEventFactory,
consumeBatches, frameTime, &seq, &inputEvent);
if (status != OK && status != WOULD_BLOCK) {
ALOGE("channel '%s' ~ Failed to consume input event. status=%d",
getInputChannelName().c_str(), status);
ALOGE("channel '%s' ~ Failed to consume input event. status=%s(%d)",
getInputChannelName().c_str(), statusToString(status).c_str(), status);
return status;
}
@ -502,9 +502,9 @@ static jlong nativeInit(JNIEnv* env, jclass clazz, jobject receiverWeak,
receiverWeak, inputChannel, messageQueue);
status_t status = receiver->initialize();
if (status) {
std::string message =
android::base::StringPrintf("Failed to initialize input event receiver. status=%d",
status);
std::string message = android::base::
StringPrintf("Failed to initialize input event receiver. status=%s(%d)",
statusToString(status).c_str(), status);
jniThrowRuntimeException(env, message.c_str());
return 0;
}
@ -531,7 +531,7 @@ static void nativeFinishInputEvent(JNIEnv* env, jclass clazz, jlong receiverPtr,
if (status != DEAD_OBJECT) {
std::string message =
android::base::StringPrintf("Failed to finish input event. status=%s(%d)",
strerror(-status), status);
statusToString(status).c_str(), status);
jniThrowRuntimeException(env, message.c_str());
}
}
@ -564,8 +564,8 @@ static jboolean nativeConsumeBatchedInputEvents(JNIEnv* env, jclass clazz, jlong
&consumedBatch);
if (status && status != DEAD_OBJECT && !env->ExceptionCheck()) {
std::string message =
android::base::StringPrintf("Failed to consume batched input event. status=%d",
status);
android::base::StringPrintf("Failed to consume batched input event. status=%s(%d)",
statusToString(status).c_str(), status);
jniThrowRuntimeException(env, message.c_str());
return JNI_FALSE;
}

17
tests/Input/src/com/android/test/input/InputEventSenderAndReceiverTest.kt
View File

@ -145,4 +145,21 @@ class InputEventSenderAndReceiverTest {
val received = mSender.getTimeline()
assertEquals(sent, received)
}
// If an invalid timeline is sent, the channel should get closed. This helps surface any
// app-originating bugs early, and forces the work-around to happen in the early stages of the
// event processing.
@Test
fun testSendAndReceiveInvalidTimeline() {
val sent = TestInputEventSender.Timeline(
inputEventId = 1, gpuCompletedTime = 3, presentTime = 2)
mReceiver.reportTimeline(sent.inputEventId, sent.gpuCompletedTime, sent.presentTime)
val received = mSender.getTimeline()
assertEquals(null, received)
// Sender will no longer receive callbacks for this fd, even if receiver sends a valid
// timeline later
mReceiver.reportTimeline(2 /*inputEventId*/, 3 /*gpuCompletedTime*/, 4 /*presentTime*/)
val receivedSecondTimeline = mSender.getTimeline()
assertEquals(null, receivedSecondTimeline)
}
}