Print job files and print job records not always cleaned up.

1. We want the files for a print job to be removed as early as possible
   typically because the print job was cancelled, completed, the app
   or the spooler crashed during print job construction. We were keeping
   around in the spooler and hence to disc infos for jobs that are in
   final state since the app that created them may hold a reference to
   a local print job objec whose info it can access to get the latest
   print job state potentially after the job reached final state. The
   issue was that we were persisting to disc created print jobs which
   were during construction which requires careful handling for the
   various cases above. This is tricky and error prone.

   We used to tell the spooler to forget the print jobs infos when the
   app that created them died. The implementation to forget a print
   job was not careful and was nuking currently running print jobs in
   addition to the ones in a terminal state. Further, if the app dies
   before a print job is completed we were left with a stale print
   job in the spooler since we missed the signal to forget it (assuming
   we forget only inactive jobs). These issues suggest that the approach
   is problematic.

   Now we have a cache of print job infos for the jobs an app created.
   This cache is updated when the state of a print jobs changes using
   the new print job state observation code. When the app dies we
   remove the cached jobs for that app. Now if the app calls to get
   the print jobs it gets the cached ones, i.e. the print jobs it
   created during its lifetime, plus the print jobs that are still
   active fetched from the spooler. Note that transient state cannot
   be kept in the spooler since we unbind from it if there is no
   work and it may get killed.

2. Improved the print sub-system logging code to show the cached
   print job infos for apps and also dump the print job PDF file
   names.

bug:10958357

Change-Id: I6f7c1968b6b7ba5be182a10df044ff7ea1fc3a61

2 files changed, 153 insertions, 83 deletions

diff --git a/services/java/com/android/server/print/RemotePrintSpooler.java b/services/java/com/android/server/print/RemotePrintSpooler.java
index f98a805..798cea3 100644
--- a/services/java/com/android/server/print/RemotePrintSpooler.java
+++ b/services/java/com/android/server/print/RemotePrintSpooler.java
@@ -33,7 +33,6 @@ import android.print.IPrintSpoolerCallbacks;
 import android.print.IPrintSpoolerClient;
 import android.print.PrintJobId;
 import android.print.PrintJobInfo;
-import android.print.PrintManager;
 import android.util.Slog;
 import android.util.TimedRemoteCaller;
 
