Fix session start race condition
Session finished callback is triggered before the session is ended at
the VibratorManagerService, which can cause a race condition if a second
session is started right away.
This will cause the second session to be ignored because the service is
still cleaning up the previous session.
Fixing the callback to be triggered after all operations at the service
are complete and the session is trully finished.
Bug: 392420704
Test: android.os.cts.VendorVibrationSessionTest
Flag: android.os.vibrator.vendor_vibration_effects
Change-Id: Ia2f1c2dec29eb5bc7515e9de1bd236b77bd84b83
diff --git a/services/core/java/com/android/server/vibrator/VendorVibrationSession.java b/services/core/java/com/android/server/vibrator/VendorVibrationSession.java
index bda3d44..621a128 100644
--- a/services/core/java/com/android/server/vibrator/VendorVibrationSession.java
+++ b/services/core/java/com/android/server/vibrator/VendorVibrationSession.java
@@ -51,6 +51,7 @@
final class VendorVibrationSession extends IVibrationSession.Stub
implements VibrationSession, CancellationSignal.OnCancelListener, IBinder.DeathRecipient {
private static final String TAG = "VendorVibrationSession";
+ private static final boolean DEBUG = false;
/** Calls into VibratorManager functionality needed for playing an {@link ExternalVibration}. */
interface VibratorManagerHooks {
@@ -73,8 +74,8 @@
private final ICancellationSignal mCancellationSignal = CancellationSignal.createTransport();
private final int[] mVibratorIds;
private final long mCreateUptime;
- private final long mCreateTime; // for debugging
- private final IVibrationSessionCallback mCallback;
+ private final long mCreateTime;
+ private final VendorCallbackWrapper mCallback;
private final CallerInfo mCallerInfo;
private final VibratorManagerHooks mManagerHooks;
private final DeviceAdapter mDeviceAdapter;
@@ -88,11 +89,11 @@
@GuardedBy("mLock")
private boolean mEndedByVendor;
@GuardedBy("mLock")
- private long mStartTime; // for debugging
+ private long mStartTime;
@GuardedBy("mLock")
private long mEndUptime;
@GuardedBy("mLock")
- private long mEndTime; // for debugging
+ private long mEndTime;
@GuardedBy("mLock")
private VibrationStepConductor mConductor;
@@ -103,7 +104,7 @@
mCreateTime = System.currentTimeMillis();
mVibratorIds = deviceAdapter.getAvailableVibratorIds();
mHandler = handler;
- mCallback = callback;
+ mCallback = new VendorCallbackWrapper(callback, handler);
mCallerInfo = callerInfo;
mManagerHooks = managerHooks;
mDeviceAdapter = deviceAdapter;
@@ -119,7 +120,9 @@
@Override
public void finishSession() {
- Slog.d(TAG, "Session finish requested, ending vibration session...");
+ if (DEBUG) {
+ Slog.d(TAG, "Session finish requested, ending vibration session...");
+ }
// Do not abort session in HAL, wait for ongoing vibration requests to complete.
// This might take a while to end the session, but it can be aborted by cancelSession.
requestEndSession(Status.FINISHED, /* shouldAbort= */ false, /* isVendorRequest= */ true);
@@ -127,7 +130,9 @@
@Override
public void cancelSession() {
- Slog.d(TAG, "Session cancel requested, aborting vibration session...");
+ if (DEBUG) {
+ Slog.d(TAG, "Session cancel requested, aborting vibration session...");
+ }
// Always abort session in HAL while cancelling it.
// This might be triggered after finishSession was already called.
requestEndSession(Status.CANCELLED_BY_USER, /* shouldAbort= */ true,
@@ -156,7 +161,7 @@
@Override
public IBinder getCallerToken() {
- return mCallback.asBinder();
+ return mCallback.getBinderToken();
}
@Override
@@ -176,36 +181,30 @@
@Override
public void onCancel() {
- Slog.d(TAG, "Session cancellation signal received, aborting vibration session...");
+ if (DEBUG) {
+ Slog.d(TAG, "Session cancellation signal received, aborting vibration session...");
+ }
requestEndSession(Status.CANCELLED_BY_USER, /* shouldAbort= */ true,
/* isVendorRequest= */ true);
}
@Override
public void binderDied() {
- Slog.d(TAG, "Session binder died, aborting vibration session...");
+ if (DEBUG) {
+ Slog.d(TAG, "Session binder died, aborting vibration session...");
+ }
requestEndSession(Status.CANCELLED_BINDER_DIED, /* shouldAbort= */ true,
/* isVendorRequest= */ false);
}
@Override
public boolean linkToDeath() {
- try {
- mCallback.asBinder().linkToDeath(this, 0);
- } catch (RemoteException e) {
- Slog.e(TAG, "Error linking session to token death", e);
- return false;
- }
- return true;
+ return mCallback.linkToDeath(this);
}
@Override
public void unlinkToDeath() {
- try {
- mCallback.asBinder().unlinkToDeath(this, 0);
- } catch (NoSuchElementException e) {
- Slog.wtf(TAG, "Failed to unlink session to token death", e);
- }
+ mCallback.unlinkToDeath(this);
}
@Override
@@ -219,26 +218,37 @@
@Override
public void notifyVibratorCallback(int vibratorId, long vibrationId, long stepId) {
- Slog.d(TAG, "Vibration callback received for vibration " + vibrationId + " step " + stepId
- + " on vibrator " + vibratorId + ", ignoring...");
+ if (DEBUG) {
+ Slog.d(TAG, "Vibration callback received for vibration " + vibrationId
+ + " step " + stepId + " on vibrator " + vibratorId + ", ignoring...");
+ }
}
@Override
public void notifySyncedVibratorsCallback(long vibrationId) {
- Slog.d(TAG, "Synced vibration callback received for vibration " + vibrationId
- + ", ignoring...");
+ if (DEBUG) {
+ Slog.d(TAG, "Synced vibration callback received for vibration " + vibrationId
+ + ", ignoring...");
+ }
}
@Override
public void notifySessionCallback() {
- Slog.d(TAG, "Session callback received, ending vibration session...");
+ if (DEBUG) {
+ Slog.d(TAG, "Session callback received, ending vibration session...");
+ }
synchronized (mLock) {
// If end was not requested then the HAL has cancelled the session.
- maybeSetEndRequestLocked(Status.CANCELLED_BY_UNKNOWN_REASON,
+ notifyEndRequestLocked(Status.CANCELLED_BY_UNKNOWN_REASON,
/* isVendorRequest= */ false);
maybeSetStatusToRequestedLocked();
clearVibrationConductor();
- mHandler.post(() -> mManagerHooks.onSessionReleased(mSessionId));
+ final Status endStatus = mStatus;
+ mHandler.post(() -> {
+ mManagerHooks.onSessionReleased(mSessionId);
+ // Only trigger client callback after session is released in the manager.
+ mCallback.notifyFinished(endStatus);
+ });
}
}
@@ -271,7 +281,7 @@
public boolean isEnded() {
synchronized (mLock) {
- return mStatus != Status.RUNNING;
+ return mEndTime > 0;
}
}
@@ -297,19 +307,17 @@
// Session already ended, skip start callbacks.
isAlreadyEnded = true;
} else {
+ if (DEBUG) {
+ Slog.d(TAG, "Session started at the HAL");
+ }
mStartTime = System.currentTimeMillis();
- // Run client callback in separate thread.
- mHandler.post(() -> {
- try {
- mCallback.onStarted(this);
- } catch (RemoteException e) {
- Slog.e(TAG, "Error notifying vendor session started", e);
- }
- });
+ mCallback.notifyStarted(this);
}
}
if (isAlreadyEnded) {
- Slog.d(TAG, "Session already ended after starting the HAL, aborting...");
+ if (DEBUG) {
+ Slog.d(TAG, "Session already ended after starting the HAL, aborting...");
+ }
mHandler.post(() -> mManagerHooks.endSession(mSessionId, /* shouldAbort= */ true));
}
}
@@ -337,8 +345,10 @@
public boolean maybeSetVibrationConductor(VibrationStepConductor conductor) {
synchronized (mLock) {
if (mConductor != null) {
- Slog.d(TAG, "Session still dispatching previous vibration, new vibration "
- + conductor.getVibration().id + " ignored");
+ if (DEBUG) {
+ Slog.d(TAG, "Session still dispatching previous vibration, new vibration "
+ + conductor.getVibration().id + " ignored");
+ }
return false;
}
mConductor = conductor;
@@ -347,53 +357,45 @@
}
private void requestEndSession(Status status, boolean shouldAbort, boolean isVendorRequest) {
- Slog.d(TAG, "Session end request received with status " + status);
- boolean shouldTriggerSessionHook = false;
+ if (DEBUG) {
+ Slog.d(TAG, "Session end request received with status " + status);
+ }
synchronized (mLock) {
- maybeSetEndRequestLocked(status, isVendorRequest);
+ notifyEndRequestLocked(status, isVendorRequest);
if (!isEnded() && isStarted()) {
// Trigger session hook even if it was already triggered, in case a second request
// is aborting the ongoing/ending session. This might cause it to end right away.
// Wait for HAL callback before setting the end status.
- shouldTriggerSessionHook = true;
+ if (DEBUG) {
+ Slog.d(TAG, "Requesting HAL session end with abort=" + shouldAbort);
+ }
+ mHandler.post(() -> mManagerHooks.endSession(mSessionId, shouldAbort));
} else {
- // Session not active in the HAL, set end status right away.
+ // Session not active in the HAL, try to set end status right away.
maybeSetStatusToRequestedLocked();
+ // Use status used to end this session, which might be different from requested.
+ mCallback.notifyFinished(mStatus);
}
}
- if (shouldTriggerSessionHook) {
- Slog.d(TAG, "Requesting HAL session end with abort=" + shouldAbort);
- mHandler.post(() -> mManagerHooks.endSession(mSessionId, shouldAbort));
- }
}
@GuardedBy("mLock")
- private void maybeSetEndRequestLocked(Status status, boolean isVendorRequest) {
+ private void notifyEndRequestLocked(Status status, boolean isVendorRequest) {
if (mEndStatusRequest != null) {
- // End already requested, keep first requested status and time.
+ // End already requested, keep first requested status.
return;
}
- Slog.d(TAG, "Session end request accepted for status " + status);
+ if (DEBUG) {
+ Slog.d(TAG, "Session end request accepted for status " + status);
+ }
mEndStatusRequest = status;
mEndedByVendor = isVendorRequest;
- mEndTime = System.currentTimeMillis();
- mEndUptime = SystemClock.uptimeMillis();
+ mCallback.notifyFinishing();
if (mConductor != null) {
// Vibration is being dispatched when session end was requested, cancel it.
mConductor.notifyCancelled(new Vibration.EndInfo(status),
/* immediate= */ status != Status.FINISHED);
}
- if (isStarted()) {
- // Only trigger "finishing" callback if session started.
- // Run client callback in separate thread.
- mHandler.post(() -> {
- try {
- mCallback.onFinishing();
- } catch (RemoteException e) {
- Slog.e(TAG, "Error notifying vendor session is finishing", e);
- }
- });
- }
}
@GuardedBy("mLock")
@@ -406,40 +408,123 @@
// No end status was requested, nothing to set.
return;
}
- Slog.d(TAG, "Session end request applied for status " + mEndStatusRequest);
+ if (DEBUG) {
+ Slog.d(TAG, "Session end request applied for status " + mEndStatusRequest);
+ }
mStatus = mEndStatusRequest;
- // Run client callback in separate thread.
- final Status endStatus = mStatus;
- mHandler.post(() -> {
- try {
- mCallback.onFinished(toSessionStatus(endStatus));
- } catch (RemoteException e) {
- Slog.e(TAG, "Error notifying vendor session finished", e);
- }
- });
+ mEndTime = System.currentTimeMillis();
+ mEndUptime = SystemClock.uptimeMillis();
}
- @android.os.vibrator.VendorVibrationSession.Status
- private static int toSessionStatus(Status status) {
- // Exhaustive switch to cover all possible internal status.
- return switch (status) {
- case FINISHED
- -> android.os.vibrator.VendorVibrationSession.STATUS_SUCCESS;
- case IGNORED_UNSUPPORTED
- -> STATUS_UNSUPPORTED;
- case CANCELLED_BINDER_DIED, CANCELLED_BY_APP_OPS, CANCELLED_BY_USER,
- CANCELLED_SUPERSEDED, CANCELLED_BY_FOREGROUND_USER, CANCELLED_BY_SCREEN_OFF,
- CANCELLED_BY_SETTINGS_UPDATE, CANCELLED_BY_UNKNOWN_REASON
- -> android.os.vibrator.VendorVibrationSession.STATUS_CANCELED;
- case IGNORED_APP_OPS, IGNORED_BACKGROUND, IGNORED_FOR_EXTERNAL, IGNORED_FOR_ONGOING,
- IGNORED_FOR_POWER, IGNORED_FOR_SETTINGS, IGNORED_FOR_HIGHER_IMPORTANCE,
- IGNORED_FOR_RINGER_MODE, IGNORED_FROM_VIRTUAL_DEVICE, IGNORED_SUPERSEDED,
- IGNORED_MISSING_PERMISSION, IGNORED_ON_WIRELESS_CHARGER
- -> android.os.vibrator.VendorVibrationSession.STATUS_IGNORED;
- case UNKNOWN, IGNORED_ERROR_APP_OPS, IGNORED_ERROR_CANCELLING, IGNORED_ERROR_SCHEDULING,
- IGNORED_ERROR_TOKEN, FORWARDED_TO_INPUT_DEVICES, FINISHED_UNEXPECTED, RUNNING
- -> android.os.vibrator.VendorVibrationSession.STATUS_UNKNOWN_ERROR;
- };
+ /**
+ * Wrapper class to handle client callbacks asynchronously.
+ *
+ * <p>This class is also responsible for link/unlink to the client process binder death, and for
+ * making sure the callbacks are only triggered once. The conversion between session status and
+ * the API status code is also defined here.
+ */
+ private static final class VendorCallbackWrapper {
+ private final IVibrationSessionCallback mCallback;
+ private final Handler mHandler;
+
+ private boolean mIsStarted;
+ private boolean mIsFinishing;
+ private boolean mIsFinished;
+
+ VendorCallbackWrapper(@NonNull IVibrationSessionCallback callback,
+ @NonNull Handler handler) {
+ mCallback = callback;
+ mHandler = handler;
+ }
+
+ synchronized IBinder getBinderToken() {
+ return mCallback.asBinder();
+ }
+
+ synchronized boolean linkToDeath(DeathRecipient recipient) {
+ try {
+ mCallback.asBinder().linkToDeath(recipient, 0);
+ } catch (RemoteException e) {
+ Slog.e(TAG, "Error linking session to token death", e);
+ return false;
+ }
+ return true;
+ }
+
+ synchronized void unlinkToDeath(DeathRecipient recipient) {
+ try {
+ mCallback.asBinder().unlinkToDeath(recipient, 0);
+ } catch (NoSuchElementException e) {
+ Slog.wtf(TAG, "Failed to unlink session to token death", e);
+ }
+ }
+
+ synchronized void notifyStarted(IVibrationSession session) {
+ if (mIsStarted) {
+ return;
+ }
+ mIsStarted = true;
+ mHandler.post(() -> {
+ try {
+ mCallback.onStarted(session);
+ } catch (RemoteException e) {
+ Slog.e(TAG, "Error notifying vendor session started", e);
+ }
+ });
+ }
+
+ synchronized void notifyFinishing() {
+ if (!mIsStarted || mIsFinishing || mIsFinished) {
+ // Ignore if never started or if already finishing or finished.
+ return;
+ }
+ mIsFinishing = true;
+ mHandler.post(() -> {
+ try {
+ mCallback.onFinishing();
+ } catch (RemoteException e) {
+ Slog.e(TAG, "Error notifying vendor session is finishing", e);
+ }
+ });
+ }
+
+ synchronized void notifyFinished(Status status) {
+ if (mIsFinished) {
+ return;
+ }
+ mIsFinished = true;
+ mHandler.post(() -> {
+ try {
+ mCallback.onFinished(toSessionStatus(status));
+ } catch (RemoteException e) {
+ Slog.e(TAG, "Error notifying vendor session finished", e);
+ }
+ });
+ }
+
+ @android.os.vibrator.VendorVibrationSession.Status
+ private static int toSessionStatus(Status status) {
+ // Exhaustive switch to cover all possible internal status.
+ return switch (status) {
+ case FINISHED
+ -> android.os.vibrator.VendorVibrationSession.STATUS_SUCCESS;
+ case IGNORED_UNSUPPORTED
+ -> STATUS_UNSUPPORTED;
+ case CANCELLED_BINDER_DIED, CANCELLED_BY_APP_OPS, CANCELLED_BY_USER,
+ CANCELLED_SUPERSEDED, CANCELLED_BY_FOREGROUND_USER, CANCELLED_BY_SCREEN_OFF,
+ CANCELLED_BY_SETTINGS_UPDATE, CANCELLED_BY_UNKNOWN_REASON
+ -> android.os.vibrator.VendorVibrationSession.STATUS_CANCELED;
+ case IGNORED_APP_OPS, IGNORED_BACKGROUND, IGNORED_FOR_EXTERNAL, IGNORED_FOR_ONGOING,
+ IGNORED_FOR_POWER, IGNORED_FOR_SETTINGS, IGNORED_FOR_HIGHER_IMPORTANCE,
+ IGNORED_FOR_RINGER_MODE, IGNORED_FROM_VIRTUAL_DEVICE, IGNORED_SUPERSEDED,
+ IGNORED_MISSING_PERMISSION, IGNORED_ON_WIRELESS_CHARGER
+ -> android.os.vibrator.VendorVibrationSession.STATUS_IGNORED;
+ case UNKNOWN, IGNORED_ERROR_APP_OPS, IGNORED_ERROR_CANCELLING,
+ IGNORED_ERROR_SCHEDULING, IGNORED_ERROR_TOKEN, FORWARDED_TO_INPUT_DEVICES,
+ FINISHED_UNEXPECTED, RUNNING
+ -> android.os.vibrator.VendorVibrationSession.STATUS_UNKNOWN_ERROR;
+ };
+ }
}
/**
@@ -499,7 +584,7 @@
@Override
public void logMetrics(VibratorFrameworkStatsLogger statsLogger) {
if (mStartTime > 0) {
- // Only log sessions that have started.
+ // Only log sessions that have started in the HAL.
statsLogger.logVibrationVendorSessionStarted(mCallerInfo.uid);
statsLogger.logVibrationVendorSessionVibrations(mCallerInfo.uid,
mVibrations.size());