Bug 1254468 - Remove broken TransitionAwareCursorLoaderCallbacks r=sebastian a=ritu

TransitionAwareCursorLoaderCallbacks is fundamentally flawed: old CursorLoader
cursors _must_ not be used after onLoadFinished has been called. However
we sometimes queue the cursor swapping (which is implemented by subclasses
in onLoadFinishedAfterTransitions) until after transitions have finished.
CursorLoader.deliverResult() closes the old cursor immediately after calling
onLoadFinished (with the new cursor). At this stage the adapter is
still holding onto the old (but now closed cursor), and will crash if it tries
to read this cursor (which can happen if the adapter is still iterating over the
cursor).

Instead we should ensure that we swap the cursors during onLoadFinished - the simplest
way to do this is by eliminating TransitionAwareCursorLoader and using onLoadFinished
the way the Android framework expects.

It's worth noting that TransitionAwareCursorLoader is obsolete: at the time it was added,
home panels were placed in the HomePagerTabStrip, which notified TransitionsTracker about
its transitions. However HomePagerTabStrip no longer exists, hence there's no need
for us to care about these transitions anymore. (The crash seems to happen because we
try to hide the doorhanger every time we receive LOCATION_CHANGE, and each of these starts
a hide transition - even if no doorhanger is shown - hence we often have a transition
in progress every time we show topsites.)

MozReview-Commit-ID: HsytLpHOrp2
This commit is contained in:
Andrzej Hunt 2016-03-14 15:38:53 -07:00
parent bed0a0b496
commit e01296ef75
9 changed files with 19 additions and 83 deletions

View File

