From 17761fff2a5ba2e35b84c932c58508c43191ea38 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 23 Jun 2020 01:33:08 +0100 Subject: [PATCH 1/2] Isolate batched input scheduling logic. Having whether something is currently scheduled get modified in the actual processing logic and away from the scheduling logic makes it unnecessarily more difficult to reason about. In this case, we were only checking whether the Choreographer-aligned runnable was actually scheduled before we'd consume input. When trying to unbuffer dispatches so that things were no longer Choreographer-aligned, however, we'd unschedule the that runnable just before we tried to consume input and hence just before the check . This meant we never actually consumed the batches until the next one came in where we would then consume them immediately. Worse, if another one never came in then we'd never actually consume the batch. By removing the scheduling logic from the processing logic we avoid this situation. Now only the schedule, unschedule, and actual runnable logic touch their own scheduled state, so that we don't have to consider the two pieces of logic together except in isolated places. Bug: 158540186 Bug: 159239700 Test: manual, using app that injects unbuffered dispatch requests either at ACTION_DOWN or first ACTION_MOVE Change-Id: I7521a5a16ed52afc41261f501128b5498ea0602c --- core/java/android/view/ViewRootImpl.java | 37 +++++++++++++----------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index b860bac0d0012..a15f66a60281e 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -8107,7 +8107,9 @@ public final class ViewRootImpl implements ViewParent, } void scheduleConsumeBatchedInput() { - if (!mConsumeBatchedInputScheduled) { + // If anything is currently scheduled to consume batched input then there's no point in + // scheduling it again. + if (!mConsumeBatchedInputScheduled && !mConsumeBatchedInputImmediatelyScheduled) { mConsumeBatchedInputScheduled = true; mChoreographer.postCallback(Choreographer.CALLBACK_INPUT, mConsumedBatchedInputRunnable, null); @@ -8130,22 +8132,15 @@ public final class ViewRootImpl implements ViewParent, } } - void doConsumeBatchedInput(long frameTimeNanos) { - if (mConsumeBatchedInputScheduled) { - mConsumeBatchedInputScheduled = false; - if (mInputEventReceiver != null) { - if (mInputEventReceiver.consumeBatchedInputEvents(frameTimeNanos) - && frameTimeNanos != -1) { - // If we consumed a batch here, we want to go ahead and schedule the - // consumption of batched input events on the next frame. Otherwise, we would - // wait until we have more input events pending and might get starved by other - // things occurring in the process. If the frame time is -1, however, then - // we're in a non-batching mode, so there's no need to schedule this. - scheduleConsumeBatchedInput(); - } - } - doProcessInputEvents(); + boolean doConsumeBatchedInput(long frameTimeNanos) { + final boolean consumedBatches; + if (mInputEventReceiver != null) { + consumedBatches = mInputEventReceiver.consumeBatchedInputEvents(frameTimeNanos); + } else { + consumedBatches = false; } + doProcessInputEvents(); + return consumedBatches; } final class TraversalRunnable implements Runnable { @@ -8218,7 +8213,14 @@ public final class ViewRootImpl implements ViewParent, final class ConsumeBatchedInputRunnable implements Runnable { @Override public void run() { - doConsumeBatchedInput(mChoreographer.getFrameTimeNanos()); + mConsumeBatchedInputScheduled = false; + if (doConsumeBatchedInput(mChoreographer.getFrameTimeNanos())) { + // If we consumed a batch here, we want to go ahead and schedule the + // consumption of batched input events on the next frame. Otherwise, we would + // wait until we have more input events pending and might get starved by other + // things occurring in the process. + scheduleConsumeBatchedInput(); + } } } final ConsumeBatchedInputRunnable mConsumedBatchedInputRunnable = @@ -8228,6 +8230,7 @@ public final class ViewRootImpl implements ViewParent, final class ConsumeBatchedInputImmediatelyRunnable implements Runnable { @Override public void run() { + mConsumeBatchedInputImmediatelyScheduled = false; doConsumeBatchedInput(-1); } } From 48f110ec7bcbc28f9e2f0625f1e6f936827d26d6 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Mon, 22 Jun 2020 12:53:35 -0700 Subject: [PATCH 2/2] Prevent ANR if window is stopped When an activity is stopped, there could still be some pending events in the inputconsumer due to batching. Those events are supposed to be handled when choreographer's 'CALLBACK_INPUT' occurs, but this will never happen if the window is stopped. There are two possible ways these events can occur: 1. The last input event happens before onStop, but at that point, choreographer will already stop sending callbacks. The solution to this is to add a call to "consumeImmediately" inside setWindowStopped. 2. Input events come in after window has already been stopped. At that point, the callback we posted in 1. would already have executed (and found that there's no events). The solution to this is to immediately consume all input if mStopped=true. We do this by switching to "unbuffered" mode for simplicity. This is reproducible via monkey testing, but it's plausible that such race condition can occur in real world usage. Test: adb shell monkey --throttle 40 50000 Bug: 159239700 Change-Id: I74d8f55503ef7df2fd01931afb362d68e5026e86 --- core/java/android/view/ViewRootImpl.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index a15f66a60281e..896a2de8b1354 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -1726,8 +1726,10 @@ public final class ViewRootImpl implements ViewParent, destroySurface(); } } + scheduleConsumeBatchedInputImmediately(); } + /** Register callbacks to be notified when the ViewRootImpl surface changes. */ interface SurfaceChangedCallback { void surfaceCreated(Transaction t); @@ -8184,8 +8186,11 @@ public final class ViewRootImpl implements ViewParent, @Override public void onBatchedInputEventPending(int source) { + // mStopped: There will be no more choreographer callbacks if we are stopped, + // so we must consume all input immediately to prevent ANR final boolean unbuffered = mUnbufferedInputDispatch - || (source & mUnbufferedInputSource) != SOURCE_CLASS_NONE; + || (source & mUnbufferedInputSource) != SOURCE_CLASS_NONE + || mStopped; if (unbuffered) { if (mConsumeBatchedInputScheduled) { unscheduleConsumeBatchedInput();