Bug 1254468 - Remove broken TransitionAwareCursorLoaderCallbacks r?sebastian draft
authorAndrzej Hunt <andrzej@ahunt.org>
Mon, 14 Mar 2016 15:38:53 -0700
changeset 340536 2aec507de98a45c07afee3355f47294555f712fd
parent 338556 1832f51ba1a518c64fdb2f3affeb2b470e89d0c9
child 340537 67ae52caaa78623fa2aaf4ecbc1ecb2d7d401c2d
push id13001
push userbmo:ahunt@mozilla.com
push dateTue, 15 Mar 2016 16:20:31 +0000
reviewerssebastian
bugs1254468
milestone47.0a1
Bug 1254468 - Remove broken TransitionAwareCursorLoaderCallbacks r?sebastian 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
mobile/android/base/java/org/mozilla/gecko/home/BookmarksPanel.java
mobile/android/base/java/org/mozilla/gecko/home/DynamicPanel.java
mobile/android/base/java/org/mozilla/gecko/home/HistoryPanel.java
mobile/android/base/java/org/mozilla/gecko/home/ReadingListPanel.java
mobile/android/base/java/org/mozilla/gecko/home/RecentTabsPanel.java
mobile/android/base/java/org/mozilla/gecko/home/RemoteTabsBaseFragment.java
mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java
mobile/android/base/java/org/mozilla/gecko/home/TransitionAwareCursorLoaderCallbacks.java
mobile/android/base/moz.build
--- a/mobile/android/base/java/org/mozilla/gecko/home/BookmarksPanel.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/BookmarksPanel.java
@@ -17,16 +17,17 @@ import org.mozilla.gecko.home.BookmarksL
 import org.mozilla.gecko.home.HomeContextMenuInfo.RemoveItemType;
 import org.mozilla.gecko.home.HomePager.OnUrlOpenListener;
 
 import android.app.Activity;
 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;
 import android.view.ViewGroup;
 import android.view.ViewStub;
 import android.widget.ImageView;
 import android.widget.TextView;
 
