diff --git a/services/core/java/com/android/server/storage/StorageUserConnection.java b/services/core/java/com/android/server/storage/StorageUserConnection.java index 11b6e55dc5e51..b79fc8dd1e1ac 100644 --- a/services/core/java/com/android/server/storage/StorageUserConnection.java +++ b/services/core/java/com/android/server/storage/StorageUserConnection.java @@ -23,7 +23,6 @@ import static android.service.storage.ExternalStorageService.FLAG_SESSION_TYPE_F import static com.android.server.storage.StorageSessionController.ExternalStorageServiceException; import android.annotation.MainThread; -import android.annotation.NonNull; import android.annotation.Nullable; import android.content.ComponentName; import android.content.Context; @@ -35,27 +34,28 @@ import android.os.IBinder; import android.os.ParcelFileDescriptor; import android.os.ParcelableException; import android.os.RemoteCallback; -import android.os.RemoteException; import android.os.UserHandle; import android.os.storage.StorageManagerInternal; import android.os.storage.StorageVolume; import android.service.storage.ExternalStorageService; import android.service.storage.IExternalStorageService; +import android.text.TextUtils; import android.util.Slog; import com.android.internal.annotations.GuardedBy; import com.android.internal.util.Preconditions; import com.android.server.LocalServices; +import com.android.server.pm.UserManagerInternal; import java.io.IOException; -import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; /** * Controls the lifecycle of the {@link ActiveConnection} to an {@link ExternalStorageService} @@ -66,20 +66,25 @@ public final class StorageUserConnection { private static final int DEFAULT_REMOTE_TIMEOUT_SECONDS = 20; - private final Object mSessionsLock = new Object(); + private final Object mLock = new Object(); private final Context mContext; private final int mUserId; private final StorageSessionController mSessionController; private final ActiveConnection mActiveConnection = new ActiveConnection(); + private final boolean mIsDemoUser; @GuardedBy("mLock") private final Map mSessions = new HashMap<>(); - private final HandlerThread mHandlerThread; + @GuardedBy("mLock") @Nullable private HandlerThread mHandlerThread; public StorageUserConnection(Context context, int userId, StorageSessionController controller) { mContext = Objects.requireNonNull(context); mUserId = Preconditions.checkArgumentNonnegative(userId); mSessionController = controller; - mHandlerThread = new HandlerThread("StorageUserConnectionThread-" + mUserId); - mHandlerThread.start(); + mIsDemoUser = LocalServices.getService(UserManagerInternal.class) + .getUserInfo(userId).isDemo(); + if (mIsDemoUser) { + mHandlerThread = new HandlerThread("StorageUserConnectionThread-" + mUserId); + mHandlerThread.start(); + } } /** @@ -96,12 +101,13 @@ public final class StorageUserConnection { Objects.requireNonNull(upperPath); Objects.requireNonNull(lowerPath); - Session session = new Session(sessionId, upperPath, lowerPath); - synchronized (mSessionsLock) { + prepareRemote(); + synchronized (mLock) { Preconditions.checkArgument(!mSessions.containsKey(sessionId)); + Session session = new Session(sessionId, upperPath, lowerPath); mSessions.put(sessionId, session); + mActiveConnection.startSessionLocked(session, pfd); } - mActiveConnection.startSession(session, pfd); } /** @@ -115,7 +121,10 @@ public final class StorageUserConnection { Objects.requireNonNull(sessionId); Objects.requireNonNull(vol); - mActiveConnection.notifyVolumeStateChanged(sessionId, vol); + prepareRemote(); + synchronized (mLock) { + mActiveConnection.notifyVolumeStateChangedLocked(sessionId, vol); + } } /** @@ -126,7 +135,7 @@ public final class StorageUserConnection { * with {@link #waitForExit}. **/ public Session removeSession(String sessionId) { - synchronized (mSessionsLock) { + synchronized (mLock) { return mSessions.remove(sessionId); } } @@ -144,7 +153,10 @@ public final class StorageUserConnection { } Slog.i(TAG, "Waiting for session end " + session + " ..."); - mActiveConnection.endSession(session); + prepareRemote(); + synchronized (mLock) { + mActiveConnection.endSessionLocked(session); + } } /** Restarts all available sessions for a user without blocking. @@ -152,7 +164,7 @@ public final class StorageUserConnection { * Any failures will be ignored. **/ public void resetUserSessions() { - synchronized (mSessionsLock) { + synchronized (mLock) { if (mSessions.isEmpty()) { // Nothing to reset if we have no sessions to restart; we typically // hit this path if the user was consciously shut down. @@ -167,7 +179,7 @@ public final class StorageUserConnection { * Removes all sessions, without waiting. */ public void removeAllSessions() { - synchronized (mSessionsLock) { + synchronized (mLock) { Slog.i(TAG, "Removing " + mSessions.size() + " sessions for user: " + mUserId + "..."); mSessions.clear(); } @@ -179,54 +191,68 @@ public final class StorageUserConnection { */ public void close() { mActiveConnection.close(); - mHandlerThread.quit(); + if (mIsDemoUser) { + mHandlerThread.quit(); + } } /** Returns all created sessions. */ public Set getAllSessionIds() { - synchronized (mSessionsLock) { + synchronized (mLock) { return new HashSet<>(mSessions.keySet()); } } - @FunctionalInterface - interface AsyncStorageServiceCall { - void run(@NonNull IExternalStorageService service, RemoteCallback callback) throws - RemoteException; + private void prepareRemote() throws ExternalStorageServiceException { + try { + waitForLatch(mActiveConnection.bind(), "remote_prepare_user " + mUserId); + } catch (IllegalStateException | TimeoutException e) { + throw new ExternalStorageServiceException("Failed to prepare remote", e); + } + } + + private void waitForLatch(CountDownLatch latch, String reason) throws TimeoutException { + try { + if (!latch.await(DEFAULT_REMOTE_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + // TODO(b/140025078): Call ActivityManager ANR API? + Slog.wtf(TAG, "Failed to bind to the ExternalStorageService for user " + mUserId); + throw new TimeoutException("Latch wait for " + reason + " elapsed"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Latch wait for " + reason + " interrupted"); + } } private final class ActiveConnection implements AutoCloseable { - private final Object mLock = new Object(); - // Lifecycle connection to the external storage service, needed to unbind. @GuardedBy("mLock") @Nullable private ServiceConnection mServiceConnection; - - // A future that holds the remote interface - @GuardedBy("mLock") - @Nullable private CompletableFuture mRemoteFuture; - - // A list of outstanding futures for async calls, for which we are still waiting - // for a callback. Used to unblock waiters if the service dies. - @GuardedBy("mLock") - private ArrayList> mOutstandingOps = new ArrayList<>(); + // True if we are connecting, either bound or binding + // False && mRemote != null means we are connected + // False && mRemote == null means we are neither connecting nor connected + @GuardedBy("mLock") @Nullable private boolean mIsConnecting; + // Binder object representing the external storage service. + // Non-null indicates we are connected + @GuardedBy("mLock") @Nullable private IExternalStorageService mRemote; + // Exception, if any, thrown from #startSessionLocked or #endSessionLocked + // Local variables cannot be referenced from a lambda expression :( so we + // save the exception received in the callback here. Since we guard access + // (and clear the exception state) with the same lock which we hold during + // the entire transaction, there is no risk of race. + @GuardedBy("mLock") @Nullable private ParcelableException mLastException; + // Not guarded by any lock intentionally and non final because we cannot + // reset latches so need to create a new one after one use + private CountDownLatch mLatch; @Override public void close() { ServiceConnection oldConnection = null; synchronized (mLock) { Slog.i(TAG, "Closing connection for user " + mUserId); + mIsConnecting = false; oldConnection = mServiceConnection; mServiceConnection = null; - if (mRemoteFuture != null) { - // Let folks who are waiting for the connection know it ain't gonna happen - mRemoteFuture.cancel(true); - mRemoteFuture = null; - } - // Let folks waiting for callbacks from the remote know it ain't gonna happen - for (CompletableFuture op : mOutstandingOps) { - op.cancel(true); - } - mOutstandingOps.clear(); + mRemote = null; } if (oldConnection != null) { @@ -240,37 +266,37 @@ public final class StorageUserConnection { } } - private void waitForAsync(AsyncStorageServiceCall asyncCall) throws Exception { - CompletableFuture serviceFuture = connectIfNeeded(); - CompletableFuture opFuture = new CompletableFuture<>(); - - try { - synchronized (mLock) { - mOutstandingOps.add(opFuture); - } - serviceFuture.thenCompose(service -> { - try { - asyncCall.run(service, - new RemoteCallback(result -> setResult(result, opFuture))); - } catch (RemoteException e) { - opFuture.completeExceptionally(e); - } - - return opFuture; - }).get(DEFAULT_REMOTE_TIMEOUT_SECONDS, TimeUnit.SECONDS); - } finally { - synchronized (mLock) { - mOutstandingOps.remove(opFuture); - } + public boolean isActiveLocked(Session session) { + if (!session.isInitialisedLocked()) { + Slog.i(TAG, "Session not initialised " + session); + return false; } + + if (mRemote == null) { + throw new IllegalStateException("Valid session with inactive connection"); + } + return true; } - public void startSession(Session session, ParcelFileDescriptor fd) + public void startSessionLocked(Session session, ParcelFileDescriptor fd) throws ExternalStorageServiceException { + if (!isActiveLocked(session)) { + try { + fd.close(); + } catch (IOException e) { + // ignore + } + return; + } + + CountDownLatch latch = new CountDownLatch(1); try { - waitForAsync((service, callback) -> service.startSession(session.sessionId, + mRemote.startSession(session.sessionId, FLAG_SESSION_TYPE_FUSE | FLAG_SESSION_ATTRIBUTE_INDEXABLE, - fd, session.upperPath, session.lowerPath, callback)); + fd, session.upperPath, session.lowerPath, new RemoteCallback(result -> + setResultLocked(latch, result))); + waitForLatch(latch, "start_session " + session); + maybeThrowExceptionLocked(); } catch (Exception e) { throw new ExternalStorageServiceException("Failed to start session: " + session, e); } finally { @@ -282,49 +308,73 @@ public final class StorageUserConnection { } } - public void endSession(Session session) throws ExternalStorageServiceException { + public void endSessionLocked(Session session) throws ExternalStorageServiceException { + if (!isActiveLocked(session)) { + // Nothing to end, not started yet + return; + } + + CountDownLatch latch = new CountDownLatch(1); try { - waitForAsync((service, callback) -> - service.endSession(session.sessionId, callback)); + mRemote.endSession(session.sessionId, new RemoteCallback(result -> + setResultLocked(latch, result))); + waitForLatch(latch, "end_session " + session); + maybeThrowExceptionLocked(); } catch (Exception e) { throw new ExternalStorageServiceException("Failed to end session: " + session, e); } } - - public void notifyVolumeStateChanged(String sessionId, StorageVolume vol) throws + public void notifyVolumeStateChangedLocked(String sessionId, StorageVolume vol) throws ExternalStorageServiceException { + CountDownLatch latch = new CountDownLatch(1); try { - waitForAsync((service, callback) -> - service.notifyVolumeStateChanged(sessionId, vol, callback)); + mRemote.notifyVolumeStateChanged(sessionId, vol, new RemoteCallback( + result -> setResultLocked(latch, result))); + waitForLatch(latch, "notify_volume_state_changed " + vol); + maybeThrowExceptionLocked(); } catch (Exception e) { throw new ExternalStorageServiceException("Failed to notify volume state changed " + "for vol : " + vol, e); } } - private void setResult(Bundle result, CompletableFuture future) { - ParcelableException ex = result.getParcelable(EXTRA_ERROR); - if (ex != null) { - future.completeExceptionally(ex); - } else { - future.complete(null); + private void setResultLocked(CountDownLatch latch, Bundle result) { + mLastException = result.getParcelable(EXTRA_ERROR); + latch.countDown(); + } + + private void maybeThrowExceptionLocked() throws IOException { + if (mLastException != null) { + ParcelableException lastException = mLastException; + mLastException = null; + try { + lastException.maybeRethrow(IOException.class); + } catch (IOException e) { + throw e; + } + throw new RuntimeException(lastException); } } - private CompletableFuture connectIfNeeded() throws - ExternalStorageServiceException { + public CountDownLatch bind() throws ExternalStorageServiceException { ComponentName name = mSessionController.getExternalStorageServiceComponentName(); if (name == null) { // Not ready to bind throw new ExternalStorageServiceException( "Not ready to bind to the ExternalStorageService for user " + mUserId); } + synchronized (mLock) { - if (mRemoteFuture != null) { - return mRemoteFuture; - } - mRemoteFuture = new CompletableFuture<>(); + if (mRemote != null || mIsConnecting) { + // Connected or connecting (bound or binding) + // Will wait on a latch that will countdown when we connect, unless we are + // connected and the latch has already countdown, yay! + return mLatch; + } // else neither connected nor connecting + + mLatch = new CountDownLatch(1); + mIsConnecting = true; mServiceConnection = new ServiceConnection() { @Override public void onServiceConnected(ComponentName name, IBinder service) { @@ -356,9 +406,16 @@ public final class StorageUserConnection { private void handleConnection(IBinder service) { synchronized (mLock) { - mRemoteFuture.complete( - IExternalStorageService.Stub.asInterface(service)); + if (mIsConnecting) { + mRemote = IExternalStorageService.Stub.asInterface(service); + mIsConnecting = false; + mLatch.countDown(); + // Separate thread so we don't block the main thead + return; + } } + Slog.wtf(TAG, "Connection closed to the ExternalStorageService for user " + + mUserId); } private void handleDisconnection() { @@ -372,18 +429,32 @@ public final class StorageUserConnection { }; Slog.i(TAG, "Binding to the ExternalStorageService for user " + mUserId); - // Schedule on a worker thread, because the system server main thread can be - // very busy early in boot. - if (mContext.bindServiceAsUser(new Intent().setComponent(name), - mServiceConnection, - Context.BIND_AUTO_CREATE | Context.BIND_IMPORTANT, - mHandlerThread.getThreadHandler(), - UserHandle.of(mUserId))) { - Slog.i(TAG, "Bound to the ExternalStorageService for user " + mUserId); - return mRemoteFuture; + if (mIsDemoUser) { + // Schedule on a worker thread for demo user to avoid deadlock + if (mContext.bindServiceAsUser(new Intent().setComponent(name), + mServiceConnection, + Context.BIND_AUTO_CREATE | Context.BIND_IMPORTANT, + mHandlerThread.getThreadHandler(), + UserHandle.of(mUserId))) { + Slog.i(TAG, "Bound to the ExternalStorageService for user " + mUserId); + return mLatch; + } else { + mIsConnecting = false; + throw new ExternalStorageServiceException( + "Failed to bind to the ExternalStorageService for user " + mUserId); + } } else { - throw new ExternalStorageServiceException( - "Failed to bind to the ExternalStorageService for user " + mUserId); + if (mContext.bindServiceAsUser(new Intent().setComponent(name), + mServiceConnection, + Context.BIND_AUTO_CREATE | Context.BIND_IMPORTANT, + UserHandle.of(mUserId))) { + Slog.i(TAG, "Bound to the ExternalStorageService for user " + mUserId); + return mLatch; + } else { + mIsConnecting = false; + throw new ExternalStorageServiceException( + "Failed to bind to the ExternalStorageService for user " + mUserId); + } } } } @@ -405,5 +476,10 @@ public final class StorageUserConnection { return "[SessionId: " + sessionId + ". UpperPath: " + upperPath + ". LowerPath: " + lowerPath + "]"; } + + @GuardedBy("mLock") + public boolean isInitialisedLocked() { + return !TextUtils.isEmpty(upperPath) && !TextUtils.isEmpty(lowerPath); + } } }