From 7fa65eef203c4ed3ce00ddef96ccf311d3bfb58c Mon Sep 17 00:00:00 2001 From: Yohei Yukawa Date: Wed, 8 Feb 2017 11:54:05 -0800 Subject: [PATCH] Avoid sync IPCs from TSMS to SpellCheckerService Currently, TextServicesManagerServices uses an AIDL interface called ISpellCheckerService when binding to a spell-checking service. However, this interface uses synchronous (blocking) binder calls rather than asynchronous (oneway) calls. As a result, there are situations where the system process has made a blocking binder call into untrusted application code from its main looper thread. As general policy, the system process must never allow its looper threads to block on application code. This CL addresses the above issue by converting ISpellCheckerService into oneway interface, which instead takes a result receiver ISpellCheckerServiceCallback so that spell-checking services can return results asynchronously. Note that the above protocol issue was also the root cause of Bug 5471520. Hence we can also logically revert the previous CL [1] for Bug 5471520. [1]: Iedf2c2cdd8d4834545d06d72ade3ce211b104b1d 4e713f14419a37f385cf1509b011982bdcf67edc Test: Ran `adb shell dumpsys textservices` to check the "Spell Checker Bind Groups:" section in the following three steps. 1. Before apps start requesting spell checker sessions. 2. While apps are owning active spell checker sessions. 3. After all the apps that owned spell checker sessions are gone. Made sure that spell checker service is not running when there is not spell checker bind group. Bug: 7254002 Change-Id: I92e7aa40dc9ea14f67d355f0bfa15325b775d27b --- Android.mk | 1 + .../textservice/SpellCheckerService.java | 40 ++- .../textservice/ISpellCheckerService.aidl | 24 +- .../ISpellCheckerServiceCallback.aidl | 35 ++ .../ITextServicesSessionListener.aidl | 2 +- .../server/TextServicesManagerService.java | 305 ++++++++++-------- 6 files changed, 260 insertions(+), 147 deletions(-) create mode 100644 core/java/com/android/internal/textservice/ISpellCheckerServiceCallback.aidl diff --git a/Android.mk b/Android.mk index a1e9ed9abca0a..02b141d96338a 100644 --- a/Android.mk +++ b/Android.mk @@ -370,6 +370,7 @@ LOCAL_SRC_FILES += \ core/java/com/android/internal/statusbar/IStatusBar.aidl \ core/java/com/android/internal/statusbar/IStatusBarService.aidl \ core/java/com/android/internal/textservice/ISpellCheckerService.aidl \ + core/java/com/android/internal/textservice/ISpellCheckerServiceCallback.aidl \ core/java/com/android/internal/textservice/ISpellCheckerSession.aidl \ core/java/com/android/internal/textservice/ISpellCheckerSessionListener.aidl \ core/java/com/android/internal/textservice/ITextServicesManager.aidl \ diff --git a/core/java/android/service/textservice/SpellCheckerService.java b/core/java/android/service/textservice/SpellCheckerService.java index 120a37a06958c..bd1b44ca18d01 100644 --- a/core/java/android/service/textservice/SpellCheckerService.java +++ b/core/java/android/service/textservice/SpellCheckerService.java @@ -17,6 +17,7 @@ package android.service.textservice; import com.android.internal.textservice.ISpellCheckerService; +import com.android.internal.textservice.ISpellCheckerServiceCallback; import com.android.internal.textservice.ISpellCheckerSession; import com.android.internal.textservice.ISpellCheckerSessionListener; @@ -311,16 +312,39 @@ public abstract class SpellCheckerService extends Service { mInternalServiceRef = new WeakReference(service); } + /** + * Called from the system when an application is requesting a new spell checker session. + * + *

Note: This is an internal protocol used by the system to establish spell checker + * sessions, which is not guaranteed to be stable and is subject to change.

+ * + * @param locale locale to be returned from {@link Session#getLocale()} + * @param listener IPC channel object to be used to implement + * {@link Session#onGetSuggestionsMultiple(TextInfo[], int, boolean)} and + * {@link Session#onGetSuggestions(TextInfo, int)} + * @param bundle bundle to be returned from {@link Session#getBundle()} + * @param callback IPC channel to return the result to the caller in an asynchronous manner + */ @Override - public ISpellCheckerSession getISpellCheckerSession( - String locale, ISpellCheckerSessionListener listener, Bundle bundle) { + public void getISpellCheckerSession( + String locale, ISpellCheckerSessionListener listener, Bundle bundle, + ISpellCheckerServiceCallback callback) { final SpellCheckerService service = mInternalServiceRef.get(); - if (service == null) return null; - final Session session = service.createSession(); - final InternalISpellCheckerSession internalSession = - new InternalISpellCheckerSession(locale, listener, bundle, session); - session.onCreate(); - return internalSession; + final InternalISpellCheckerSession internalSession; + if (service == null) { + // If the owner SpellCheckerService object was already destroyed and got GC-ed, + // the weak-reference returns null and we should just ignore this request. + internalSession = null; + } else { + final Session session = service.createSession(); + internalSession = + new InternalISpellCheckerSession(locale, listener, bundle, session); + session.onCreate(); + } + try { + callback.onSessionCreated(internalSession); + } catch (RemoteException e) { + } } } diff --git a/core/java/com/android/internal/textservice/ISpellCheckerService.aidl b/core/java/com/android/internal/textservice/ISpellCheckerService.aidl index 67d7b3e524172..6a25964586675 100644 --- a/core/java/com/android/internal/textservice/ISpellCheckerService.aidl +++ b/core/java/com/android/internal/textservice/ISpellCheckerService.aidl @@ -16,16 +16,32 @@ package com.android.internal.textservice; +import com.android.internal.textservice.ISpellCheckerServiceCallback; import com.android.internal.textservice.ISpellCheckerSession; import com.android.internal.textservice.ISpellCheckerSessionListener; import android.os.Bundle; /** - * Public interface to the global spell checker. + * IPC channels from TextServicesManagerService to SpellCheckerService. * @hide */ -interface ISpellCheckerService { - ISpellCheckerSession getISpellCheckerSession( - String locale, ISpellCheckerSessionListener listener, in Bundle bundle); +oneway interface ISpellCheckerService { + /** + * Called from the system when an application is requesting a new spell checker session. + * + *

Note: This is an internal protocol used by the system to establish spell checker sessions, + * which is not guaranteed to be stable and is subject to change.

+ * + * @param locale locale to be returned from + * {@link android.service.textservice.SpellCheckerService.Session#getLocale()} + * @param listener IPC channel object to be used to implement + * {@link android.service.textservice.SpellCheckerService.Session#onGetSuggestionsMultiple(TextInfo[], int, boolean)} and + * {@link android.service.textservice.SpellCheckerService.Session#onGetSuggestions(TextInfo, int)} + * @param bundle bundle to be returned from {@link android.service.textservice.SpellCheckerService.Session#getBundle()} + * @param callback IPC channel to return the result to the caller in an asynchronous manner + */ + void getISpellCheckerSession( + String locale, ISpellCheckerSessionListener listener, in Bundle bundle, + ISpellCheckerServiceCallback callback); } diff --git a/core/java/com/android/internal/textservice/ISpellCheckerServiceCallback.aidl b/core/java/com/android/internal/textservice/ISpellCheckerServiceCallback.aidl new file mode 100644 index 0000000000000..e716cc1bea420 --- /dev/null +++ b/core/java/com/android/internal/textservice/ISpellCheckerServiceCallback.aidl @@ -0,0 +1,35 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.internal.textservice; + +import com.android.internal.textservice.ISpellCheckerSession; +import com.android.internal.textservice.ISpellCheckerSessionListener; + +import android.os.Bundle; + +/** + * IPC channels from SpellCheckerService to TextServicesManagerService. + * @hide + */ +oneway interface ISpellCheckerServiceCallback { + // TODO: Currently SpellCheckerSession just ignores null newSession and continues waiting for + // the next onSessionCreated with non-null newSession, which is supposed to never happen if + // the system is working normally. We should at least free up resources in SpellCheckerSession. + // Note: This method is called from non-system processes, in theory we cannot assume that + // this method is always be called only once with non-null value. + void onSessionCreated(ISpellCheckerSession newSession); +} diff --git a/core/java/com/android/internal/textservice/ITextServicesSessionListener.aidl b/core/java/com/android/internal/textservice/ITextServicesSessionListener.aidl index ecb6cd0f85d19..08d2a5d277e0c 100644 --- a/core/java/com/android/internal/textservice/ITextServicesSessionListener.aidl +++ b/core/java/com/android/internal/textservice/ITextServicesSessionListener.aidl @@ -21,7 +21,7 @@ import com.android.internal.textservice.ISpellCheckerSession; import android.view.textservice.SpellCheckerInfo; /** - * Interface to the text service session. + * (Per-session) IPC channels from TextServicesManagerService to spell checker client applications. * @hide */ interface ITextServicesSessionListener { diff --git a/services/core/java/com/android/server/TextServicesManagerService.java b/services/core/java/com/android/server/TextServicesManagerService.java index 2b5166e0d52cc..feda273b4e5c4 100644 --- a/services/core/java/com/android/server/TextServicesManagerService.java +++ b/services/core/java/com/android/server/TextServicesManagerService.java @@ -20,6 +20,7 @@ import com.android.internal.annotations.GuardedBy; import com.android.internal.content.PackageMonitor; import com.android.internal.inputmethod.InputMethodUtils; import com.android.internal.textservice.ISpellCheckerService; +import com.android.internal.textservice.ISpellCheckerServiceCallback; import com.android.internal.textservice.ISpellCheckerSession; import com.android.internal.textservice.ISpellCheckerSessionListener; import com.android.internal.textservice.ITextServicesManager; @@ -68,7 +69,6 @@ import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.concurrent.CopyOnWriteArrayList; public class TextServicesManagerService extends ITextServicesManager.Stub { private static final String TAG = TextServicesManagerService.class.getSimpleName(); @@ -549,56 +549,26 @@ public class TextServicesManagerService extends ITextServicesManager.Stub { return; } final SpellCheckerInfo sci = mSpellCheckerMap.get(sciId); + SpellCheckerBindGroup bindGroup = mSpellCheckerBindGroups.get(sciId); final int uid = Binder.getCallingUid(); - if (mSpellCheckerBindGroups.containsKey(sciId)) { - final SpellCheckerBindGroup bindGroup = mSpellCheckerBindGroups.get(sciId); - if (bindGroup != null) { - final InternalDeathRecipient recipient = - mSpellCheckerBindGroups.get(sciId).addListener( - tsListener, locale, scListener, uid, bundle); - if (recipient == null) { - if (DBG) { - Slog.w(TAG, "Didn't create a death recipient."); - } - return; - } - if (bindGroup.mSpellChecker == null & bindGroup.mConnected) { - Slog.e(TAG, "The state of the spell checker bind group is illegal."); - bindGroup.removeAll(); - } else if (bindGroup.mSpellChecker != null) { - if (DBG) { - Slog.w(TAG, "Existing bind found. Return a spell checker session now. " - + "Listeners count = " + bindGroup.mListeners.size()); - } - try { - final ISpellCheckerSession session = - bindGroup.mSpellChecker.getISpellCheckerSession( - recipient.mScLocale, recipient.mScListener, bundle); - if (session != null) { - tsListener.onServiceConnected(session); - return; - } else { - if (DBG) { - Slog.w(TAG, "Existing bind already expired. "); - } - bindGroup.removeAll(); - } - } catch (RemoteException e) { - Slog.e(TAG, "Exception in getting spell checker session: " + e); - bindGroup.removeAll(); - } - } + if (bindGroup == null) { + final long ident = Binder.clearCallingIdentity(); + try { + bindGroup = startSpellCheckerServiceInnerLocked(sci); + } finally { + Binder.restoreCallingIdentity(ident); + } + if (bindGroup == null) { + // startSpellCheckerServiceInnerLocked failed. + return; } } - final long ident = Binder.clearCallingIdentity(); - try { - startSpellCheckerServiceInnerLocked( - sci, locale, tsListener, scListener, uid, bundle); - } finally { - Binder.restoreCallingIdentity(ident); - } + + // Start getISpellCheckerSession async IPC, or just queue the request until the spell + // checker service is bound. + bindGroup.getISpellCheckerSessionOrQueueLocked( + new SessionRequest(uid, locale, tsListener, scListener, bundle)); } - return; } @Override @@ -611,9 +581,8 @@ public class TextServicesManagerService extends ITextServicesManager.Stub { } } - private void startSpellCheckerServiceInnerLocked(SpellCheckerInfo info, String locale, - ITextServicesSessionListener tsListener, ISpellCheckerSessionListener scListener, - int uid, Bundle bundle) { + @Nullable + private SpellCheckerBindGroup startSpellCheckerServiceInnerLocked(SpellCheckerInfo info) { if (DBG) { Slog.w(TAG, "Start spell checker session inner locked."); } @@ -627,11 +596,11 @@ public class TextServicesManagerService extends ITextServicesManager.Stub { if (!bindCurrentSpellCheckerService(serviceIntent, connection, Context.BIND_AUTO_CREATE | Context.BIND_FOREGROUND_SERVICE_WHILE_AWAKE)) { Slog.e(TAG, "Failed to get a spell checker service."); - return; + return null; } - final SpellCheckerBindGroup group = new SpellCheckerBindGroup( - connection, tsListener, locale, scListener, uid, bundle); + final SpellCheckerBindGroup group = new SpellCheckerBindGroup(connection); mSpellCheckerBindGroups.put(sciId, group); + return group; } @Override @@ -814,16 +783,32 @@ public class TextServicesManagerService extends ITextServicesManager.Stub { pw.println(" " + ent.getKey() + " " + grp + ":"); pw.println(" " + "mInternalConnection=" + grp.mInternalConnection); pw.println(" " + "mSpellChecker=" + grp.mSpellChecker); - pw.println(" " + "mBound=" + grp.mBound + " mConnected=" + grp.mConnected); + pw.println(" " + "mUnbindCalled=" + grp.mUnbindCalled); + pw.println(" " + "mConnected=" + grp.mConnected); + final int numPendingSessionRequests = grp.mPendingSessionRequests.size(); + for (int i = 0; i < numPendingSessionRequests; i++) { + final SessionRequest req = grp.mPendingSessionRequests.get(i); + pw.println(" " + "Pending Request #" + i + ":"); + pw.println(" " + "mTsListener=" + req.mTsListener); + pw.println(" " + "mScListener=" + req.mScListener); + pw.println(" " + "mScLocale=" + req.mLocale + " mUid=" + req.mUserId); + } + final int numOnGoingSessionRequests = grp.mOnGoingSessionRequests.size(); + for (int i = 0; i < numOnGoingSessionRequests; i++) { + final SessionRequest req = grp.mOnGoingSessionRequests.get(i); + pw.println(" " + "On going Request #" + i + ":"); + ++i; + pw.println(" " + "mTsListener=" + req.mTsListener); + pw.println(" " + "mScListener=" + req.mScListener); + pw.println( + " " + "mScLocale=" + req.mLocale + " mUid=" + req.mUserId); + } final int N = grp.mListeners.size(); for (int i = 0; i < N; i++) { final InternalDeathRecipient listener = grp.mListeners.get(i); pw.println(" " + "Listener #" + i + ":"); - pw.println(" " + "mTsListener=" + listener.mTsListener); pw.println(" " + "mScListener=" + listener.mScListener); pw.println(" " + "mGroup=" + listener.mGroup); - pw.println(" " + "mScLocale=" + listener.mScLocale - + " mUid=" + listener.mUid); } } pw.println(""); @@ -832,25 +817,44 @@ public class TextServicesManagerService extends ITextServicesManager.Stub { } } + private static final class SessionRequest { + @UserIdInt + public final int mUserId; + @Nullable + public final String mLocale; + @NonNull + public final ITextServicesSessionListener mTsListener; + @NonNull + public final ISpellCheckerSessionListener mScListener; + @Nullable + public final Bundle mBundle; + + SessionRequest(@UserIdInt final int userId, @Nullable String locale, + @NonNull ITextServicesSessionListener tsListener, + @NonNull ISpellCheckerSessionListener scListener, @Nullable Bundle bundle) { + mUserId = userId; + mLocale = locale; + mTsListener = tsListener; + mScListener = scListener; + mBundle = bundle; + } + } + // SpellCheckerBindGroup contains active text service session listeners. // If there are no listeners anymore, the SpellCheckerBindGroup instance will be removed from // mSpellCheckerBindGroups private final class SpellCheckerBindGroup { private final String TAG = SpellCheckerBindGroup.class.getSimpleName(); private final InternalServiceConnection mInternalConnection; - private final CopyOnWriteArrayList mListeners = - new CopyOnWriteArrayList<>(); - public boolean mBound; - public ISpellCheckerService mSpellChecker; - public boolean mConnected; + private final ArrayList mListeners = new ArrayList<>(); + private boolean mUnbindCalled; + private ISpellCheckerService mSpellChecker; + private boolean mConnected; + private final ArrayList mPendingSessionRequests = new ArrayList<>(); + private final ArrayList mOnGoingSessionRequests = new ArrayList<>(); - public SpellCheckerBindGroup(InternalServiceConnection connection, - ITextServicesSessionListener listener, String locale, - ISpellCheckerSessionListener scListener, int uid, Bundle bundle) { + public SpellCheckerBindGroup(InternalServiceConnection connection) { mInternalConnection = connection; - mBound = true; - mConnected = false; - addListener(listener, locale, scListener, uid, bundle); } public void onServiceConnected(ISpellCheckerService spellChecker) { @@ -858,55 +862,15 @@ public class TextServicesManagerService extends ITextServicesManager.Stub { Slog.d(TAG, "onServiceConnected"); } - for (InternalDeathRecipient listener : mListeners) { - try { - final ISpellCheckerSession session = spellChecker.getISpellCheckerSession( - listener.mScLocale, listener.mScListener, listener.mBundle); - synchronized(mSpellCheckerMap) { - if (mListeners.contains(listener)) { - listener.mTsListener.onServiceConnected(session); - } - } - } catch (RemoteException e) { - Slog.e(TAG, "Exception in getting the spell checker session." - + "Reconnect to the spellchecker. ", e); - removeAll(); - return; - } - } synchronized(mSpellCheckerMap) { mSpellChecker = spellChecker; mConnected = true; + // Dispatch pending getISpellCheckerSession requests. + mPendingSessionRequests.forEach(this::getISpellCheckerSessionLocked); + mPendingSessionRequests.clear(); } } - public InternalDeathRecipient addListener(ITextServicesSessionListener tsListener, - String locale, ISpellCheckerSessionListener scListener, int uid, Bundle bundle) { - if (DBG) { - Slog.d(TAG, "addListener: " + locale); - } - InternalDeathRecipient recipient = null; - synchronized(mSpellCheckerMap) { - try { - final int size = mListeners.size(); - for (int i = 0; i < size; ++i) { - if (mListeners.get(i).hasSpellCheckerListener(scListener)) { - // do not add the lister if the group already contains this. - return null; - } - } - recipient = new InternalDeathRecipient( - this, tsListener, locale, scListener, uid, bundle); - scListener.asBinder().linkToDeath(recipient, 0); - mListeners.add(recipient); - } catch(RemoteException e) { - // do nothing - } - cleanLocked(); - } - return recipient; - } - public void removeListener(ISpellCheckerSessionListener listener) { if (DBG) { Slog.w(TAG, "remove listener: " + listener.hashCode()); @@ -941,20 +905,29 @@ public class TextServicesManagerService extends ITextServicesManager.Stub { if (DBG) { Slog.d(TAG, "cleanLocked"); } - // If there are no more active listeners, clean up. Only do this - // once. - if (mBound && mListeners.isEmpty()) { - mBound = false; - final String sciId = mInternalConnection.mSciId; - SpellCheckerBindGroup cur = mSpellCheckerBindGroups.get(sciId); - if (cur == this) { - if (DBG) { - Slog.d(TAG, "Remove bind group."); - } - mSpellCheckerBindGroups.remove(sciId); - } - mContext.unbindService(mInternalConnection); + if (mUnbindCalled) { + return; } + // If there are no more active listeners, clean up. Only do this once. + if (!mListeners.isEmpty()) { + return; + } + if (!mPendingSessionRequests.isEmpty()) { + return; + } + if (!mOnGoingSessionRequests.isEmpty()) { + return; + } + final String sciId = mInternalConnection.mSciId; + final SpellCheckerBindGroup cur = mSpellCheckerBindGroups.get(sciId); + if (cur == this) { + if (DBG) { + Slog.d(TAG, "Remove bind group."); + } + mSpellCheckerBindGroups.remove(sciId); + } + mContext.unbindService(mInternalConnection); + mUnbindCalled = true; } public void removeAll() { @@ -966,6 +939,59 @@ public class TextServicesManagerService extends ITextServicesManager.Stub { idr.mScListener.asBinder().unlinkToDeath(idr, 0); } mListeners.clear(); + mPendingSessionRequests.clear(); + mOnGoingSessionRequests.clear(); + cleanLocked(); + } + } + + public void getISpellCheckerSessionOrQueueLocked(@NonNull SessionRequest request) { + if (mUnbindCalled) { + return; + } + if (!mConnected) { + mPendingSessionRequests.add(request); + return; + } + getISpellCheckerSessionLocked(request); + } + + private void getISpellCheckerSessionLocked(@NonNull SessionRequest request) { + if (mUnbindCalled) { + return; + } + try { + mSpellChecker.getISpellCheckerSession( + request.mLocale, request.mScListener, request.mBundle, + new ISpellCheckerServiceCallbackBinder(this, request)); + mOnGoingSessionRequests.add(request); + } catch(RemoteException e) { + // The target spell checker service is not available. Better to reset the state. + removeAll(); + } + cleanLocked(); + } + + void onSessionCreated(@Nullable final ISpellCheckerSession newSession, + @NonNull final SessionRequest request) { + synchronized (mSpellCheckerMap) { + if (mUnbindCalled) { + return; + } + if (mOnGoingSessionRequests.remove(request)) { + final InternalDeathRecipient recipient = + new InternalDeathRecipient(this, request.mScListener); + try { + request.mTsListener.onServiceConnected(newSession); + request.mScListener.asBinder().linkToDeath(recipient, 0); + mListeners.add(recipient); + } catch (RemoteException e) { + // Technically this can happen if the spell checker client app is already + // dead. We can just forget about this request; the request is already + // removed from mOnGoingSessionRequests and the death recipient listener is + // not yet added to mListeners. There is nothing to release further. + } + } cleanLocked(); } } @@ -1008,21 +1034,13 @@ public class TextServicesManagerService extends ITextServicesManager.Stub { } private static final class InternalDeathRecipient implements IBinder.DeathRecipient { - public final ITextServicesSessionListener mTsListener; public final ISpellCheckerSessionListener mScListener; - public final String mScLocale; private final SpellCheckerBindGroup mGroup; - public final int mUid; - public final Bundle mBundle; + public InternalDeathRecipient(SpellCheckerBindGroup group, - ITextServicesSessionListener tsListener, String scLocale, - ISpellCheckerSessionListener scListener, int uid, Bundle bundle) { - mTsListener = tsListener; + ISpellCheckerSessionListener scListener) { mScListener = scListener; - mScLocale = scLocale; mGroup = group; - mUid = uid; - mBundle = bundle; } public boolean hasSpellCheckerListener(ISpellCheckerSessionListener listener) { @@ -1035,6 +1053,25 @@ public class TextServicesManagerService extends ITextServicesManager.Stub { } } + private static final class ISpellCheckerServiceCallbackBinder + extends ISpellCheckerServiceCallback.Stub { + @NonNull + private final SpellCheckerBindGroup mBindGroup; + @NonNull + private final SessionRequest mRequest; + + ISpellCheckerServiceCallbackBinder(@NonNull final SpellCheckerBindGroup bindGroup, + @NonNull final SessionRequest request) { + mBindGroup = bindGroup; + mRequest = request; + } + + @Override + public void onSessionCreated(@Nullable ISpellCheckerSession newSession) { + mBindGroup.onSessionCreated(newSession, mRequest); + } + } + private static final class TextServicesSettings { private final ContentResolver mResolver; @UserIdInt