@@ -210,37 +211,35 @@ public class BookmarksPanel extends Home
         public RefreshType getRefreshType() {
             return mRefreshType;
         }
     }
 
     /**
      * 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) {
                 return new BookmarksLoader(getActivity());
             } else {
                 FolderInfo folderInfo = (FolderInfo) args.getParcelable(BOOKMARKS_FOLDER_INFO);
                 RefreshType refreshType = (RefreshType) args.getParcelable(BOOKMARKS_REFRESH_TYPE);
                 return new BookmarksLoader(getActivity(), folderInfo, refreshType);
             }
         }
 
         @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);
         }
 
         @Override
         public void onLoaderReset(Loader<Cursor> loader) {
-            super.onLoaderReset(loader);
-
             if (mList != null) {
                 mListAdapter.swapCursor(null);
             }
         }
     }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/home/DynamicPanel.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/DynamicPanel.java
@@ -340,39 +340,37 @@ public class DynamicPanel extends HomeFr
             // to pull items from fake_home_items.json.
             return cr.query(queryUri, null, selection, selectionArgs, null);
         }
     }
 
     /**
      * 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);
 
             Log.d(LOGTAG, "Creating loader for request: " + request);
             return new PanelDatasetLoader(getActivity(), request);
         }
 
         @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);
 
             if (mPanelLayout != null) {
                 mPanelLayout.deliverDataset(request, cursor);
             }
         }
 
         @Override
         public void onLoaderReset(Loader<Cursor> loader) {
-            super.onLoaderReset(loader);
-
             final DatasetRequest request = getRequestFromLoader(loader);
             Log.d(LOGTAG, "Resetting loader for request: " + request);
 
             if (mPanelLayout != null) {
                 mPanelLayout.releaseDataset(request.getViewIndex());
             }
         }
 
--- a/mobile/android/base/java/org/mozilla/gecko/home/HistoryPanel.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/HistoryPanel.java
@@ -32,16 +32,17 @@ import org.mozilla.gecko.util.HardwareUt
 
 import android.app.AlertDialog;
 import android.content.ContentResolver;
 import android.content.Context;
 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;
 import android.text.TextPaint;
 import android.text.method.LinkMovementMethod;
 import android.text.style.ClickableSpan;
 import android.text.style.StyleSpan;
 import android.text.style.UnderlineSpan;
@@ -492,27 +493,26 @@ public class HistoryPanel extends HomeFr
             final TextView textView = (TextView) view.getTag();
             textView.setText(getMostRecentSectionTitle(current));
             textView.setTextColor(ColorUtils.getColor(context, current == selected ? R.color.text_and_tabs_tray_grey : R.color.disabled_grey));
             textView.setCompoundDrawablesWithIntrinsicBounds(0, 0, current == selected ? R.drawable.home_group_collapsed : 0, 0);
             return view;
         }
     }
 
-    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);
         }
     }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/home/ReadingListPanel.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/ReadingListPanel.java
@@ -2,16 +2,17 @@
  * 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 java.util.EnumSet;
 
+import android.support.v4.app.LoaderManager;
 import android.util.Log;
 import org.mozilla.gecko.GeckoProfile;
 import org.mozilla.gecko.R;
 import org.mozilla.gecko.ReaderModeUtils;
 import org.mozilla.gecko.Telemetry;
 import org.mozilla.gecko.TelemetryContract;
 import org.mozilla.gecko.db.BrowserContract.ReadingListItems;
 import org.mozilla.gecko.db.BrowserContract.URLColumns;
@@ -207,27 +208,26 @@ public class ReadingListPanel extends Ho
         public View newView(Context context, Cursor cursor, ViewGroup parent) {
             return LayoutInflater.from(parent.getContext()).inflate(R.layout.reading_list_item_row, parent, false);
         }
     }
 
     /**
      * 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);
         }
     }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/home/RecentTabsPanel.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/RecentTabsPanel.java
@@ -27,16 +27,17 @@ import org.mozilla.gecko.util.NativeEven
 import org.mozilla.gecko.util.NativeJSObject;
 import org.mozilla.gecko.util.ThreadUtils;
 
 import android.content.Context;
 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;
 import android.view.View;
 import android.view.ViewGroup;
 import android.view.ViewStub;
 import android.widget.AdapterView;
 import android.widget.ImageView;
@@ -404,27 +405,26 @@ public class RecentTabsPanel extends Hom
             } else if (itemType == ROW_STANDARD) {
                 final TwoLinePageRow pageRow = (TwoLinePageRow) view;
                 pageRow.setShowIcons(false);
                 pageRow.updateFromCursor(c);
             }
          }
     }
 
-    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);
         }
     }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/home/RemoteTabsBaseFragment.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/RemoteTabsBaseFragment.java
@@ -5,16 +5,17 @@
 
 package org.mozilla.gecko.home;
 
 import android.accounts.Account;
 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;
 import android.view.ContextMenu;
 import android.view.MenuInflater;
 import android.view.MenuItem;
 import android.view.View;
 
@@ -209,27 +210,27 @@ public abstract class RemoteTabsBaseFrag
         }
 
         @Override
         public Cursor loadCursor() {
             return mProfile.getDB().getTabsAccessor().getRemoteTabsCursor(getContext());
         }
     }
 
-    protected class CursorLoaderCallbacks extends TransitionAwareCursorLoaderCallbacks {
+    protected class CursorLoaderCallbacks implements LoaderManager.LoaderCallbacks<Cursor> {
         private BrowserDB mDB;    // Pseudo-final: set in onCreateLoader.
 
         @Override
         public Loader<Cursor> onCreateLoader(int id, Bundle args) {
             mDB = GeckoProfile.get(getActivity()).getDB();
             return new RemoteTabsCursorLoader(getActivity());
         }
 
         @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
             // list is updated in place; the hidden clients list is built
             // incrementally.
             mHiddenClients.clear();
             final Iterator<RemoteClient> it = clients.iterator();
             while (it.hasNext()) {
@@ -242,17 +243,16 @@ public abstract class RemoteTabsBaseFrag
 
             mAdapter.replaceClients(clients);
             updateUiFromClients(clients, mHiddenClients);
             scheduleLastSyncedTime();
         }
 
         @Override
         public void onLoaderReset(Loader<Cursor> loader) {
-            super.onLoaderReset(loader);
             mAdapter.replaceClients(null);
         }
     }
 
     protected class RemoteTabsRefreshListener implements SwipeRefreshLayout.OnRefreshListener {
         @Override
         public void onRefresh() {
             if (FirefoxAccounts.firefoxAccountsExist(getActivity())) {
--- a/mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java
+++ b/mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java
@@ -667,35 +667,34 @@ public class TopSitesPanel extends HomeF
         @Override
         public void onFaviconLoaded(String url, String faviconURL, Bitmap favicon) {
             if (TextUtils.equals(this.view.getUrl(), url)) {
                 this.view.displayFavicon(favicon, faviconURL, this.loadId);
             }
         }
     }
 
-    private class CursorLoaderCallbacks extends TransitionAwareCursorLoaderCallbacks {
+    private class CursorLoaderCallbacks implements LoaderCallbacks<Cursor> {
         @Override
         public Loader<Cursor> onCreateLoader(int id, Bundle args) {
             trace("Creating TopSitesLoader: " + id);
             return new TopSitesLoader(getActivity());
         }
 
         /**
          * This method is called *twice* in some circumstances.
          *
          * If you try to avoid that through some kind of boolean flag,
          * sometimes (e.g., returning to the activity) you'll *not* be called
          * twice, and thus you'll never draw thumbnails.
          *
          * 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);
             mGridAdapter.swapCursor(c);
             updateUiFromCursor(c);
 
             final int col = c.getColumnIndexOrThrow(TopSites.URL);
 
@@ -731,18 +730,16 @@ public class TopSitesPanel extends HomeF
 
             Bundle bundle = new Bundle();
             bundle.putStringArrayList(THUMBNAILS_URLS_KEY, urls);
             getLoaderManager().restartLoader(LOADER_ID_THUMBNAILS, bundle, mThumbnailsLoaderCallbacks);
         }
 
         @Override
         public void onLoaderReset(Loader<Cursor> loader) {
-            super.onLoaderReset(loader);
-
             if (mListAdapter != null) {
                 mListAdapter.swapCursor(null);
             }
 
             if (mGridAdapter != null) {
                 mGridAdapter.swapCursor(null);
             }
         }
deleted file mode 100644
--- a/mobile/android/base/java/org/mozilla/gecko/home/TransitionAwareCursorLoaderCallbacks.java
+++ /dev/null
@@ -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;
-        }
-    }
-}
\ No newline at end of file
--- a/mobile/android/base/moz.build
+++ b/mobile/android/base/moz.build
@@ -413,17 +413,16 @@ gbjar.sources += ['java/org/mozilla/geck
     'home/SimpleCursorLoader.java',
     'home/SpacingDecoration.java',
     'home/TabMenuStrip.java',
     'home/TabMenuStripLayout.java',
     'home/TopSitesGridItemView.java',
     'home/TopSitesGridView.java',
     'home/TopSitesPanel.java',
     'home/TopSitesThumbnailView.java',
-    'home/TransitionAwareCursorLoaderCallbacks.java',
     'home/TwoLinePageRow.java',
     'InputConnectionListener.java',
     'InputMethods.java',
     'IntentHelper.java',
     'javaaddons/JavaAddonManager.java',
     'javaaddons/JavaAddonManagerV1.java',
     'lwt/LightweightTheme.java',
     'lwt/LightweightThemeDrawable.java',