@ -22,6 +22,7 @@ import android.content.Context;
import android.content.res.Configuration;
import android.database.Cursor;
import android.os.Bundle;
import android.support.v4.app.LoaderManager;
import android.support.v4.content.Loader;
import android.view.LayoutInflater;
import android.view.View;
@ -215,7 +216,7 @@ public class BookmarksPanel extends HomeFragment {
/**
* Loader callbacks for the LoaderManager of this fragment.
*/
private class CursorLoaderCallbacks extends TransitionAwareCursorLoaderCallbacks {
private class CursorLoaderCallbacks implements LoaderManager.LoaderCallbacks<Cursor> {
@Override
public Loader<Cursor> onCreateLoader(int id, Bundle args) {
if (args == null) {
@ -228,7 +229,7 @@ public class BookmarksPanel extends HomeFragment {
}
@Override
public void onLoadFinishedAfterTransitions(Loader<Cursor> loader, Cursor c) {
public void onLoadFinished(Loader<Cursor> loader, Cursor c) {
BookmarksLoader bl = (BookmarksLoader) loader;
mListAdapter.swapCursor(c, bl.getFolderInfo(), bl.getRefreshType());
updateUiFromCursor(c);
@ -236,8 +237,6 @@ public class BookmarksPanel extends HomeFragment {
@Override
public void onLoaderReset(Loader<Cursor> loader) {
super.onLoaderReset(loader);
if (mList != null) {
mListAdapter.swapCursor(null);
}

View File

@ -345,7 +345,7 @@ public class DynamicPanel extends HomeFragment {
/**
* LoaderCallbacks implementation that interacts with the LoaderManager.
*/
private class PanelLoaderCallbacks extends TransitionAwareCursorLoaderCallbacks {
private class PanelLoaderCallbacks implements LoaderManager.LoaderCallbacks<Cursor> {
@Override
public Loader<Cursor> onCreateLoader(int id, Bundle args) {
final DatasetRequest request = (DatasetRequest) args.getParcelable(DATASET_REQUEST);
@ -355,7 +355,7 @@ public class DynamicPanel extends HomeFragment {
}
@Override
public void onLoadFinishedAfterTransitions(Loader<Cursor> loader, Cursor cursor) {
public void onLoadFinished(Loader<Cursor> loader, Cursor cursor) {
final DatasetRequest request = getRequestFromLoader(loader);
Log.d(LOGTAG, "Finished loader for request: " + request);
@ -366,8 +366,6 @@ public class DynamicPanel extends HomeFragment {
@Override
public void onLoaderReset(Loader<Cursor> loader) {
super.onLoaderReset(loader);
final DatasetRequest request = getRequestFromLoader(loader);
Log.d(LOGTAG, "Resetting loader for request: " + request);

View File

@ -37,6 +37,7 @@ import android.content.DialogInterface;
import android.database.Cursor;
import android.graphics.Typeface;
import android.os.Bundle;
import android.support.v4.app.LoaderManager;
import android.support.v4.content.Loader;
import android.support.v4.widget.CursorAdapter;
import android.text.SpannableStringBuilder;
@ -497,21 +498,20 @@ public class HistoryPanel extends HomeFragment {
}
}
private class CursorLoaderCallbacks extends TransitionAwareCursorLoaderCallbacks {
private class CursorLoaderCallbacks implements LoaderManager.LoaderCallbacks<Cursor> {
@Override
public Loader<Cursor> onCreateLoader(int id, Bundle args) {
return new HistoryCursorLoader(getActivity());
}
@Override
public void onLoadFinishedAfterTransitions(Loader<Cursor> loader, Cursor c) {
public void onLoadFinished(Loader<Cursor> loader, Cursor c) {
mAdapter.swapCursor(c);
updateUiFromCursor(c);
}
@Override
public void onLoaderReset(Loader<Cursor> loader) {
super.onLoaderReset(loader);
mAdapter.swapCursor(null);
}
}

View File

@ -24,6 +24,7 @@ import android.content.Context;
import android.database.Cursor;
import android.os.Bundle;
import android.support.v4.content.Loader;
import android.support.v4.app.LoaderManager;
import android.support.v4.widget.CursorAdapter;
import android.text.SpannableStringBuilder;
import android.text.Spanned;
@ -212,21 +213,20 @@ public class ReadingListPanel extends HomeFragment {
/**
* LoaderCallbacks implementation that interacts with the LoaderManager.
*/
private class CursorLoaderCallbacks extends TransitionAwareCursorLoaderCallbacks {
private class CursorLoaderCallbacks implements LoaderManager.LoaderCallbacks<Cursor> {
@Override
public Loader<Cursor> onCreateLoader(int id, Bundle args) {
return new ReadingListLoader(getActivity());
}
@Override
public void onLoadFinishedAfterTransitions(Loader<Cursor> loader, Cursor c) {
public void onLoadFinished(Loader<Cursor> loader, Cursor c) {
mAdapter.swapCursor(c);
updateUiFromCursor(c);
}
@Override
public void onLoaderReset(Loader<Cursor> loader) {
super.onLoaderReset(loader);
mAdapter.swapCursor(null);
}
}

View File

@ -32,6 +32,7 @@ import android.database.Cursor;
import android.database.MatrixCursor;
import android.database.MatrixCursor.RowBuilder;
import android.os.Bundle;
import android.support.v4.app.LoaderManager;
import android.support.v4.content.Loader;
import android.util.Log;
import android.view.LayoutInflater;
@ -409,21 +410,20 @@ public class RecentTabsPanel extends HomeFragment
}
}
private class CursorLoaderCallbacks extends TransitionAwareCursorLoaderCallbacks {
private class CursorLoaderCallbacks implements LoaderManager.LoaderCallbacks<Cursor> {
@Override
public Loader<Cursor> onCreateLoader(int id, Bundle args) {
return new RecentTabsCursorLoader(getActivity(), mClosedTabs);
}
@Override
public void onLoadFinishedAfterTransitions(Loader<Cursor> loader, Cursor c) {
public void onLoadFinished(Loader<Cursor> loader, Cursor c) {
mAdapter.swapCursor(c);
updateUiFromCursor(c);
}
@Override
public void onLoaderReset(Loader<Cursor> loader) {
super.onLoaderReset(loader);
mAdapter.swapCursor(null);
}
}

View File

@ -10,6 +10,7 @@ import android.content.Context;
import android.database.Cursor;
import android.os.Bundle;
import android.os.Handler;
import android.support.v4.app.LoaderManager;
import android.support.v4.content.Loader;
import android.support.v4.widget.SwipeRefreshLayout;
import android.util.Log;
@ -214,7 +215,7 @@ public abstract class RemoteTabsBaseFragment extends HomeFragment implements Rem
}
}
protected class CursorLoaderCallbacks extends TransitionAwareCursorLoaderCallbacks {
protected class CursorLoaderCallbacks implements LoaderManager.LoaderCallbacks<Cursor> {
private BrowserDB mDB; // Pseudo-final: set in onCreateLoader.
@Override
@ -224,7 +225,7 @@ public abstract class RemoteTabsBaseFragment extends HomeFragment implements Rem
}
@Override
public void onLoadFinishedAfterTransitions(Loader<Cursor> loader, Cursor c) {
public void onLoadFinished(Loader<Cursor> loader, Cursor c) {
final List<RemoteClient> clients = mDB.getTabsAccessor().getClientsFromCursor(c);
// Filter the hidden clients out of the clients list. The clients
@ -247,7 +248,6 @@ public abstract class RemoteTabsBaseFragment extends HomeFragment implements Rem
@Override
public void onLoaderReset(Loader<Cursor> loader) {
super.onLoaderReset(loader);
mAdapter.replaceClients(null);
}
}

View File

@ -672,7 +672,7 @@ public class TopSitesPanel extends HomeFragment {
}
}
private class CursorLoaderCallbacks extends TransitionAwareCursorLoaderCallbacks {
private class CursorLoaderCallbacks implements LoaderCallbacks<Cursor> {
@Override
public Loader<Cursor> onCreateLoader(int id, Bundle args) {
trace("Creating TopSitesLoader: " + id);
@ -689,8 +689,7 @@ public class TopSitesPanel extends HomeFragment {
* The root cause is TopSitesLoader.loadCursor being called twice.
* Why that is... dunno.
*/
@Override
protected void onLoadFinishedAfterTransitions(Loader<Cursor> loader, Cursor c) {
public void onLoadFinished(Loader<Cursor> loader, Cursor c) {
debug("onLoadFinished: " + c.getCount() + " rows.");
mListAdapter.swapCursor(c);
@ -736,8 +735,6 @@ public class TopSitesPanel extends HomeFragment {
@Override
public void onLoaderReset(Loader<Cursor> loader) {
super.onLoaderReset(loader);
if (mListAdapter != null) {
mListAdapter.swapCursor(null);
}

View File

@ -1,57 +0,0 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package org.mozilla.gecko.home;
import android.database.Cursor;
import android.support.v4.app.LoaderManager.LoaderCallbacks;
import android.support.v4.content.Loader;
import org.mozilla.gecko.animation.TransitionsTracker;
/**
* A {@link LoaderCallbacks} implementation that avoids running its
* {@link #onLoadFinished(Loader, Cursor)} method during animations as it's
* likely to trigger a layout traversal as a result of a cursor swap in the
* target adapter.
*/
public abstract class TransitionAwareCursorLoaderCallbacks implements LoaderCallbacks<Cursor> {
private OnLoadFinishedRunnable onLoadFinishedRunnable;
@Override
public void onLoadFinished(Loader<Cursor> loader, Cursor c) {
if (onLoadFinishedRunnable != null) {
TransitionsTracker.cancelPendingAction(onLoadFinishedRunnable);
}
onLoadFinishedRunnable = new OnLoadFinishedRunnable(loader, c);
TransitionsTracker.runAfterTransitions(onLoadFinishedRunnable);
}
protected abstract void onLoadFinishedAfterTransitions(Loader<Cursor> loade, Cursor c);
@Override
public void onLoaderReset(Loader<Cursor> loader) {
if (onLoadFinishedRunnable != null) {
TransitionsTracker.cancelPendingAction(onLoadFinishedRunnable);
onLoadFinishedRunnable = null;
}
}
private class OnLoadFinishedRunnable implements Runnable {
private final Loader<Cursor> loader;
private final Cursor cursor;
public OnLoadFinishedRunnable(Loader<Cursor> loader, Cursor cursor) {
this.loader = loader;
this.cursor = cursor;
}
@Override
public void run() {
onLoadFinishedAfterTransitions(loader, cursor);
onLoadFinishedRunnable = null;
}
}
}

View File

@ -417,7 +417,6 @@ gbjar.sources += ['java/org/mozilla/gecko/' + x for x in [
'home/TopSitesGridView.java',
'home/TopSitesPanel.java',
'home/TopSitesThumbnailView.java',
'home/TransitionAwareCursorLoaderCallbacks.java',
'home/TwoLinePageRow.java',
'InputConnectionListener.java',
'InputMethods.java',