From 6310dfd04e0d3561d1ef58121046e6255eac8c87 Mon Sep 17 00:00:00 2001 From: linyuh Date: Thu, 28 Jun 2018 10:14:11 -0700 Subject: Also ignore IllegalStateException thrown by SQLiteClosable when coalescing fails. Test: Manual PiperOrigin-RevId: 202499434 Change-Id: Ie41eeb782072d82c5613b44be99649f43807498d --- .../android/dialer/calllog/database/Coalescer.java | 84 +++++++++++++++++----- .../dialer/calllog/ui/NewCallLogFragment.java | 19 +++-- 2 files changed, 73 insertions(+), 30 deletions(-) (limited to 'java/com/android/dialer/calllog') diff --git a/java/com/android/dialer/calllog/database/Coalescer.java b/java/com/android/dialer/calllog/database/Coalescer.java index 2ad9f9a97..fd751e767 100644 --- a/java/com/android/dialer/calllog/database/Coalescer.java +++ b/java/com/android/dialer/calllog/database/Coalescer.java @@ -16,6 +16,7 @@ package com.android.dialer.calllog.database; import android.database.Cursor; +import android.database.StaleDataException; import android.provider.CallLog.Calls; import android.support.annotation.NonNull; import android.support.annotation.WorkerThread; @@ -85,34 +86,72 @@ public class Coalescer { @WorkerThread @NonNull private ImmutableList coalesceInternal( - Cursor allAnnotatedCallLogRowsSortedByTimestampDesc) { + Cursor allAnnotatedCallLogRowsSortedByTimestampDesc) throws ExpectedCoalescerException { Assert.isWorkerThread(); - if (!allAnnotatedCallLogRowsSortedByTimestampDesc.moveToFirst()) { - return ImmutableList.of(); - } - ImmutableList.Builder coalescedRowListBuilder = new ImmutableList.Builder<>(); - RowCombiner rowCombiner = new RowCombiner(allAnnotatedCallLogRowsSortedByTimestampDesc); - rowCombiner.startNewGroup(); + try { + if (!allAnnotatedCallLogRowsSortedByTimestampDesc.moveToFirst()) { + return ImmutableList.of(); + } - long coalescedRowId = 0; - do { - boolean isRowMerged = rowCombiner.mergeRow(allAnnotatedCallLogRowsSortedByTimestampDesc); + RowCombiner rowCombiner = new RowCombiner(allAnnotatedCallLogRowsSortedByTimestampDesc); + rowCombiner.startNewGroup(); - if (isRowMerged) { - allAnnotatedCallLogRowsSortedByTimestampDesc.moveToNext(); - } + long coalescedRowId = 0; + do { + boolean isRowMerged = rowCombiner.mergeRow(allAnnotatedCallLogRowsSortedByTimestampDesc); + + if (isRowMerged) { + allAnnotatedCallLogRowsSortedByTimestampDesc.moveToNext(); + } + + if (!isRowMerged || allAnnotatedCallLogRowsSortedByTimestampDesc.isAfterLast()) { + coalescedRowListBuilder.add( + rowCombiner.combine().toBuilder().setId(coalescedRowId++).build()); + rowCombiner.startNewGroup(); + } + } while (!allAnnotatedCallLogRowsSortedByTimestampDesc.isAfterLast()); + + return coalescedRowListBuilder.build(); - if (!isRowMerged || allAnnotatedCallLogRowsSortedByTimestampDesc.isAfterLast()) { - coalescedRowListBuilder.add( - rowCombiner.combine().toBuilder().setId(coalescedRowId++).build()); - rowCombiner.startNewGroup(); + } catch (Exception exception) { + // Coalescing can fail if cursor "allAnnotatedCallLogRowsSortedByTimestampDesc" is closed by + // its loader while the work is still in progress. + // + // This can happen when the loader restarts and finishes loading data before the coalescing + // work is completed. + // + // This kind of failure doesn't have to crash the app as coalescing will be restarted on the + // latest data obtained by the loader. Therefore, we inspect the exception here and throw an + // ExpectedCoalescerException if it is the case described above. + // + // The type of expected exception depends on whether AbstractWindowedCursor#checkPosition() is + // called when the cursor is closed. + // (1) If it is called before the cursor is closed, we will get IllegalStateException thrown + // by SQLiteClosable when it attempts to acquire a reference to the database. + // (2) Otherwise, we will get StaleDataException thrown by AbstractWindowedCursor's + // checkPosition() method. + // + // Note that it would be more accurate to inspect the stack trace to locate the origin of the + // exception. However, according to the documentation on Throwable#getStackTrace, "some + // virtual machines may, under some circumstances, omit one or more stack frames from the + // stack trace". "In the extreme case, a virtual machine that has no stack trace information + // concerning this throwable is permitted to return a zero-length array from this method." + // Therefore, the best we can do is to inspect the message in the exception. + // TODO(linyuh): try to avoid the expected failure. + String message = exception.getMessage(); + if (message != null + && ((exception instanceof StaleDataException + && message.startsWith("Attempting to access a closed CursorWindow")) + || (exception instanceof IllegalStateException + && message.startsWith("attempt to re-open an already-closed object")))) { + throw new ExpectedCoalescerException(exception); } - } while (!allAnnotatedCallLogRowsSortedByTimestampDesc.isAfterLast()); - return coalescedRowListBuilder.build(); + throw exception; + } } /** Combines rows from {@link AnnotatedCallLog} into a {@link CoalescedRow}. */ @@ -337,4 +376,11 @@ public class Coalescer { return dialerPhoneNumberUtil.isMatch(groupPhoneNumber, rowPhoneNumber); } } + + /** A checked exception thrown when expected failure happens when coalescing is in progress. */ + public static final class ExpectedCoalescerException extends Exception { + ExpectedCoalescerException(Throwable throwable) { + super("Expected coalescing exception", throwable); + } + } } diff --git a/java/com/android/dialer/calllog/ui/NewCallLogFragment.java b/java/com/android/dialer/calllog/ui/NewCallLogFragment.java index 4141fe723..5e72a1af3 100644 --- a/java/com/android/dialer/calllog/ui/NewCallLogFragment.java +++ b/java/com/android/dialer/calllog/ui/NewCallLogFragment.java @@ -17,7 +17,6 @@ package com.android.dialer.calllog.ui; import android.app.Activity; import android.database.Cursor; -import android.database.StaleDataException; import android.os.Bundle; import android.support.annotation.Nullable; import android.support.annotation.VisibleForTesting; @@ -34,6 +33,7 @@ import android.view.ViewGroup; import com.android.dialer.calllog.CallLogComponent; import com.android.dialer.calllog.RefreshAnnotatedCallLogReceiver; import com.android.dialer.calllog.database.CallLogDatabaseComponent; +import com.android.dialer.calllog.database.Coalescer; import com.android.dialer.calllog.model.CoalescedRow; import com.android.dialer.common.Assert; import com.android.dialer.common.LogUtil; @@ -337,16 +337,13 @@ public final class NewCallLogFragment extends Fragment implements LoaderCallback } }, throwable -> { - if (throwable instanceof StaleDataException) { - // Coalescing can fail if the cursor passed to Coalescer is closed by the loader while - // the work is still in progress. - // This can happen when the loader restarts and finishes loading data before the - // coalescing work is completed. - // This failure doesn't need to be thrown as coalescing will be restarted on the latest - // data obtained by the loader. - // TODO(linyuh): Also throw an exception if the failure above can be avoided. - LogUtil.e("NewCallLogFragment.onLoadFinished", "coalescing failed", throwable); - } else { + // Coalescing can fail if the cursor passed to Coalescer is closed by the loader while + // the work is still in progress. + // This can happen when the loader restarts and finishes loading data before the + // coalescing work is completed. + // This failure is identified by ExpectedCoalescerException and doesn't need to be + // thrown as coalescing will be restarted on the latest data obtained by the loader. + if (!(throwable instanceof Coalescer.ExpectedCoalescerException)) { throw new AssertionError(throwable); } }); -- cgit v1.2.3 From c36785d230920498d3476d5f881ae5c55b4d2fb3 Mon Sep 17 00:00:00 2001 From: twyen Date: Thu, 28 Jun 2018 16:14:49 -0700 Subject: Avoid updating system call log cache when the call log is being built The initial build can contain up to 1000 rows, which may be more the what the system call log can handle with a batch operation. Initial build does not create any additional information, so the cache does not need to be updated. This CL also limits the cache updater to 100 rows as a safety measure. TEST=TAP Bug: 77292040 Test: TAP PiperOrigin-RevId: 202563376 Change-Id: Iff515ae2b76a19d63d9d794a0d443cc8332e83a4 --- .../dialer/calllog/CallLogCacheUpdater.java | 30 +++++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) (limited to 'java/com/android/dialer/calllog') diff --git a/java/com/android/dialer/calllog/CallLogCacheUpdater.java b/java/com/android/dialer/calllog/CallLogCacheUpdater.java index a7b2b3d0d..b3130e964 100644 --- a/java/com/android/dialer/calllog/CallLogCacheUpdater.java +++ b/java/com/android/dialer/calllog/CallLogCacheUpdater.java @@ -25,6 +25,7 @@ import android.os.RemoteException; import android.provider.CallLog; import android.provider.CallLog.Calls; import android.provider.ContactsContract.CommonDataKinds.Phone; +import android.support.annotation.VisibleForTesting; import com.android.dialer.DialerPhoneNumber; import com.android.dialer.NumberAttributes; import com.android.dialer.calllog.database.contract.AnnotatedCallLogContract.AnnotatedCallLog; @@ -33,6 +34,7 @@ import com.android.dialer.common.LogUtil; import com.android.dialer.common.concurrent.Annotations.BackgroundExecutor; import com.android.dialer.inject.ApplicationContext; import com.android.dialer.protos.ProtoParsers; +import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import java.util.ArrayList; @@ -49,13 +51,23 @@ public final class CallLogCacheUpdater { private final Context appContext; private final ListeningExecutorService backgroundExecutor; + private final CallLogState callLogState; + + /** + * Maximum numbers of operations the updater can do. Each transaction to the system call log will + * trigger a call log refresh, so the updater can only do a single batch. If there are more + * operations it will be truncated. Under normal circumstances there will only be 1 operation + */ + @VisibleForTesting static final int CACHE_UPDATE_LIMIT = 100; @Inject CallLogCacheUpdater( @ApplicationContext Context appContext, - @BackgroundExecutor ListeningExecutorService backgroundExecutor) { + @BackgroundExecutor ListeningExecutorService backgroundExecutor, + CallLogState callLogState) { this.appContext = appContext; this.backgroundExecutor = backgroundExecutor; + this.callLogState = callLogState; } /** @@ -66,17 +78,27 @@ public final class CallLogCacheUpdater { * has changed */ public ListenableFuture updateCache(CallLogMutations mutations) { - return backgroundExecutor.submit( - () -> { + return Futures.transform( + callLogState.isBuilt(), + isBuilt -> { + if (!isBuilt) { + // Initial build might need to update 1000 caches, which may overflow the batch + // operation limit. The initial data was already built with the cache, there's no need + // to update it. + LogUtil.i("CallLogCacheUpdater.updateCache", "not updating cache for initial build"); + return null; + } updateCacheInternal(mutations); return null; - }); + }, + backgroundExecutor); } private void updateCacheInternal(CallLogMutations mutations) { ArrayList operations = new ArrayList<>(); Stream.concat( mutations.getInserts().entrySet().stream(), mutations.getUpdates().entrySet().stream()) + .limit(CACHE_UPDATE_LIMIT) .forEach( entry -> { ContentValues values = entry.getValue(); -- cgit v1.2.3