@@ -91,7 +90,7 @@ final class RemotePrintSpooler {
     public static interface PrintSpoolerCallbacks {
         public void onPrintJobQueued(PrintJobInfo printJob);
         public void onAllPrintJobsForServiceHandled(ComponentName printService);
-        public void onPrintJobStateChanged(PrintJobId printJobId, int appId);
+        public void onPrintJobStateChanged(PrintJobInfo printJob);
     }
 
     public RemotePrintSpooler(Context context, int userId,
@@ -280,30 +279,6 @@ final class RemotePrintSpooler {
         }
     }
 
-    public final void forgetPrintJobs(List<PrintJobId> printJobIds) {
-        throwIfCalledOnMainThread();
-        synchronized (mLock) {
-            throwIfDestroyedLocked();
-            mCanUnbind = false;
-        }
-        try {
-            getRemoteInstanceLazy().forgetPrintJobs(printJobIds);
-        } catch (RemoteException re) {
-            Slog.e(LOG_TAG, "Error forgeting print jobs", re);
-        } catch (TimeoutException te) {
-            Slog.e(LOG_TAG, "Error forgeting print jobs", te);
-        } finally {
-            if (DEBUG) {
-                Slog.i(LOG_TAG, "[user: " + mUserHandle.getIdentifier()
-                        + "] forgetPrintJobs()");
-            }
-            synchronized (mLock) {
-                mCanUnbind = true;
-                mLock.notifyAll();
-            }
-        }
-    }
-
     public final void destroy() {
         throwIfCalledOnMainThread();
         if (DEBUG) {
@@ -323,18 +298,15 @@ final class RemotePrintSpooler {
                     .append(String.valueOf(mDestroyed)).println();
             pw.append(prefix).append("bound=")
                     .append((mRemoteInstance != null) ? "true" : "false").println();
-            pw.append(prefix).append("print jobs:").println();
-            if (mRemoteInstance != null) {
-                List<PrintJobInfo> printJobs = getPrintJobInfos(null,
-                        PrintJobInfo.STATE_ANY, PrintManager.APP_ID_ANY);
-                if (printJobs != null) {
-                    final int printJobCount = printJobs.size();
-                    for (int i = 0; i < printJobCount; i++) {
-                        PrintJobInfo printJob = printJobs.get(i);
-                        pw.append(prefix).append(prefix).append(printJob.toString());
-                        pw.println();
-                    }
-                }
+
+            pw.flush();
+
+            try {
+                getRemoteInstanceLazy().asBinder().dump(fd, new String[]{prefix});
+            } catch (TimeoutException te) {
+                /* ignore */
+            } catch (RemoteException re) {
+                /* ignore */
             }
         }
     }
@@ -346,8 +318,8 @@ final class RemotePrintSpooler {
         }
     }
 
-    private void onPrintJobStateChanged(PrintJobId printJobId, int appId) {
-        mCallbacks.onPrintJobStateChanged(printJobId, appId);
+    private void onPrintJobStateChanged(PrintJobInfo printJob) {
+        mCallbacks.onPrintJobStateChanged(printJob);
     }
 
     private IPrintSpooler getRemoteInstanceLazy() throws TimeoutException {
@@ -625,12 +597,12 @@ final class RemotePrintSpooler {
         }
 
         @Override
-        public void onPrintJobStateChanged(PrintJobId printJobId, int appId) {
+        public void onPrintJobStateChanged(PrintJobInfo printJob) {
             RemotePrintSpooler spooler = mWeakSpooler.get();
             if (spooler != null) {
                 final long identity = Binder.clearCallingIdentity();
                 try {
-                    spooler.onPrintJobStateChanged(printJobId, appId);
+                    spooler.onPrintJobStateChanged(printJob);
                 } finally {
                     Binder.restoreCallingIdentity(identity);
                 }
diff --git a/services/java/com/android/server/print/UserState.java b/services/java/com/android/server/print/UserState.java
index e5f5842..72acc53 100644
--- a/services/java/com/android/server/print/UserState.java
+++ b/services/java/com/android/server/print/UserState.java
@@ -33,7 +33,6 @@ import android.os.Looper;
 import android.os.Message;
 import android.os.RemoteCallbackList;
 import android.os.RemoteException;
-import android.os.UserManager;
 import android.print.IPrintClient;
 import android.print.IPrintDocumentAdapter;
 import android.print.IPrintJobStateChangeListener;
@@ -52,6 +51,7 @@ import android.util.ArrayMap;
 import android.util.ArraySet;
 import android.util.Log;
 import android.util.Slog;
+import android.util.SparseArray;
 
 import com.android.internal.R;
 import com.android.internal.os.BackgroundThread;
@@ -62,6 +62,7 @@ import com.android.server.print.RemotePrintSpooler.PrintSpoolerCallbacks;
 import java.io.FileDescriptor;
 import java.io.PrintWriter;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
@@ -93,8 +94,8 @@ final class UserState implements PrintSpoolerCallbacks, PrintServiceCallbacks {
     private final Set<ComponentName> mEnabledServices =
             new ArraySet<ComponentName>();
 
-    private final CreatedPrintJobTracker mCreatedPrintJobTracker =
-            new CreatedPrintJobTracker();
+    private final PrintJobForAppCache mPrintJobForAppCache =
+            new PrintJobForAppCache();
 
     private final Object mLock;
 
@@ -155,23 +156,22 @@ final class UserState implements PrintSpoolerCallbacks, PrintServiceCallbacks {
     public PrintJobInfo print(String printJobName, final IPrintClient client,
             final IPrintDocumentAdapter documentAdapter, PrintAttributes attributes,
             int appId) {
-        PrintJobId printJobId = new PrintJobId();
-
-        // Track this job so we can forget it when the creator dies.
-        if (!mCreatedPrintJobTracker.onPrintJobCreatedLocked(client.asBinder(), printJobId)) {
-            // Not adding a print job means the client is dead - done.
-            return null;
-        }
-
         // Create print job place holder.
         final PrintJobInfo printJob = new PrintJobInfo();
-        printJob.setId(printJobId);
+        printJob.setId(new PrintJobId());
         printJob.setAppId(appId);
         printJob.setLabel(printJobName);
         printJob.setAttributes(attributes);
         printJob.setState(PrintJobInfo.STATE_CREATED);
         printJob.setCopies(1);
 
+        // Track this job so we can forget it when the creator dies.
+        if (!mPrintJobForAppCache.onPrintJobCreated(client.asBinder(), appId,
+                printJob)) {
+            // Not adding a print job means the client is dead - done.
+            return null;
+        }
+
         // Spin the spooler to add the job and show the config UI.
         new AsyncTask<Void, Void, Void>() {
             @Override
@@ -185,10 +185,40 @@ final class UserState implements PrintSpoolerCallbacks, PrintServiceCallbacks {
     }
 
     public List<PrintJobInfo> getPrintJobInfos(int appId) {
-        return mSpooler.getPrintJobInfos(null, PrintJobInfo.STATE_ANY, appId);
+        List<PrintJobInfo> cachedPrintJobs = mPrintJobForAppCache.getPrintJobs(appId);
+        // Note that the print spooler is not storing print jobs that
+        // are in a terminal state as it is non-trivial to properly update
+        // the spooler state for when to forget print jobs in terminal state.
+        // Therefore, we fuse the cached print jobs for running apps (some
+        // jobs are in a terminal state) with the ones that the print
+        // spooler knows about (some jobs are being processed).
+        ArrayMap<PrintJobId, PrintJobInfo> result =
+                new ArrayMap<PrintJobId, PrintJobInfo>();
+
+        // Add the cached print jobs for running apps.
+        final int cachedPrintJobCount = cachedPrintJobs.size();
+        for (int i = 0; i < cachedPrintJobCount; i++) {
+            PrintJobInfo cachedPrintJob = cachedPrintJobs.get(i);
+            result.put(cachedPrintJob.getId(), cachedPrintJob);
+        }
+
+        // Add everything else the spooler knows about.
+        List<PrintJobInfo> printJobs = mSpooler.getPrintJobInfos(null,
+                PrintJobInfo.STATE_ANY, appId);
+        final int printJobCount = printJobs.size();
+        for (int i = 0; i < printJobCount; i++) {
+            PrintJobInfo printJob = printJobs.get(i);
+            result.put(printJob.getId(), printJob);
+        }
+
+        return new ArrayList<PrintJobInfo>(result.values());
     }
 
     public PrintJobInfo getPrintJobInfo(PrintJobId printJobId, int appId) {
+        PrintJobInfo printJob = mPrintJobForAppCache.getPrintJob(printJobId, appId);
+        if (printJob != null) {
+            return printJob;
+        }
         return mSpooler.getPrintJobInfo(printJobId, appId);
     }
 
@@ -398,9 +428,10 @@ final class UserState implements PrintSpoolerCallbacks, PrintServiceCallbacks {
     }
 
     @Override
-    public void onPrintJobStateChanged(PrintJobId printJobId, int appId) {
+    public void onPrintJobStateChanged(PrintJobInfo printJob) {
+        mPrintJobForAppCache.onPrintJobStateChanged(printJob);
         mHandler.obtainMessage(UserStateHandler.MSG_DISPATCH_PRINT_JOB_STATE_CHANGED,
-                appId, 0, printJobId).sendToTarget();
+                printJob.getAppId(), 0, printJob.getId()).sendToTarget();
     }
 
     @Override
@@ -525,6 +556,9 @@ final class UserState implements PrintSpoolerCallbacks, PrintServiceCallbacks {
             pw.println();
         }
 
+        pw.append(prefix).append(tab).append("cached print jobs:").println();
+        mPrintJobForAppCache.dump(pw, prefix + tab + tab);
+
         pw.append(prefix).append(tab).append("discovery mediator:").println();
         if (mPrinterDiscoverySession != null) {
             mPrinterDiscoverySession.dump(pw, prefix + tab + tab);
@@ -1424,34 +1458,19 @@ final class UserState implements PrintSpoolerCallbacks, PrintServiceCallbacks {
         }
     }
 
-    private final class CreatedPrintJobTracker {
-        private final ArrayMap<IBinder, List<PrintJobId>> mCreatedPrintJobs =
-                new ArrayMap<IBinder, List<PrintJobId>>();
+    private final class PrintJobForAppCache {
+        private final SparseArray<List<PrintJobInfo>> mPrintJobsForRunningApp =
+                new SparseArray<List<PrintJobInfo>>();
 
-        public boolean onPrintJobCreatedLocked(final IBinder creator, PrintJobId printJobId) {
+        public boolean onPrintJobCreated(final IBinder creator, final int appId,
+                PrintJobInfo printJob) {
             try {
                 creator.linkToDeath(new DeathRecipient() {
                     @Override
                     public void binderDied() {
                         creator.unlinkToDeath(this, 0);
-                        UserManager userManager = (UserManager) mContext.getSystemService(
-                                Context.USER_SERVICE);
-                        // If the death is a result of the user being removed, then
-                        // do nothing since the spooler data for this user will be
-                        // wiped and we cannot bind to the spooler at this point.
-                        if (userManager.getUserInfo(mUserId) == null) {
-                            return;
-                        }
-                        List<PrintJobId> printJobIds = null;
                         synchronized (mLock) {
-                            printJobIds = mCreatedPrintJobs.remove(creator);
-                            if (printJobIds == null) {
-                                return;
-                            }
-                            printJobIds = new ArrayList<PrintJobId>(printJobIds);
-                        }
-                        if (printJobIds != null) {
-                            mSpooler.forgetPrintJobs(printJobIds);
+                            mPrintJobsForRunningApp.remove(appId);
                         }
                     }
                 }, 0);
@@ -1460,14 +1479,93 @@ final class UserState implements PrintSpoolerCallbacks, PrintServiceCallbacks {
                 return false;
             }
             synchronized (mLock) {
-                List<PrintJobId> printJobIds = mCreatedPrintJobs.get(creator);
-                if (printJobIds == null) {
-                    printJobIds = new ArrayList<PrintJobId>();
-                    mCreatedPrintJobs.put(creator, printJobIds);
+                List<PrintJobInfo> printJobsForApp = mPrintJobsForRunningApp.get(appId);
+                if (printJobsForApp == null) {
+                    printJobsForApp = new ArrayList<PrintJobInfo>();
+                    mPrintJobsForRunningApp.put(appId, printJobsForApp);
                 }
-                printJobIds.add(printJobId);
+                printJobsForApp.add(printJob);
             }
             return true;
         }
+
+        public void onPrintJobStateChanged(PrintJobInfo printJob) {
+            synchronized (mLock) {
+                List<PrintJobInfo> printJobsForApp = mPrintJobsForRunningApp.get(
+                        printJob.getAppId());
+                if (printJobsForApp == null) {
+                    return;
+                }
+                final int printJobCount = printJobsForApp.size();
+                for (int i = 0; i < printJobCount; i++) {
+                    PrintJobInfo oldPrintJob = printJobsForApp.get(i);
+                    if (oldPrintJob.getId().equals(printJob.getId())) {
+                        printJobsForApp.set(i, printJob);
+                    }
+                }
+            }
+        }
+
+        public PrintJobInfo getPrintJob(PrintJobId printJobId, int appId) {
+            synchronized (mLock) {
+                List<PrintJobInfo> printJobsForApp = mPrintJobsForRunningApp.get(appId);
+                if (printJobsForApp == null) {
+                    return null;
+                }
+                final int printJobCount = printJobsForApp.size();
+                for (int i = 0; i < printJobCount; i++) {
+                    PrintJobInfo printJob = printJobsForApp.get(i);
+                    if (printJob.getId().equals(printJobId)) {
+                        return printJob;
+                    }
+                }
+            }
+            return null;
+        }
+
+        public List<PrintJobInfo> getPrintJobs(int appId) {
+            synchronized (mLock) {
+                List<PrintJobInfo> printJobs = null;
+                if (appId == PrintManager.APP_ID_ANY) {
+                    final int bucketCount = mPrintJobsForRunningApp.size();
+                    for (int i = 0; i < bucketCount; i++) {
+                        List<PrintJobInfo> bucket = mPrintJobsForRunningApp.valueAt(i);
+                        if (printJobs == null) {
+                            printJobs = new ArrayList<PrintJobInfo>();
+                        }
+                        printJobs.addAll(bucket);
+                    }
+                } else {
+                    List<PrintJobInfo> bucket = mPrintJobsForRunningApp.get(appId);
+                    if (bucket != null) {
+                        if (printJobs == null) {
+                            printJobs = new ArrayList<PrintJobInfo>();
+                        }
+                        printJobs.addAll(bucket);
+                    }
+                }
+                if (printJobs != null) {
+                    return printJobs;
+                }
+                return Collections.emptyList();
+            }
+        }
+
+        public void dump(PrintWriter pw, String prefix) {
+            synchronized (mLock) {
+                String tab = "  ";
+                final int bucketCount = mPrintJobsForRunningApp.size();
+                for (int i = 0; i < bucketCount; i++) {
+                    final int appId = mPrintJobsForRunningApp.keyAt(i);
+                    pw.append(prefix).append("appId=" + appId).append(':').println();
+                    List<PrintJobInfo> bucket = mPrintJobsForRunningApp.valueAt(i);
+                    final int printJobCount = bucket.size();
+                    for (int j = 0; j < printJobCount; j++) {
+                        PrintJobInfo printJob = bucket.get(j);
+                        pw.append(prefix).append(tab).append(printJob.toString()).println();
+                    }
+                }
+            }
+        }
     }
 }