From 76be46f4d9314fd7daca0985a0a7e02126d85975 Mon Sep 17 00:00:00 2001 From: Daichi Hirono Date: Fri, 8 Apr 2016 09:48:02 +0900 Subject: [PATCH] Fix crash when deleting multiple files. When deleting files, MtpDocumentsProvider clears LoadingTask in DocumentsLoader to update directory contents list. Previously it can clear ongoing task, and it skips calling Mapper#stopAddingDocuments. Since Mapper#startAddingDocuments and Mapper#stopAddingDocuments must be called 1 to 1, it causes precondition check failure at the next call of Mapper#startAddingDocuments. Change-Id: I23e2b117da826297e45404be4db4cc29f96e5510 Fix: 28076320 --- .../src/com/android/mtp/DocumentLoader.java | 62 ++++++++++--------- .../src/com/android/mtp/Mapper.java | 35 +++++++++++ .../com/android/mtp/MtpDocumentsProvider.java | 4 +- .../com/android/mtp/DocumentLoaderTest.java | 29 +++++++++ 4 files changed, 100 insertions(+), 30 deletions(-) diff --git a/packages/MtpDocumentsProvider/src/com/android/mtp/DocumentLoader.java b/packages/MtpDocumentsProvider/src/com/android/mtp/DocumentLoader.java index 246b95dec2eb9..329afdd4f17cd 100644 --- a/packages/MtpDocumentsProvider/src/com/android/mtp/DocumentLoader.java +++ b/packages/MtpDocumentsProvider/src/com/android/mtp/DocumentLoader.java @@ -33,7 +33,6 @@ import com.android.internal.util.Preconditions; import java.io.FileNotFoundException; import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.Date; import java.util.LinkedList; @@ -118,9 +117,10 @@ class DocumentLoader implements AutoCloseable { synchronized @Nullable LoaderTask getNextTaskOrReleaseBackgroundThread() { Preconditions.checkState(mBackgroundThread != null); - final LoaderTask task = mTaskList.findRunningTask(); - if (task != null) { - return task; + for (final LoaderTask task : mTaskList) { + if (task.getState() == LoaderTask.STATE_LOADING) { + return task; + } } final Identifier identifier = mDatabase.getUnmappedDocumentsParent(mDevice.deviceId); @@ -161,8 +161,21 @@ class DocumentLoader implements AutoCloseable { mTaskList.clearCompletedTasks(); } - synchronized void clearTask(Identifier parentIdentifier) { - mTaskList.clearTask(parentIdentifier); + /** + * Cancels the task for |parentIdentifier|. + * + * Task is removed from the cached list and it will create new task when |parentIdentifier|'s + * children are queried next. + */ + void cancelTask(Identifier parentIdentifier) { + final LoaderTask task; + synchronized (this) { + task = mTaskList.findTask(parentIdentifier); + } + if (task != null) { + task.cancel(); + mTaskList.remove(task); + } } /** @@ -205,14 +218,6 @@ class DocumentLoader implements AutoCloseable { return null; } - LoaderTask findRunningTask() { - for (int i = 0; i < size(); i++) { - if (get(i).getState() == LoaderTask.STATE_LOADING) - return get(i); - } - return null; - } - void clearCompletedTasks() { int i = 0; while (i < size()) { @@ -223,17 +228,6 @@ class DocumentLoader implements AutoCloseable { } } } - - void clearTask(Identifier parentIdentifier) { - for (int i = 0; i < size(); i++) { - final LoaderTask task = get(i); - if (task.mIdentifier.mDeviceId == parentIdentifier.mDeviceId && - task.mIdentifier.mObjectHandle == parentIdentifier.mObjectHandle) { - remove(i); - return; - } - } - } } /** @@ -245,6 +239,7 @@ class DocumentLoader implements AutoCloseable { static final int STATE_LOADING = 1; static final int STATE_COMPLETED = 2; static final int STATE_ERROR = 3; + static final int STATE_CANCELLED = 4; final MtpManager mManager; final MtpDatabase mDatabase; @@ -272,6 +267,7 @@ class DocumentLoader implements AutoCloseable { synchronized void loadObjectHandles() { assert mState == STATE_START; + mPosition = 0; 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. @@ -303,12 +299,10 @@ class DocumentLoader implements AutoCloseable { case STATE_ERROR: throw mError; } - final Cursor cursor = mDatabase.queryChildDocuments(columnNames, mIdentifier.mDocumentId); + cursor.setExtras(extras); cursor.setNotificationUri(resolver, createUri()); - cursor.respond(extras); - return cursor; } @@ -374,6 +368,10 @@ class DocumentLoader implements AutoCloseable { } } synchronized (this) { + // Check if the task is cancelled or not. + if (mState != STATE_LOADING) { + return; + } try { mDatabase.getMapper().putChildDocuments( mIdentifier.mDeviceId, @@ -402,6 +400,14 @@ class DocumentLoader implements AutoCloseable { } } + /** + * Cancels the task. + */ + synchronized void cancel() { + mDatabase.getMapper().cancelAddingDocuments(mIdentifier.mDocumentId); + mState = STATE_CANCELLED; + } + /** * Returns a state of the task. */ diff --git a/packages/MtpDocumentsProvider/src/com/android/mtp/Mapper.java b/packages/MtpDocumentsProvider/src/com/android/mtp/Mapper.java index adc71aecc3495..63f18f324d693 100644 --- a/packages/MtpDocumentsProvider/src/com/android/mtp/Mapper.java +++ b/packages/MtpDocumentsProvider/src/com/android/mtp/Mapper.java @@ -362,6 +362,41 @@ class Mapper { } } + /** + * Cancels adding documents. + * @param parentId + */ + void cancelAddingDocuments(@Nullable String parentId) { + final String selection; + final String[] args; + if (parentId != null) { + selection = COLUMN_PARENT_DOCUMENT_ID + " = ?"; + args = strings(parentId); + } else { + selection = COLUMN_PARENT_DOCUMENT_ID + " IS NULL"; + args = EMPTY_ARGS; + } + + final SQLiteDatabase database = mDatabase.getSQLiteDatabase(); + database.beginTransaction(); + try { + if (!mInMappingIds.contains(parentId)) { + return; + } + mInMappingIds.remove(parentId); + final ContentValues values = new ContentValues(); + values.put(COLUMN_ROW_STATE, ROW_STATE_VALID); + mDatabase.getSQLiteDatabase().update( + TABLE_DOCUMENTS, + values, + selection + " AND " + COLUMN_ROW_STATE + " = ?", + DatabaseUtils.appendSelectionArgs(args, strings(ROW_STATE_INVALIDATED))); + database.setTransactionSuccessful(); + } finally { + database.endTransaction(); + } + } + /** * Queries candidate for each mappingKey, and returns the first cursor that includes a * candidate. diff --git a/packages/MtpDocumentsProvider/src/com/android/mtp/MtpDocumentsProvider.java b/packages/MtpDocumentsProvider/src/com/android/mtp/MtpDocumentsProvider.java index 50781bf2c9ca1..1823711acd597 100644 --- a/packages/MtpDocumentsProvider/src/com/android/mtp/MtpDocumentsProvider.java +++ b/packages/MtpDocumentsProvider/src/com/android/mtp/MtpDocumentsProvider.java @@ -308,7 +308,7 @@ public class MtpDocumentsProvider extends DocumentsProvider { final Identifier parentIdentifier = mDatabase.getParentIdentifier(documentId); mMtpManager.deleteDocument(identifier.mDeviceId, identifier.mObjectHandle); mDatabase.deleteDocument(documentId); - getDocumentLoader(parentIdentifier).clearTask(parentIdentifier); + getDocumentLoader(parentIdentifier).cancelTask(parentIdentifier); notifyChildDocumentsChange(parentIdentifier.mDocumentId); if (parentIdentifier.mDocumentType == MtpDatabaseConstants.DOCUMENT_TYPE_STORAGE) { // If the parent is storage, the object might be appeared as child of device because @@ -402,7 +402,7 @@ public class MtpDocumentsProvider extends DocumentsProvider { final String documentId = mDatabase.putNewDocument( parentId.mDeviceId, parentDocumentId, record.operationsSupported, infoWithHandle, 0l); - getDocumentLoader(parentId).clearTask(parentId); + getDocumentLoader(parentId).cancelTask(parentId); notifyChildDocumentsChange(parentDocumentId); return documentId; } catch (FileNotFoundException | RuntimeException error) { diff --git a/packages/MtpDocumentsProvider/tests/src/com/android/mtp/DocumentLoaderTest.java b/packages/MtpDocumentsProvider/tests/src/com/android/mtp/DocumentLoaderTest.java index 45f89e4e1ffd1..60dd7e16a1d9d 100644 --- a/packages/MtpDocumentsProvider/tests/src/com/android/mtp/DocumentLoaderTest.java +++ b/packages/MtpDocumentsProvider/tests/src/com/android/mtp/DocumentLoaderTest.java @@ -21,6 +21,7 @@ import android.database.Cursor; import android.mtp.MtpObjectInfo; import android.net.Uri; import android.provider.DocumentsContract; +import android.provider.DocumentsContract.Document; import android.test.AndroidTestCase; import android.test.suitebuilder.annotation.MediumTest; @@ -28,6 +29,7 @@ import java.io.IOException; import java.util.HashMap; import java.util.Map; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeoutException; @MediumTest public class DocumentLoaderTest extends AndroidTestCase { @@ -141,6 +143,33 @@ public class DocumentLoaderTest extends AndroidTestCase { } } + public void testCancelTask() throws IOException, InterruptedException { + setUpDocument(mManager, + DocumentLoader.NUM_INITIAL_ENTRIES + DocumentLoader.NUM_LOADING_ENTRIES + 1); + + // Block the first iteration in the background thread. + mManager.blockDocument( + 0, DocumentLoader.NUM_INITIAL_ENTRIES + 1); + setUpLoader(); + try (final Cursor cursor = mLoader.queryChildDocuments( + MtpDocumentsProvider.DEFAULT_DOCUMENT_PROJECTION, mParentIdentifier)) { + assertTrue(cursor.getExtras().getBoolean(DocumentsContract.EXTRA_LOADING)); + } + Thread.sleep(DocumentLoader.NOTIFY_PERIOD_MS); + + // Clear task while the first iteration is being blocked. + mManager.unblockDocument( + 0, DocumentLoader.NUM_INITIAL_ENTRIES + 1); + mLoader.cancelTask(mParentIdentifier); + + Thread.sleep(DocumentLoader.NOTIFY_PERIOD_MS * 2); + + // Check if it's OK to query invalidated task. + try (final Cursor cursor = mLoader.queryChildDocuments( + MtpDocumentsProvider.DEFAULT_DOCUMENT_PROJECTION, mParentIdentifier)) { + } + } + private void setUpLoader() { mLoader = new DocumentLoader( new MtpDeviceRecord(