tree 0af225e738846215c312f1498e1998e979af9e78
parent 1854cb5aedaef2b3e6bbec0c69d148b148b3ecae
author Yohei Yukawa <yukawa@google.com> 1502126279 -0700
committer Yohei Yukawa <yukawa@google.com> 1502126279 -0700

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
      7fa65eef203c4ed3ce00ddef96ccf311d3bfb58c

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