From 439294404fdfc7520873c0dde0f9759a0fb61013 Mon Sep 17 00:00:00 2001 From: Sergey Volnov Date: Tue, 28 Jan 2020 15:36:59 +0000 Subject: [PATCH] Got rid of cancellation signals in the Content Capture Data Sharing Motivation: if the Content Capture service want's to end the session, it can close the read-fd, while not have finished reading, and that would result in an exception in the system server, that can later be propagated to the caller as an error. Similarly, timeout of the sharing session can be reflected as an error with a specific error code. Test: built Android locally and perfomed a manual test. Bug: 145203958 Change-Id: Iaeb45e0ab68da9fe30dce1ae4eab2354ef56d827 --- api/current.txt | 7 +- api/system-current.txt | 2 +- .../contentcapture/ContentCaptureService.java | 35 +----- .../contentcapture/DataShareReadAdapter.java | 10 +- .../contentcapture/IDataShareCallback.aidl | 2 +- .../contentcapture/IDataShareReadAdapter.aidl | 2 +- .../contentcapture/ContentCaptureManager.java | 35 +++--- .../contentcapture/DataShareWriteAdapter.java | 14 +-- .../IContentCaptureManager.aidl | 3 +- .../ContentCaptureManagerService.java | 103 +++++++++--------- 10 files changed, 84 insertions(+), 129 deletions(-) diff --git a/api/current.txt b/api/current.txt index c09dad40e532a..af5a9dbdf336a 100644 --- a/api/current.txt +++ b/api/current.txt @@ -56157,6 +56157,9 @@ package android.view.contentcapture { method public void removeData(@NonNull android.view.contentcapture.DataRemovalRequest); method public void setContentCaptureEnabled(boolean); method public void shareData(@NonNull android.view.contentcapture.DataShareRequest, @NonNull java.util.concurrent.Executor, @NonNull android.view.contentcapture.DataShareWriteAdapter); + field public static final int DATA_SHARE_ERROR_CONCURRENT_REQUEST = 2; // 0x2 + field public static final int DATA_SHARE_ERROR_TIMEOUT_INTERRUPTED = 3; // 0x3 + field public static final int DATA_SHARE_ERROR_UNKNOWN = 1; // 0x1 } public abstract class ContentCaptureSession implements java.lang.AutoCloseable { @@ -56218,9 +56221,7 @@ package android.view.contentcapture { public interface DataShareWriteAdapter { method public default void onError(int); method public void onRejected(); - method public void onWrite(@NonNull android.os.ParcelFileDescriptor, @NonNull android.os.CancellationSignal); - field public static final int ERROR_CONCURRENT_REQUEST = 1; // 0x1 - field public static final int ERROR_UNKNOWN = 2; // 0x2 + method public void onWrite(@NonNull android.os.ParcelFileDescriptor); } } diff --git a/api/system-current.txt b/api/system-current.txt index d1ff478a8590e..703f5acaa6131 100755 --- a/api/system-current.txt +++ b/api/system-current.txt @@ -10186,7 +10186,7 @@ package android.service.contentcapture { public interface DataShareReadAdapter { method public void onError(int); - method public void onStart(@NonNull android.os.ParcelFileDescriptor, @NonNull android.os.CancellationSignal); + method public void onStart(@NonNull android.os.ParcelFileDescriptor); } public final class SnapshotData implements android.os.Parcelable { diff --git a/core/java/android/service/contentcapture/ContentCaptureService.java b/core/java/android/service/contentcapture/ContentCaptureService.java index ac2532dcea7df..5a8521fead871 100644 --- a/core/java/android/service/contentcapture/ContentCaptureService.java +++ b/core/java/android/service/contentcapture/ContentCaptureService.java @@ -34,10 +34,8 @@ import android.content.Intent; import android.content.pm.ParceledListSlice; import android.os.Binder; import android.os.Bundle; -import android.os.CancellationSignal; import android.os.Handler; import android.os.IBinder; -import android.os.ICancellationSignal; import android.os.Looper; import android.os.ParcelFileDescriptor; import android.os.RemoteException; @@ -544,14 +542,11 @@ public abstract class ContentCaptureService extends Service { Preconditions.checkNotNull(adapter); Preconditions.checkNotNull(executor); - ICancellationSignal cancellationSignalTransport = - CancellationSignal.createTransport(); - - DataShareReadAdapterDelegate delegate = new DataShareReadAdapterDelegate( - executor, cancellationSignalTransport, adapter); + DataShareReadAdapterDelegate delegate = + new DataShareReadAdapterDelegate(executor, adapter); try { - callback.accept(cancellationSignalTransport, delegate); + callback.accept(delegate); } catch (RemoteException e) { Slog.e(TAG, "Failed to accept data sharing", e); } @@ -658,38 +653,20 @@ public abstract class ContentCaptureService extends Service { private final Object mLock = new Object(); private final WeakReference mAdapterReference; private final WeakReference mExecutorReference; - private final WeakReference mCancellationSignalReference; - DataShareReadAdapterDelegate(Executor executor, - ICancellationSignal cancellationSignalTransport, DataShareReadAdapter adapter) { + DataShareReadAdapterDelegate(Executor executor, DataShareReadAdapter adapter) { Preconditions.checkNotNull(executor); - Preconditions.checkNotNull(cancellationSignalTransport); Preconditions.checkNotNull(adapter); mExecutorReference = new WeakReference<>(executor); - mCancellationSignalReference = new WeakReference<>(cancellationSignalTransport); mAdapterReference = new WeakReference<>(adapter); } @Override - public void start(ParcelFileDescriptor fd, ICancellationSignal remoteCancellationSignal) + public void start(ParcelFileDescriptor fd) throws RemoteException { synchronized (mLock) { - ICancellationSignal serverControlledCancellationSignal = - mCancellationSignalReference.get(); - - if (serverControlledCancellationSignal == null) { - Slog.w(TAG, "Can't execute onStart(), reference to cancellation signal has " - + "been GC'ed"); - return; - } - - CancellationSignal cancellationSignal = - CancellationSignal.fromTransport(serverControlledCancellationSignal); - cancellationSignal.setRemote(remoteCancellationSignal); - - executeAdapterMethodLocked( - adapter -> adapter.onStart(fd, cancellationSignal), "onStart"); + executeAdapterMethodLocked(adapter -> adapter.onStart(fd), "onStart"); } } diff --git a/core/java/android/service/contentcapture/DataShareReadAdapter.java b/core/java/android/service/contentcapture/DataShareReadAdapter.java index ca6820110ea9c..b9fce6873c5f3 100644 --- a/core/java/android/service/contentcapture/DataShareReadAdapter.java +++ b/core/java/android/service/contentcapture/DataShareReadAdapter.java @@ -18,7 +18,6 @@ package android.service.contentcapture; import android.annotation.NonNull; import android.annotation.SystemApi; -import android.os.CancellationSignal; import android.os.ParcelFileDescriptor; /** @@ -34,13 +33,16 @@ public interface DataShareReadAdapter { * Signals the start of the data sharing session. * * @param fd file descriptor to use for reading data, that's being shared - * @param cancellationSignal cancellation signal to use if data is no longer needed and the - * session needs to be terminated. **/ - void onStart(@NonNull ParcelFileDescriptor fd, @NonNull CancellationSignal cancellationSignal); + void onStart(@NonNull ParcelFileDescriptor fd); /** * Signals that the session failed to start or terminated unsuccessfully. + * + *

Important: together with the error, file sharing stream might be closed, and therefore + * reading from {@code fd} from {@link #onStart} will result in the end of stream. The order of + * these 2 events is not defined, and it's important that the service treats end of stream + * correctly in this situation. **/ void onError(int errorCode); } diff --git a/core/java/android/service/contentcapture/IDataShareCallback.aidl b/core/java/android/service/contentcapture/IDataShareCallback.aidl index d972adadb53c8..84d372580e7c2 100644 --- a/core/java/android/service/contentcapture/IDataShareCallback.aidl +++ b/core/java/android/service/contentcapture/IDataShareCallback.aidl @@ -21,6 +21,6 @@ import android.service.contentcapture.IDataShareReadAdapter; /** @hide */ oneway interface IDataShareCallback { - void accept(in ICancellationSignal cancellationSignal, in IDataShareReadAdapter adapter); + void accept(in IDataShareReadAdapter adapter); void reject(); } diff --git a/core/java/android/service/contentcapture/IDataShareReadAdapter.aidl b/core/java/android/service/contentcapture/IDataShareReadAdapter.aidl index 73da5d515edc1..91c8ff1fc6624 100644 --- a/core/java/android/service/contentcapture/IDataShareReadAdapter.aidl +++ b/core/java/android/service/contentcapture/IDataShareReadAdapter.aidl @@ -20,6 +20,6 @@ import android.os.ICancellationSignal; /** @hide */ oneway interface IDataShareReadAdapter { - void start(in ParcelFileDescriptor fd, in ICancellationSignal cancellationSignal); + void start(in ParcelFileDescriptor fd); void error(int errorCode); } diff --git a/core/java/android/view/contentcapture/ContentCaptureManager.java b/core/java/android/view/contentcapture/ContentCaptureManager.java index cede3b5cf9fe5..b7b54c8c74b9b 100644 --- a/core/java/android/view/contentcapture/ContentCaptureManager.java +++ b/core/java/android/view/contentcapture/ContentCaptureManager.java @@ -32,10 +32,8 @@ import android.content.ContentCaptureOptions; import android.content.Context; import android.graphics.Canvas; import android.os.Binder; -import android.os.CancellationSignal; import android.os.Handler; import android.os.IBinder; -import android.os.ICancellationSignal; import android.os.Looper; import android.os.ParcelFileDescriptor; import android.os.RemoteException; @@ -209,6 +207,15 @@ public final class ContentCaptureManager { private static final String TAG = ContentCaptureManager.class.getSimpleName(); + /** Error happened during the data sharing session. */ + public static final int DATA_SHARE_ERROR_UNKNOWN = 1; + + /** Request has been rejected, because a concurrent data share sessions is in progress. */ + public static final int DATA_SHARE_ERROR_CONCURRENT_REQUEST = 2; + + /** Request has been interrupted because of data share session timeout. */ + public static final int DATA_SHARE_ERROR_TIMEOUT_INTERRUPTED = 3; + /** @hide */ public static final int RESULT_CODE_OK = 0; /** @hide */ @@ -656,12 +663,9 @@ public final class ContentCaptureManager { Preconditions.checkNotNull(dataShareWriteAdapter); Preconditions.checkNotNull(executor); - ICancellationSignal cancellationSignalTransport = CancellationSignal.createTransport(); - try { - mService.shareData(request, cancellationSignalTransport, - new DataShareAdapterDelegate(executor, - cancellationSignalTransport, dataShareWriteAdapter)); + mService.shareData(request, + new DataShareAdapterDelegate(executor, dataShareWriteAdapter)); } catch (RemoteException e) { e.rethrowFromSystemServer(); } @@ -719,32 +723,19 @@ public final class ContentCaptureManager { private final WeakReference mAdapterReference; private final WeakReference mExecutorReference; - private final WeakReference mCancellationSignal; - private DataShareAdapterDelegate(Executor executor, - ICancellationSignal cancellationSignalTransport, DataShareWriteAdapter adapter) { + private DataShareAdapterDelegate(Executor executor, DataShareWriteAdapter adapter) { Preconditions.checkNotNull(executor); - Preconditions.checkNotNull(cancellationSignalTransport); Preconditions.checkNotNull(adapter); mExecutorReference = new WeakReference<>(executor); mAdapterReference = new WeakReference<>(adapter); - mCancellationSignal = new WeakReference<>(cancellationSignalTransport); } @Override public void write(ParcelFileDescriptor destination) throws RemoteException { - ICancellationSignal cancellationSignalTransport = mCancellationSignal.get(); - if (cancellationSignalTransport == null) { - Slog.w(TAG, "Can't execute write(), reference to cancellation signal has been " - + "GC'ed"); - } - CancellationSignal cancellationSignal = - CancellationSignal.fromTransport(cancellationSignalTransport); - - executeAdapterMethodLocked(adapter -> adapter.onWrite(destination, cancellationSignal), - "onWrite"); + executeAdapterMethodLocked(adapter -> adapter.onWrite(destination), "onWrite"); } @Override diff --git a/core/java/android/view/contentcapture/DataShareWriteAdapter.java b/core/java/android/view/contentcapture/DataShareWriteAdapter.java index f791fea7ee8d9..2beaededf8b1e 100644 --- a/core/java/android/view/contentcapture/DataShareWriteAdapter.java +++ b/core/java/android/view/contentcapture/DataShareWriteAdapter.java @@ -17,18 +17,11 @@ package android.view.contentcapture; import android.annotation.NonNull; -import android.os.CancellationSignal; import android.os.ParcelFileDescriptor; /** Adapter class used by apps to share data with the Content Capture service. */ public interface DataShareWriteAdapter { - /** Request has been rejected, because a concurrent data share sessions is in progress. */ - int ERROR_CONCURRENT_REQUEST = 1; - - /** Data share session timed out. */ - int ERROR_UNKNOWN = 2; - /** * Method invoked when the data share session has been started and the app needs to start * writing into the file used for sharing. @@ -36,12 +29,9 @@ public interface DataShareWriteAdapter { *

App needs to handle explicitly cases when the file descriptor is closed and handle * gracefully if IOExceptions happen. * - * @param destination file descriptor used to write data into - * @param cancellationSignal cancellation signal that the app can use to subscribe to cancel - * operations. + * @param destination file descriptor used to write data into. */ - void onWrite(@NonNull ParcelFileDescriptor destination, - @NonNull CancellationSignal cancellationSignal); + void onWrite(@NonNull ParcelFileDescriptor destination); /** Data share sessions has been rejected by the Content Capture service. */ void onRejected(); diff --git a/core/java/android/view/contentcapture/IContentCaptureManager.aidl b/core/java/android/view/contentcapture/IContentCaptureManager.aidl index 5217e68eac505..01ead4601e598 100644 --- a/core/java/android/view/contentcapture/IContentCaptureManager.aidl +++ b/core/java/android/view/contentcapture/IContentCaptureManager.aidl @@ -69,8 +69,7 @@ oneway interface IContentCaptureManager { /** * Requests sharing of a binary data with the content capture service. */ - void shareData(in DataShareRequest request, in ICancellationSignal cancellationSignal, - in IDataShareWriteAdapter adapter); + void shareData(in DataShareRequest request, in IDataShareWriteAdapter adapter); /** * Returns whether the content capture feature is enabled for the calling user. diff --git a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java index 5602d1a851e8d..31ea5faa05f18 100644 --- a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java +++ b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java @@ -45,10 +45,8 @@ import android.database.ContentObserver; import android.os.Binder; import android.os.Build; import android.os.Bundle; -import android.os.CancellationSignal; import android.os.Handler; import android.os.IBinder; -import android.os.ICancellationSignal; import android.os.Looper; import android.os.ParcelFileDescriptor; import android.os.RemoteException; @@ -73,7 +71,6 @@ import android.view.contentcapture.ContentCaptureHelper; import android.view.contentcapture.ContentCaptureManager; import android.view.contentcapture.DataRemovalRequest; import android.view.contentcapture.DataShareRequest; -import android.view.contentcapture.DataShareWriteAdapter; import android.view.contentcapture.IContentCaptureManager; import android.view.contentcapture.IDataShareWriteAdapter; @@ -648,7 +645,6 @@ public final class ContentCaptureManagerService extends @Override public void shareData(@NonNull DataShareRequest request, - @NonNull ICancellationSignal clientCancellationSignal, @NonNull IDataShareWriteAdapter clientAdapter) { Preconditions.checkNotNull(request); Preconditions.checkNotNull(clientAdapter); @@ -662,7 +658,8 @@ public final class ContentCaptureManagerService extends if (mPackagesWithShareRequests.size() >= MAX_CONCURRENT_FILE_SHARING_REQUESTS || mPackagesWithShareRequests.contains(request.getPackageName())) { try { - clientAdapter.error(DataShareWriteAdapter.ERROR_CONCURRENT_REQUEST); + clientAdapter.error( + ContentCaptureManager.DATA_SHARE_ERROR_CONCURRENT_REQUEST); } catch (RemoteException e) { Slog.e(TAG, "Failed to send error message to client"); } @@ -670,8 +667,8 @@ public final class ContentCaptureManagerService extends } service.onDataSharedLocked(request, - new DataShareCallbackDelegate(request, clientCancellationSignal, - clientAdapter, ContentCaptureManagerService.this)); + new DataShareCallbackDelegate(request, clientAdapter, + ContentCaptureManagerService.this)); } } @@ -922,41 +919,36 @@ public final class ContentCaptureManagerService extends private static class DataShareCallbackDelegate extends IDataShareCallback.Stub { @NonNull private final DataShareRequest mDataShareRequest; - @NonNull - private final WeakReference mClientCancellationSignalReference; @NonNull private final WeakReference mClientAdapterReference; @NonNull private final WeakReference mParentServiceReference; DataShareCallbackDelegate(@NonNull DataShareRequest dataShareRequest, - @NonNull ICancellationSignal clientCancellationSignal, @NonNull IDataShareWriteAdapter clientAdapter, ContentCaptureManagerService parentService) { mDataShareRequest = dataShareRequest; - mClientCancellationSignalReference = new WeakReference<>(clientCancellationSignal); mClientAdapterReference = new WeakReference<>(clientAdapter); mParentServiceReference = new WeakReference<>(parentService); } @Override - public void accept(ICancellationSignal serviceCancellationSignal, - IDataShareReadAdapter serviceAdapter) throws RemoteException { + public void accept(IDataShareReadAdapter serviceAdapter) throws RemoteException { Slog.i(TAG, "Data share request accepted by Content Capture service"); final ContentCaptureManagerService parentService = mParentServiceReference.get(); - final ICancellationSignal clientCancellationSignal = - mClientCancellationSignalReference.get(); final IDataShareWriteAdapter clientAdapter = mClientAdapterReference.get(); - if (parentService == null || clientCancellationSignal == null - || clientAdapter == null) { + if (parentService == null || clientAdapter == null) { Slog.w(TAG, "Can't fulfill accept() request, because remote objects have been " + "GC'ed"); return; } + final WeakReference serviceAdapterReference = + new WeakReference<>(serviceAdapter); + Pair clientPipe = createPipe(); if (clientPipe == null) { - clientAdapter.error(DataShareWriteAdapter.ERROR_UNKNOWN); - serviceAdapter.error(DataShareWriteAdapter.ERROR_UNKNOWN); + clientAdapter.error(ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); + serviceAdapter.error(ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); return; } @@ -967,31 +959,18 @@ public final class ContentCaptureManagerService extends if (servicePipe == null) { bestEffortCloseFileDescriptors(sourceIn, sinkIn); - clientAdapter.error(DataShareWriteAdapter.ERROR_UNKNOWN); - serviceAdapter.error(DataShareWriteAdapter.ERROR_UNKNOWN); + clientAdapter.error(ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); + serviceAdapter.error(ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); return; } ParcelFileDescriptor sourceOut = servicePipe.second; ParcelFileDescriptor sinkOut = servicePipe.first; - ICancellationSignal cancellationSignalTransport = - CancellationSignal.createTransport(); parentService.mPackagesWithShareRequests.add(mDataShareRequest.getPackageName()); clientAdapter.write(sourceIn); - serviceAdapter.start(sinkOut, cancellationSignalTransport); - - CancellationSignal cancellationSignal = - CancellationSignal.fromTransport(cancellationSignalTransport); - - cancellationSignal.setOnCancelListener(() -> { - try { - clientCancellationSignal.cancel(); - } catch (RemoteException e) { - Slog.e(TAG, "Failed to propagate cancel operation to the caller", e); - } - }); + serviceAdapter.start(sinkOut); // File descriptor received by the client app will be a copy of the current one. Close // the one that belongs to the system server, so there's only 1 open left for the @@ -1016,6 +995,9 @@ public final class ContentCaptureManagerService extends } } catch (IOException e) { Slog.e(TAG, "Failed to pipe client and service streams", e); + + sendErrorSignal(mClientAdapterReference, serviceAdapterReference, + ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); } }); @@ -1025,7 +1007,7 @@ public final class ContentCaptureManagerService extends sinkIn, sourceOut, sinkOut, - new WeakReference<>(serviceCancellationSignal)), + serviceAdapterReference), MAX_DATA_SHARE_FILE_DESCRIPTORS_TTL_MS); } @@ -1047,15 +1029,10 @@ public final class ContentCaptureManagerService extends ParcelFileDescriptor sinkIn, ParcelFileDescriptor sourceOut, ParcelFileDescriptor sinkOut, - WeakReference serviceCancellationSignalReference) { + WeakReference serviceAdapterReference) { final ContentCaptureManagerService parentService = mParentServiceReference.get(); - final ICancellationSignal clientCancellationSignal = - mClientCancellationSignalReference.get(); - final ICancellationSignal serviceCancellationSignal = - serviceCancellationSignalReference.get(); - if (parentService == null || clientCancellationSignal == null - || serviceCancellationSignal == null) { + if (parentService == null) { Slog.w(TAG, "Can't enforce data sharing TTL, because remote objects have been " + "GC'ed"); return; @@ -1067,7 +1044,7 @@ public final class ContentCaptureManagerService extends // Interaction finished successfully <=> all data has been written to Content // Capture Service. If it hasn't been read successfully, service would be able - // to signal through the cancellation signal. + // to signal by closing the input stream while not have finished reading. boolean finishedSuccessfully = !sinkIn.getFileDescriptor().valid() && !sourceOut.getFileDescriptor().valid(); @@ -1087,16 +1064,8 @@ public final class ContentCaptureManagerService extends bestEffortCloseFileDescriptors(sourceIn, sinkIn, sourceOut, sinkOut); if (!finishedSuccessfully) { - try { - clientCancellationSignal.cancel(); - } catch (RemoteException e) { - Slog.e(TAG, "Failed to cancel() the client operation", e); - } - try { - serviceCancellationSignal.cancel(); - } catch (RemoteException e) { - Slog.e(TAG, "Failed to cancel() the service operation", e); - } + sendErrorSignal(mClientAdapterReference, serviceAdapterReference, + ContentCaptureManager.DATA_SHARE_ERROR_TIMEOUT_INTERRUPTED); } } } @@ -1139,5 +1108,31 @@ public final class ContentCaptureManagerService extends bestEffortCloseFileDescriptor(fd); } } + + private static void sendErrorSignal( + WeakReference clientAdapterReference, + WeakReference serviceAdapterReference, + int errorCode) { + + final IDataShareWriteAdapter clientAdapter = clientAdapterReference.get(); + final IDataShareReadAdapter serviceAdapter = serviceAdapterReference.get(); + + if (clientAdapter == null || serviceAdapter == null) { + Slog.w(TAG, "Can't propagate error() to read/write data share adapters, because " + + "remote objects have been GC'ed"); + return; + } + + try { + clientAdapter.error(errorCode); + } catch (RemoteException e) { + Slog.e(TAG, "Failed to call error() the client operation", e); + } + try { + serviceAdapter.error(errorCode); + } catch (RemoteException e) { + Slog.e(TAG, "Failed to call error() the service operation", e); + } + } } }