Count error document to complete adding documents to the database.

Previously DocumentLoader#LoaderTask had a counter to count loaded
documents and completes adding documents to the database.  However it
does not count documents where a MTP device returns an error for
getObjectInfo. The CL fixes the problem to ensure we complete documents
loading.

BUG=27729653

Change-Id: I696eac790a6535f1bd7a1855dc2d6f932e32eae5

4 files changed, 167 insertions, 152 deletions

diff --git a/packages/MtpDocumentsProvider/src/com/android/mtp/DocumentLoader.java b/packages/MtpDocumentsProvider/src/com/android/mtp/DocumentLoader.java
index 0705214b6abd..c42cf20c14b0 100644
--- a/packages/MtpDocumentsProvider/src/com/android/mtp/DocumentLoader.java
+++ b/packages/MtpDocumentsProvider/src/com/android/mtp/DocumentLoader.java
@@ -81,11 +81,9 @@ class DocumentLoader implements AutoCloseable {
             // 3. startAddingChildDocuemnts.
             // 4. stopAddingChildDocuments - It removes the new document added at the step 2,
             //     because it is not updated between start/stopAddingChildDocuments.
-            task = LoaderTask.create(mDatabase, mMtpManager, mDevice.operationsSupported, parent);
-            task.fillDocuments(loadDocuments(
-                    mMtpManager,
-                    parent.mDeviceId,
-                    task.getUnloadedObjectHandles(NUM_INITIAL_ENTRIES)));
+            task = new LoaderTask(mMtpManager, mDatabase, mDevice.operationsSupported, parent);
+            task.loadObjectHandles();
+            task.loadObjectInfoList(NUM_INITIAL_ENTRIES);
         } else {
             // Once remove the existing task in order to add it to the head of the list.
             mTaskList.remove(task);
@@ -130,15 +128,11 @@ class DocumentLoader implements AutoCloseable {
                 Preconditions.checkState(existingTask.getState() != LoaderTask.STATE_LOADING);
                 mTaskList.remove(existingTask);
             }
-            try {
-                final LoaderTask newTask = LoaderTask.create(
-                        mDatabase, mMtpManager, mDevice.operationsSupported, identifier);
-                mTaskList.addFirst(newTask);
-                return newTask;
-            } catch (IOException exception) {
-                Log.e(MtpDocumentsProvider.TAG, "Failed to create a task for mapping", exception);
-                // Continue to release the background thread.
-            }
+            final LoaderTask newTask = new LoaderTask(
+                    mMtpManager, mDatabase, mDevice.operationsSupported, identifier);
+            newTask.loadObjectHandles();
+            mTaskList.addFirst(newTask);
+            return newTask;
         }
 
         mBackgroundThread = null;
