Skip to content
Commit 4163a96f authored by Yohei Yukawa's avatar Yohei Yukawa
Browse files

Fix system restart due to race in TSMS

This is a follow up CL to my previous CL [1], which aimed to get rid of
sync IPCs from TextServicesManagerService (TSMS) to
SpellCheckerService (SCS).

What was overlooked is that mSomeArrayList.forEach(action) throws
ConcurrentModificationException when the specified action adds or
removes items to/from mSomeArrayList.  This can happen in my previous CL
when

 1. An app requests a new spell checker service.
 2. TSMS puts the request to mPendingSessionRequests and launchs the
    current SCS by binding to it.
 3. SCS#onCreate() succeeds.
 4. TSMS receives onServiceConnected() callback.
 4. SCS crashes.
 5. TSMS tries to dispatch pending requests to the SCS as follows
    mPendingSessionRequests.forEach(this::getISpellCheckerSessionLocked)
 6. In getISpellCheckerSessionLocked() the IPC to the SCS fails because
    the target SCS process is already dead.  This triggeres removeAll(),
    which clears mPendingSessionRequests.
 7. ConcurrentModificationException is thrown, which will never be
    caught and results in system restart.

Essentially this is a timing issue, but you can most likely reproduce
the issue by manually adding a certain delay in
SpellCheckerBindGroup#onServiceConnected() as follows.

  public void onServiceConnectedLocked(ISpellCheckerService spellChecker) {
      if (DBG) {
          Slog.d(TAG, "onServiceConnected");
      }

 +    try {
 +        Thread.sleep(100 /* msec */);
 +    } catch (InterruptedException e) {
 +    }
      mSpellChecker = spellChecker;
      mConnected = true;
      // Dispatch pending getISpellCheckerSession requests.
      mPendingSessionRequests.forEach(this::getISpellCheckerSessionLocked);
      mPendingSessionRequests.clear();
  }

Then you can also emulate the SCS crash as follows.

  public class CrashingSpellCheckerService extends SpellCheckerService {
      @Override
      public void onCreate() {
          super.onCreate();
          new Thread(new Runnable() {
              @Override
              public void run() {
                  try {
                      Thread.sleep(10 /* msec */);
                  } catch (InterruptedException e) {
                  }
                  Process.killProcess(Process.myPid());
              }
          }).start();
      }

      ...
  }

This CL addresses the above issue by simply inlining
getISpellCheckerSessionLocked() to each call site to make it clear what
will happen in RemoteException is thrown.

 [1]: I92e7aa40dc9ea14f67d355f0bfa15325b775d27b
      7fa65eef

Test: Manually made sure that without this CL the system can restart
      with the above technique.
Test: Manually made sure that AOSP spell checker service still works
      as expected.
Fixes: 63452807
Change-Id: I6de5ac0507574f28a9859b50d081378112f1f7df
parent 1854cb5a
Loading
Loading
Loading
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment