diff options
3 files changed, 169 insertions, 75 deletions
diff --git a/core/java/android/service/contentcapture/ContentCaptureService.java b/core/java/android/service/contentcapture/ContentCaptureService.java index 61744e423b5b..ef55f0615ec7 100644 --- a/core/java/android/service/contentcapture/ContentCaptureService.java +++ b/core/java/android/service/contentcapture/ContentCaptureService.java @@ -60,7 +60,9 @@ import com.android.internal.util.Preconditions; import java.io.FileDescriptor; import java.io.PrintWriter; import java.lang.ref.WeakReference; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.concurrent.Executor; import java.util.function.Consumer; @@ -119,6 +121,9 @@ public abstract class ContentCaptureService extends Service { */ public static final String SERVICE_META_DATA = "android.content_capture"; + private final LocalDataShareAdapterResourceManager mDataShareAdapterResourceManager = + new LocalDataShareAdapterResourceManager(); + private Handler mHandler; private IContentCaptureServiceCallback mCallback; @@ -546,7 +551,8 @@ public abstract class ContentCaptureService extends Service { Preconditions.checkNotNull(executor); DataShareReadAdapterDelegate delegate = - new DataShareReadAdapterDelegate(executor, adapter); + new DataShareReadAdapterDelegate(executor, adapter, + mDataShareAdapterResourceManager); try { callback.accept(delegate); @@ -653,16 +659,17 @@ public abstract class ContentCaptureService extends Service { private static class DataShareReadAdapterDelegate extends IDataShareReadAdapter.Stub { + private final WeakReference<LocalDataShareAdapterResourceManager> mResourceManagerReference; private final Object mLock = new Object(); - private final WeakReference<DataShareReadAdapter> mAdapterReference; - private final WeakReference<Executor> mExecutorReference; - DataShareReadAdapterDelegate(Executor executor, DataShareReadAdapter adapter) { + DataShareReadAdapterDelegate(Executor executor, DataShareReadAdapter adapter, + LocalDataShareAdapterResourceManager resourceManager) { Preconditions.checkNotNull(executor); Preconditions.checkNotNull(adapter); + Preconditions.checkNotNull(resourceManager); - mExecutorReference = new WeakReference<>(executor); - mAdapterReference = new WeakReference<>(adapter); + resourceManager.initializeForDelegate(this, adapter, executor); + mResourceManagerReference = new WeakReference<>(resourceManager); } @Override @@ -670,6 +677,10 @@ public abstract class ContentCaptureService extends Service { throws RemoteException { synchronized (mLock) { executeAdapterMethodLocked(adapter -> adapter.onStart(fd), "onStart"); + + // Client app and Service successfully connected, so this object would be kept alive + // until the session has finished. + clearHardReferences(); } } @@ -678,16 +689,23 @@ public abstract class ContentCaptureService extends Service { synchronized (mLock) { executeAdapterMethodLocked( adapter -> adapter.onError(errorCode), "onError"); + clearHardReferences(); } } private void executeAdapterMethodLocked(Consumer<DataShareReadAdapter> adapterFn, String methodName) { - DataShareReadAdapter adapter = mAdapterReference.get(); - Executor executor = mExecutorReference.get(); + LocalDataShareAdapterResourceManager resourceManager = mResourceManagerReference.get(); + if (resourceManager == null) { + Slog.w(TAG, "Can't execute " + methodName + "(), resource manager has been GC'ed"); + return; + } + + DataShareReadAdapter adapter = resourceManager.getAdapter(this); + Executor executor = resourceManager.getExecutor(this); if (adapter == null || executor == null) { - Slog.w(TAG, "Can't execute " + methodName + "(), references have been GC'ed"); + Slog.w(TAG, "Can't execute " + methodName + "(), references are null"); return; } @@ -698,5 +716,51 @@ public abstract class ContentCaptureService extends Service { Binder.restoreCallingIdentity(identity); } } + + private void clearHardReferences() { + LocalDataShareAdapterResourceManager resourceManager = mResourceManagerReference.get(); + if (resourceManager == null) { + Slog.w(TAG, "Can't clear references, resource manager has been GC'ed"); + return; + } + + resourceManager.clearHardReferences(this); + } + } + + /** + * Wrapper class making sure dependencies on the current application stay in the application + * context. + */ + private static class LocalDataShareAdapterResourceManager { + + // Keeping hard references to the remote objects in the current process (static context) + // to prevent them to be gc'ed during the lifetime of the application. This is an + // artifact of only operating with weak references remotely: there has to be at least 1 + // hard reference in order for this to not be killed. + private Map<DataShareReadAdapterDelegate, DataShareReadAdapter> + mDataShareReadAdapterHardReferences = new HashMap<>(); + private Map<DataShareReadAdapterDelegate, Executor> mExecutorHardReferences = + new HashMap<>(); + + + void initializeForDelegate(DataShareReadAdapterDelegate delegate, + DataShareReadAdapter adapter, Executor executor) { + mDataShareReadAdapterHardReferences.put(delegate, adapter); + mExecutorHardReferences.remove(delegate, executor); + } + + Executor getExecutor(DataShareReadAdapterDelegate delegate) { + return mExecutorHardReferences.get(delegate); + } + + DataShareReadAdapter getAdapter(DataShareReadAdapterDelegate delegate) { + return mDataShareReadAdapterHardReferences.get(delegate); + } + + void clearHardReferences(DataShareReadAdapterDelegate delegate) { + mDataShareReadAdapterHardReferences.remove(delegate); + mExecutorHardReferences.remove(delegate); + } } } diff --git a/core/java/android/view/contentcapture/ContentCaptureManager.java b/core/java/android/view/contentcapture/ContentCaptureManager.java index 954b83b6f9ee..edc6b1251f2c 100644 --- a/core/java/android/view/contentcapture/ContentCaptureManager.java +++ b/core/java/android/view/contentcapture/ContentCaptureManager.java @@ -54,6 +54,8 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.ref.WeakReference; import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; import java.util.Set; import java.util.concurrent.Executor; import java.util.function.Consumer; @@ -360,6 +362,9 @@ public final class ContentCaptureManager { @NonNull private final IContentCaptureManager mService; + @GuardedBy("mLock") + private final LocalDataShareAdapterResourceManager mDataShareAdapterResourceManager; + @NonNull final ContentCaptureOptions mOptions; @@ -399,6 +404,8 @@ public final class ContentCaptureManager { // do, then we should optimize it to run the tests after the Choreographer finishes the most // important steps of the frame. mHandler = Handler.createAsync(Looper.getMainLooper()); + + mDataShareAdapterResourceManager = new LocalDataShareAdapterResourceManager(); } /** @@ -681,7 +688,8 @@ public final class ContentCaptureManager { try { mService.shareData(request, - new DataShareAdapterDelegate(executor, dataShareWriteAdapter)); + new DataShareAdapterDelegate(executor, dataShareWriteAdapter, + mDataShareAdapterResourceManager)); } catch (RemoteException e) { e.rethrowFromSystemServer(); } @@ -737,40 +745,53 @@ public final class ContentCaptureManager { private static class DataShareAdapterDelegate extends IDataShareWriteAdapter.Stub { - private final WeakReference<DataShareWriteAdapter> mAdapterReference; - private final WeakReference<Executor> mExecutorReference; + private final WeakReference<LocalDataShareAdapterResourceManager> mResourceManagerReference; - private DataShareAdapterDelegate(Executor executor, DataShareWriteAdapter adapter) { + private DataShareAdapterDelegate(Executor executor, DataShareWriteAdapter adapter, + LocalDataShareAdapterResourceManager resourceManager) { Preconditions.checkNotNull(executor); Preconditions.checkNotNull(adapter); + Preconditions.checkNotNull(resourceManager); - mExecutorReference = new WeakReference<>(executor); - mAdapterReference = new WeakReference<>(adapter); + resourceManager.initializeForDelegate(this, adapter, executor); + mResourceManagerReference = new WeakReference<>(resourceManager); } @Override public void write(ParcelFileDescriptor destination) throws RemoteException { executeAdapterMethodLocked(adapter -> adapter.onWrite(destination), "onWrite"); + + // Client app and Service successfully connected, so this object would be kept alive + // until the session has finished. + clearHardReferences(); } @Override public void error(int errorCode) throws RemoteException { executeAdapterMethodLocked(adapter -> adapter.onError(errorCode), "onError"); + clearHardReferences(); } @Override public void rejected() throws RemoteException { executeAdapterMethodLocked(DataShareWriteAdapter::onRejected, "onRejected"); + clearHardReferences(); } private void executeAdapterMethodLocked(Consumer<DataShareWriteAdapter> adapterFn, String methodName) { - DataShareWriteAdapter adapter = mAdapterReference.get(); - Executor executor = mExecutorReference.get(); + LocalDataShareAdapterResourceManager resourceManager = mResourceManagerReference.get(); + if (resourceManager == null) { + Slog.w(TAG, "Can't execute " + methodName + "(), resource manager has been GC'ed"); + return; + } + + DataShareWriteAdapter adapter = resourceManager.getAdapter(this); + Executor executor = resourceManager.getExecutor(this); if (adapter == null || executor == null) { - Slog.w(TAG, "Can't execute " + methodName + "(), references have been GC'ed"); + Slog.w(TAG, "Can't execute " + methodName + "(), references are null"); return; } @@ -781,5 +802,50 @@ public final class ContentCaptureManager { Binder.restoreCallingIdentity(identity); } } + + private void clearHardReferences() { + LocalDataShareAdapterResourceManager resourceManager = mResourceManagerReference.get(); + if (resourceManager == null) { + Slog.w(TAG, "Can't clear references, resource manager has been GC'ed"); + return; + } + + resourceManager.clearHardReferences(this); + } + } + + /** + * Wrapper class making sure dependencies on the current application stay in the application + * context. + */ + private static class LocalDataShareAdapterResourceManager { + + // Keeping hard references to the remote objects in the current process (static context) + // to prevent them to be gc'ed during the lifetime of the application. This is an + // artifact of only operating with weak references remotely: there has to be at least 1 + // hard reference in order for this to not be killed. + private Map<DataShareAdapterDelegate, DataShareWriteAdapter> mWriteAdapterHardReferences = + new HashMap<>(); + private Map<DataShareAdapterDelegate, Executor> mExecutorHardReferences = + new HashMap<>(); + + void initializeForDelegate(DataShareAdapterDelegate delegate, DataShareWriteAdapter adapter, + Executor executor) { + mWriteAdapterHardReferences.put(delegate, adapter); + mExecutorHardReferences.remove(delegate, executor); + } + + Executor getExecutor(DataShareAdapterDelegate delegate) { + return mExecutorHardReferences.get(delegate); + } + + DataShareWriteAdapter getAdapter(DataShareAdapterDelegate delegate) { + return mWriteAdapterHardReferences.get(delegate); + } + + void clearHardReferences(DataShareAdapterDelegate delegate) { + mWriteAdapterHardReferences.remove(delegate); + mExecutorHardReferences.remove(delegate); + } } } diff --git a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java index 5d2b9f359381..ce539dabfac1 100644 --- a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java +++ b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java @@ -89,7 +89,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.io.PrintWriter; -import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -919,35 +918,24 @@ public final class ContentCaptureManagerService extends private static class DataShareCallbackDelegate extends IDataShareCallback.Stub { @NonNull private final DataShareRequest mDataShareRequest; - @NonNull private final WeakReference<IDataShareWriteAdapter> mClientAdapterReference; - @NonNull private final WeakReference<ContentCaptureManagerService> mParentServiceReference; + @NonNull private final IDataShareWriteAdapter mClientAdapter; + @NonNull private final ContentCaptureManagerService mParentService; DataShareCallbackDelegate(@NonNull DataShareRequest dataShareRequest, @NonNull IDataShareWriteAdapter clientAdapter, ContentCaptureManagerService parentService) { mDataShareRequest = dataShareRequest; - mClientAdapterReference = new WeakReference<>(clientAdapter); - mParentServiceReference = new WeakReference<>(parentService); + mClientAdapter = clientAdapter; + mParentService = parentService; } @Override public void accept(IDataShareReadAdapter serviceAdapter) throws RemoteException { Slog.i(TAG, "Data share request accepted by Content Capture service"); - final ContentCaptureManagerService parentService = mParentServiceReference.get(); - final IDataShareWriteAdapter clientAdapter = mClientAdapterReference.get(); - if (parentService == null || clientAdapter == null) { - Slog.w(TAG, "Can't fulfill accept() request, because remote objects have been " - + "GC'ed"); - return; - } - - final WeakReference<IDataShareReadAdapter> serviceAdapterReference = - new WeakReference<>(serviceAdapter); - Pair<ParcelFileDescriptor, ParcelFileDescriptor> clientPipe = createPipe(); if (clientPipe == null) { - clientAdapter.error(ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); + mClientAdapter.error(ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); serviceAdapter.error(ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); return; } @@ -959,7 +947,7 @@ public final class ContentCaptureManagerService extends if (servicePipe == null) { bestEffortCloseFileDescriptors(sourceIn, sinkIn); - clientAdapter.error(ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); + mClientAdapter.error(ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); serviceAdapter.error(ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); return; } @@ -967,9 +955,9 @@ public final class ContentCaptureManagerService extends ParcelFileDescriptor sourceOut = servicePipe.second; ParcelFileDescriptor sinkOut = servicePipe.first; - parentService.mPackagesWithShareRequests.add(mDataShareRequest.getPackageName()); + mParentService.mPackagesWithShareRequests.add(mDataShareRequest.getPackageName()); - clientAdapter.write(sourceIn); + mClientAdapter.write(sourceIn); serviceAdapter.start(sinkOut); // File descriptor received by the client app will be a copy of the current one. Close @@ -977,7 +965,7 @@ public final class ContentCaptureManagerService extends // current pipe. bestEffortCloseFileDescriptor(sourceIn); - parentService.mDataShareExecutor.execute(() -> { + mParentService.mDataShareExecutor.execute(() -> { try (InputStream fis = new ParcelFileDescriptor.AutoCloseInputStream(sinkIn); OutputStream fos = @@ -996,23 +984,23 @@ public final class ContentCaptureManagerService extends } catch (IOException e) { Slog.e(TAG, "Failed to pipe client and service streams", e); - sendErrorSignal(mClientAdapterReference, serviceAdapterReference, + sendErrorSignal(mClientAdapter, serviceAdapter, ContentCaptureManager.DATA_SHARE_ERROR_UNKNOWN); } finally { - synchronized (parentService.mLock) { - parentService.mPackagesWithShareRequests + synchronized (mParentService.mLock) { + mParentService.mPackagesWithShareRequests .remove(mDataShareRequest.getPackageName()); } } }); - parentService.mHandler.postDelayed(() -> + mParentService.mHandler.postDelayed(() -> enforceDataSharingTtl( sourceIn, sinkIn, sourceOut, sinkOut, - serviceAdapterReference), + serviceAdapter), MAX_DATA_SHARE_FILE_DESCRIPTORS_TTL_MS); } @@ -1020,31 +1008,17 @@ public final class ContentCaptureManagerService extends public void reject() throws RemoteException { Slog.i(TAG, "Data share request rejected by Content Capture service"); - IDataShareWriteAdapter clientAdapter = mClientAdapterReference.get(); - if (clientAdapter == null) { - Slog.w(TAG, "Can't fulfill reject() request, because remote objects have been " - + "GC'ed"); - return; - } - - clientAdapter.rejected(); + mClientAdapter.rejected(); } private void enforceDataSharingTtl(ParcelFileDescriptor sourceIn, ParcelFileDescriptor sinkIn, ParcelFileDescriptor sourceOut, ParcelFileDescriptor sinkOut, - WeakReference<IDataShareReadAdapter> serviceAdapterReference) { + IDataShareReadAdapter serviceAdapter) { - final ContentCaptureManagerService parentService = mParentServiceReference.get(); - if (parentService == null) { - Slog.w(TAG, "Can't enforce data sharing TTL, because remote objects have been " - + "GC'ed"); - return; - } - - synchronized (parentService.mLock) { - parentService.mPackagesWithShareRequests + synchronized (mParentService.mLock) { + mParentService.mPackagesWithShareRequests .remove(mDataShareRequest.getPackageName()); // Interaction finished successfully <=> all data has been written to Content @@ -1069,7 +1043,7 @@ public final class ContentCaptureManagerService extends bestEffortCloseFileDescriptors(sourceIn, sinkIn, sourceOut, sinkOut); if (!finishedSuccessfully) { - sendErrorSignal(mClientAdapterReference, serviceAdapterReference, + sendErrorSignal(mClientAdapter, serviceAdapter, ContentCaptureManager.DATA_SHARE_ERROR_TIMEOUT_INTERRUPTED); } } @@ -1115,19 +1089,9 @@ public final class ContentCaptureManagerService extends } private static void sendErrorSignal( - WeakReference<IDataShareWriteAdapter> clientAdapterReference, - WeakReference<IDataShareReadAdapter> serviceAdapterReference, + IDataShareWriteAdapter clientAdapter, + IDataShareReadAdapter serviceAdapter, 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) { |