@@ -170,24 +164,6 @@ class DocumentLoader implements AutoCloseable {
     }
 
     /**
-     * Helper method to loads multiple object info.
-     */
-    private static MtpObjectInfo[] loadDocuments(MtpManager manager, int deviceId, int[] handles)
-            throws IOException {
-        final ArrayList<MtpObjectInfo> objects = new ArrayList<>();
-        for (int i = 0; i < handles.length; i++) {
-            final MtpObjectInfo info = manager.getObjectInfo(deviceId, handles[i]);
-            if (info == null) {
-                Log.e(MtpDocumentsProvider.TAG,
-                        "Failed to obtain object info handle=" + handles[i]);
-                continue;
-            }
-            objects.add(info);
-        }
-        return objects.toArray(new MtpObjectInfo[objects.size()]);
-    }
-
-    /**
      * Background thread to fetch object info.
      */
     private class BackgroundLoaderThread extends Thread {
@@ -203,21 +179,13 @@ class DocumentLoader implements AutoCloseable {
                 if (task == null) {
                     return;
                 }
-                try {
-                    final MtpObjectInfo[] objectInfos = loadDocuments(
-                            mMtpManager,
-                            task.mIdentifier.mDeviceId,
-                            task.getUnloadedObjectHandles(NUM_LOADING_ENTRIES));
-                    task.fillDocuments(objectInfos);
-                    final boolean shouldNotify =
-                            task.mLastNotified.getTime() <
-                            new Date().getTime() - NOTIFY_PERIOD_MS ||
-                            task.getState() != LoaderTask.STATE_LOADING;
-                    if (shouldNotify) {
-                        task.notify(mResolver);
-                    }
-                } catch (IOException exception) {
-                    task.setError(exception);
+                task.loadObjectInfoList(NUM_LOADING_ENTRIES);
+                final boolean shouldNotify =
+                        task.mLastNotified.getTime() <
+                        new Date().getTime() - NOTIFY_PERIOD_MS ||
+                        task.getState() != LoaderTask.STATE_LOADING;
+                if (shouldNotify) {
+                    task.notify(mResolver);
                 }
             }
         }
@@ -271,43 +239,66 @@ class DocumentLoader implements AutoCloseable {
      * Each task is responsible for fetching child documents for the given parent document.
      */
     private static class LoaderTask {
-        static final int STATE_LOADING = 0;
-        static final int STATE_COMPLETED = 1;
-        static final int STATE_ERROR = 2;
+        static final int STATE_START = 0;
+        static final int STATE_LOADING = 1;
+        static final int STATE_COMPLETED = 2;
+        static final int STATE_ERROR = 3;
 
+        final MtpManager mManager;
         final MtpDatabase mDatabase;
         final int[] mOperationsSupported;
         final Identifier mIdentifier;
-        final int[] mObjectHandles;
+        int[] mObjectHandles;
+        int mState;
         Date mLastNotified;
-        int mNumLoaded;
-        Exception mError;
+        int mPosition;
+        IOException mError;
 
-        LoaderTask(MtpDatabase database, int[] operationsSupported, Identifier identifier,
-                int[] objectHandles) {
+        LoaderTask(MtpManager manager, MtpDatabase database, int[] operationsSupported,
+                Identifier identifier) {
             Preconditions.checkNotNull(operationsSupported);
-            Preconditions.checkNotNull(objectHandles);
+            mManager = manager;
             mDatabase = database;
             mOperationsSupported = operationsSupported;
             mIdentifier = identifier;
-            mObjectHandles = objectHandles;
-            mNumLoaded = 0;
+            mObjectHandles = null;
+            mState = STATE_START;
+            mPosition = 0;
             mLastNotified = new Date();
         }
 
+        synchronized void loadObjectHandles() {
+            assert mState == STATE_START;
+            int parentHandle = mIdentifier.mObjectHandle;
+            // Need to pass the special value MtpManager.OBJECT_HANDLE_ROOT_CHILDREN to
+            // getObjectHandles if we would like to obtain children under the root.
+            if (mIdentifier.mDocumentType == MtpDatabaseConstants.DOCUMENT_TYPE_STORAGE) {
+                parentHandle = MtpManager.OBJECT_HANDLE_ROOT_CHILDREN;
+            }
+            try {
+                mObjectHandles = mManager.getObjectHandles(
+                        mIdentifier.mDeviceId, mIdentifier.mStorageId, parentHandle);
+                mState = STATE_LOADING;
+            } catch (IOException error) {
+                mError = error;
+                mState = STATE_ERROR;
+            }
+        }
+
         /**
          * Returns a cursor that traverses the child document of the parent document handled by the
          * task.
          * The returned task may have a EXTRA_LOADING flag.
          */
-        Cursor createCursor(ContentResolver resolver, String[] columnNames) throws IOException {
+        synchronized Cursor createCursor(ContentResolver resolver, String[] columnNames)
+                throws IOException {
             final Bundle extras = new Bundle();
             switch (getState()) {
                 case STATE_LOADING:
                     extras.putBoolean(DocumentsContract.EXTRA_LOADING, true);
                     break;
                 case STATE_ERROR:
-                    throw new IOException(mError);
+                    throw mError;
             }
 
             final Cursor cursor =
@@ -319,26 +310,67 @@ class DocumentLoader implements AutoCloseable {
         }
 
         /**
-         * Returns a state of the task.
+         * Stores object information into database.
          */
-        int getState() {
-            if (mError != null) {
-                return STATE_ERROR;
-            } else if (mNumLoaded == mObjectHandles.length) {
-                return STATE_COMPLETED;
-            } else {
-                return STATE_LOADING;
+        void loadObjectInfoList(int count) {
+            synchronized (this) {
+                if (mState != STATE_LOADING) {
+                    return;
+                }
+                if (mPosition == 0) {
+                    try{
+                        mDatabase.getMapper().startAddingDocuments(mIdentifier.mDocumentId);
+                    } catch (FileNotFoundException error) {
+                        mError = error;
+                        mState = STATE_ERROR;
+                        return;
+                    }
+                }
+            }
+            final ArrayList<MtpObjectInfo> infoList = new ArrayList<>();
+            for (int chunkEnd = mPosition + count;
+                    mPosition < mObjectHandles.length && mPosition < chunkEnd;
+                    mPosition++) {
+                try {
+                    infoList.add(mManager.getObjectInfo(
+                            mIdentifier.mDeviceId, mObjectHandles[mPosition]));
+                } catch (IOException error) {
+                    Log.e(MtpDocumentsProvider.TAG, "Failed to load object info", error);
+                }
+            }
+            synchronized (this) {
+                try {
+                    mDatabase.getMapper().putChildDocuments(
+                            mIdentifier.mDeviceId,
+                            mIdentifier.mDocumentId,
+                            mOperationsSupported,
+                            infoList.toArray(new MtpObjectInfo[infoList.size()]));
+                } catch (FileNotFoundException error) {
+                    // Looks like the parent document information is removed.
+                    // Adding documents has already cancelled in Mapper so we don't need to invoke
+                    // stopAddingDocuments.
+                    mError = error;
+                    mState = STATE_ERROR;
+                    return;
+                }
+                if (mPosition >= mObjectHandles.length) {
+                    try{
+                        mDatabase.getMapper().stopAddingDocuments(mIdentifier.mDocumentId);
+                        mState = STATE_COMPLETED;
+                    } catch (FileNotFoundException error) {
+                        mError = error;
+                        mState = STATE_ERROR;
+                        return;
+                    }
+                }
             }
         }
 
         /**
-         * Obtains object handles that have not been loaded yet.
+         * Returns a state of the task.
          */
-        int[] getUnloadedObjectHandles(int count) {
-            return Arrays.copyOfRange(
-                    mObjectHandles,
-                    mNumLoaded,
-                    Math.min(mNumLoaded + count, mObjectHandles.length));
+        int getState() {
+            return mState;
         }
 
         /**
@@ -349,69 +381,9 @@ class DocumentLoader implements AutoCloseable {
             mLastNotified = new Date();
         }
 
-        /**
-         * Stores object information into database.
-         */
-        void fillDocuments(MtpObjectInfo[] objectInfoList) {
-            if (objectInfoList.length == 0 || getState() != STATE_LOADING) {
-                return;
-            }
-            try{
-                if (mNumLoaded == 0) {
-                    mDatabase.getMapper().startAddingDocuments(mIdentifier.mDocumentId);
-                }
-                mDatabase.getMapper().putChildDocuments(
-                        mIdentifier.mDeviceId, mIdentifier.mDocumentId, mOperationsSupported,
-                        objectInfoList);
-                mNumLoaded += objectInfoList.length;
-                if (getState() != STATE_LOADING) {
-                    mDatabase.getMapper().stopAddingDocuments(mIdentifier.mDocumentId);
-                }
-            } catch (FileNotFoundException exception) {
-                setErrorInternal(exception);
-            }
-        }
-
-        /**
-         * Marks the loading task as error.
-         */
-        void setError(Exception error) {
-            final int lastState = getState();
-            setErrorInternal(error);
-            if (lastState == STATE_LOADING) {
-                try {
-                    mDatabase.getMapper().stopAddingDocuments(mIdentifier.mDocumentId);
-                } catch (FileNotFoundException exception) {
-                    setErrorInternal(exception);
-                }
-            }
-        }
-
-        private void setErrorInternal(Exception error) {
-            Log.e(MtpDocumentsProvider.TAG, "Error in DocumentLoader thread", error);
-            mError = error;
-            mNumLoaded = 0;
-        }
-
         private Uri createUri() {
             return DocumentsContract.buildChildDocumentsUri(
                     MtpDocumentsProvider.AUTHORITY, mIdentifier.mDocumentId);
         }
-
-        /**
-         * Creates a LoaderTask that loads children of the given document.
-         */
-        static LoaderTask create(MtpDatabase database, MtpManager manager,
-                int[] operationsSupported, Identifier parent)
-                throws IOException {
-            int parentHandle = parent.mObjectHandle;
-            // Need to pass the special value MtpManager.OBJECT_HANDLE_ROOT_CHILDREN to
-            // getObjectHandles if we would like to obtain children under the root.
-            if (parent.mDocumentType == MtpDatabaseConstants.DOCUMENT_TYPE_STORAGE) {
-                parentHandle = MtpManager.OBJECT_HANDLE_ROOT_CHILDREN;
-            }
-            return new LoaderTask(database, operationsSupported, parent, manager.getObjectHandles(
-                    parent.mDeviceId, parent.mStorageId, parentHandle));
-        }
     }
 }
diff --git a/packages/MtpDocumentsProvider/src/com/android/mtp/MtpManager.java b/packages/MtpDocumentsProvider/src/com/android/mtp/MtpManager.java
index 6fb2a786be80..766cc88ff2b1 100644
--- a/packages/MtpDocumentsProvider/src/com/android/mtp/MtpManager.java
+++ b/packages/MtpDocumentsProvider/src/com/android/mtp/MtpManager.java
@@ -130,11 +130,14 @@ class MtpManager {
         return devices.toArray(new MtpDeviceRecord[devices.size()]);
     }
 
-    MtpObjectInfo getObjectInfo(int deviceId, int objectHandle)
-            throws IOException {
+    MtpObjectInfo getObjectInfo(int deviceId, int objectHandle) throws IOException {
         final MtpDevice device = getDevice(deviceId);
         synchronized (device) {
-            return device.getObjectInfo(objectHandle);
+            final MtpObjectInfo info = device.getObjectInfo(objectHandle);
+            if (info == null) {
+                throw new IOException("Failed to get object info: " + objectHandle);
+            }
+            return info;
         }
     }
 
diff --git a/packages/MtpDocumentsProvider/tests/src/com/android/mtp/DocumentLoaderTest.java b/packages/MtpDocumentsProvider/tests/src/com/android/mtp/DocumentLoaderTest.java
index db25421afab0..45f89e4e1ffd 100644
--- a/packages/MtpDocumentsProvider/tests/src/com/android/mtp/DocumentLoaderTest.java
+++ b/packages/MtpDocumentsProvider/tests/src/com/android/mtp/DocumentLoaderTest.java
@@ -55,13 +55,6 @@ public class DocumentLoaderTest extends AndroidTestCase {
 
         mManager = new BlockableTestMtpManager(getContext());
         mResolver = new TestContentResolver();
-        mLoader = new DocumentLoader(
-                new MtpDeviceRecord(
-                        0, "Device", "Key", true, new MtpRoot[0],
-                        TestUtil.OPERATIONS_SUPPORTED, new int[0]),
-                mManager,
-                mResolver,
-                mDatabase);
     }
 
     @Override
@@ -71,6 +64,8 @@ public class DocumentLoaderTest extends AndroidTestCase {
     }
 
     public void testBasic() throws Exception {
+        setUpLoader();
+
         final Uri uri = DocumentsContract.buildChildDocumentsUri(
                 MtpDocumentsProvider.AUTHORITY, mParentIdentifier.mDocumentId);
         setUpDocument(mManager, 40);
@@ -107,6 +102,55 @@ public class DocumentLoaderTest extends AndroidTestCase {
         assertEquals(2, mResolver.getChangeCount(uri));
     }
 
+    public void testError_GetObjectHandles() throws Exception {
+        mManager = new BlockableTestMtpManager(getContext()) {
+            @Override
+            int[] getObjectHandles(int deviceId, int storageId, int parentObjectHandle)
+                    throws IOException {
+                throw new IOException();
+            }
+        };
+        setUpLoader();
+        mManager.setObjectHandles(0, 0, MtpManager.OBJECT_HANDLE_ROOT_CHILDREN, null);
+        try {
+            try (final Cursor cursor = mLoader.queryChildDocuments(
+                    MtpDocumentsProvider.DEFAULT_DOCUMENT_PROJECTION, mParentIdentifier)) {}
+            fail();
+        } catch (IOException exception) {
+            // Expect exception.
+        }
+    }
+
+    public void testError_GetObjectInfo() throws Exception {
+        mManager = new BlockableTestMtpManager(getContext()) {
+            @Override
+            MtpObjectInfo getObjectInfo(int deviceId, int objectHandle) throws IOException {
+                if (objectHandle == DocumentLoader.NUM_INITIAL_ENTRIES) {
+                    throw new IOException();
+                } else {
+                    return super.getObjectInfo(deviceId, objectHandle);
+                }
+            }
+        };
+        setUpLoader();
+        setUpDocument(mManager, DocumentLoader.NUM_INITIAL_ENTRIES);
+        try (final Cursor cursor = mLoader.queryChildDocuments(
+                MtpDocumentsProvider.DEFAULT_DOCUMENT_PROJECTION, mParentIdentifier)) {
+            // Even if MtpManager returns an error for a document, loading must complete.
+            assertFalse(cursor.getExtras().getBoolean(DocumentsContract.EXTRA_LOADING));
+        }
+    }
+
+    private void setUpLoader() {
+        mLoader = new DocumentLoader(
+                new MtpDeviceRecord(
+                        0, "Device", "Key", true, new MtpRoot[0],
+                        TestUtil.OPERATIONS_SUPPORTED, new int[0]),
+                mManager,
+                mResolver,
+                mDatabase);
+    }
+
     private void setUpDocument(TestMtpManager manager, int count) {
         int[] childDocuments = new int[count];
         for (int i = 0; i < childDocuments.length; i++) {
diff --git a/packages/MtpDocumentsProvider/tests/src/com/android/mtp/MtpDocumentsProviderTest.java b/packages/MtpDocumentsProvider/tests/src/com/android/mtp/MtpDocumentsProviderTest.java
index 9c1880a246a4..0cfe1e6c95fa 100644
--- a/packages/MtpDocumentsProvider/tests/src/com/android/mtp/MtpDocumentsProviderTest.java
+++ b/packages/MtpDocumentsProvider/tests/src/com/android/mtp/MtpDocumentsProviderTest.java
@@ -419,20 +419,16 @@ public class MtpDocumentsProviderTest extends AndroidTestCase {
         try {
             mProvider.queryChildDocuments("1", null, null);
             fail();
-        } catch (Throwable error) {
-            assertTrue(error instanceof FileNotFoundException);
-        }
+        } catch (FileNotFoundException error) {}
     }
 
     public void testQueryChildDocuments_documentError() throws Exception {
         setupProvider(MtpDatabaseConstants.FLAG_DATABASE_IN_MEMORY);
         setupRoots(0, new MtpRoot[] { new MtpRoot(0, 0, "Storage", 1000, 1000, "") });
         mMtpManager.setObjectHandles(0, 0, -1, new int[] { 1 });
-        try {
-            mProvider.queryChildDocuments("1", null, null);
-            fail();
-        } catch (Throwable error) {
-            assertTrue(error instanceof FileNotFoundException);
+        try (final Cursor cursor = mProvider.queryChildDocuments("1", null, null)) {
+            assertEquals(0, cursor.getCount());
+            assertFalse(cursor.getExtras().getBoolean(DocumentsContract.EXTRA_LOADING));
         }
     }