1077590 - Make all per-profile DB access go through a profile

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Wesley Johnston (:wesj)]

+++ This bug was initially created as a clone of Bug #1072332 +++

Lets look at making our database access smarter about profiles and threads. Rnewman suggested:

Avoiding this and the other issues is why I suggest this kind of idiom, where we're very explicit about the profile that we're using:

  final GeckoProfile profile = GeckoProfile.get();
  if (profile == null) {
    // Well, that shouldn't happen; we should always have a profile!
    return;
  }

  if (profile.isGuestProfile()) {
    // Maybe we don't want to do this write...
    return;
  }

  ThreadUtils.postToBackgroundThread(new Runnable() {
    @Override
    public void run() {
      // Or capture browserDB outside the runnable. Doesn't matter.
      profile.browserDB().writeWhatever(someURL);
    }
  });

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Here's a WIP for this. I'm basically trying to force all access to any perProfile databases through the LocalBrowserDB object, force all of those to use a profile, and ensure that they all get a db when they're called so that we can ensure they'll be called with the correct profile no matter when they actually run later.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

Lets try to move a bit quickly here.

https://tbpl.mozilla.org/?tree=Try&rev=026983d11039

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8499905 [details] [diff] [review]
WIP Patch

Review of attachment 8499905 [details] [diff] [review]:
-----------------------------------------------------------------

Huge props for doing this. Killing singletons is a noble quest.

Did you add android.iml by accident?

This patch assumes that getProfile will always return a profile, and that getDB will always return a valid LocalBrowserDB. Are we confident in those assumptions?

Also, sadness about some of the reindenting. Please fix what you can. See inline.

::: mobile/android/base/BrowserApp.java
@@ +499,4 @@
>          (new UIAsyncTask.WithoutParams<String>(ThreadUtils.getBackgroundHandler()) {
>              @Override
>              public String doInBackground() {
> +                return Favicons.getFaviconURLForPageURL(db, getContentResolver(), url);

Paging ckitching; we touched on this some when working on the share overlay.

::: mobile/android/base/GeckoApp.java
@@ +521,5 @@
>      void handleClearHistory() {
> +        ThreadUtils.postToBackgroundThread(new Runnable() {
> +            @Override
> +            public void run() {
> +                getProfile().getDb().clearHistory(getContentResolver());

Lift the 'get' out of the runnable.

@@ -1465,5 @@
>          Tabs.registerOnTabsChangedListener(this);
>  
>          initializeChrome();
>  
> -        BrowserDB.initialize(getProfile().getName());

Yay, this'll fix Bug 1077858.

::: mobile/android/base/GeckoProfile.java
@@ +17,5 @@
>  
>  import org.mozilla.gecko.GeckoProfileDirectories.NoMozillaDirectoryException;
>  import org.mozilla.gecko.GeckoProfileDirectories.NoSuchProfileException;
>  import org.mozilla.gecko.db.LocalBrowserDB;
> +import org.mozilla.gecko.db.SuggestedSites;

Unneeded import.

@@ +52,5 @@
>      private final File mMozillaDir;
>      private final boolean mIsWebAppProfile;
>      private final Context mApplicationContext;
>  
> +    private final LocalBrowserDB mDb;

I have a preference for "DB" throughout (mDB, getDB), if only for consistency with LocalBrowserDB.

::: mobile/android/base/GeckoView.java
@@ +137,5 @@
>              // If you want to use GeckoNetworkManager, start it.
>  
>              GeckoLoader.loadMozGlue(context);
> +            final GeckoProfile profile = GeckoProfile.get(context);
> +            profile.getDb().setEnabled(false);

Something to think about: perhaps we instead want a DisabledLocalBrowserDB that we hand to the profile, rather than having a boolean flag that LocalBrowserDB has to check all the time?

::: mobile/android/base/GlobalHistory.java
@@ +43,3 @@
>      private boolean mProcessing; // = false             // whether or not the runnable is queued/working
>  
> +    private class NotifierRunnable implements Runnable {

static

@@ +49,5 @@
> +        public NotifierRunnable(final Context context) {
> +            mContext = context;
> +            mProfile = GeckoProfile.get(context);
> +        }
> +        @Override

Newline before.

::: mobile/android/base/Tab.java
@@ +293,5 @@
>          }
>  
>          final ContentResolver cr = mAppContext.getContentResolver();
> +        final GeckoProfile profile = GeckoProfile.get(mAppContext);
> +        final URLMetadata urlMetadata = profile.getDb().urlMetadata;

Subtly eliminating a singleton \o/

@@ +447,5 @@
>          });
>      }
>  
>      public void addBookmark() {
> +        final LocalBrowserDB db = GeckoProfile.get(mAppContext).getDb();

Would it be fair to say a Tab is linked to a profile, and just set a profile member when we set mAppContext?

@@ +710,5 @@
>      void handleContentLoaded() {
>          setLoadProgressIfLoading(LOAD_PROGRESS_LOADED);
>      }
>  
> +    protected void saveThumbnailToDB(final LocalBrowserDB db) {

I love it.

::: mobile/android/base/Tabs.java
@@ +74,2 @@
>  
> +    private class PersistTabsRunnable implements Runnable {

static, take app context as constructor argument?

@@ +145,5 @@
>          // The listener will run on the background thread (see 2nd argument).
>          mAccountManager.addOnAccountsUpdatedListener(mAccountListener, ThreadUtils.getBackgroundHandler(), false);
>  
>          if (mContentObserver != null) {
> +            // Its safe to use the db here since we aren't doing any I/O.

Nit: It's

@@ +195,5 @@
>                      }
>                  }
>              };
> +
> +            // Its safe to use the db here since we aren't doing any I/O.

Same.

@@ +680,5 @@
>       * those requests are removed.
>       */
>      private void queuePersistAllTabs() {
> +        final Handler backgroundHandler = ThreadUtils.getBackgroundHandler();
> +        if (mPersistTabsRunnable != null) {

Note that this is safe because there's only one caller, and that's always on the same thread.

::: mobile/android/base/db/BrowserDB.java
@@ +34,5 @@
>   *
>   * Also manages some flags.
>   */
>  public class BrowserDB {
> +    public static void initialize(String profile) {

Looks like this can just go…

::: mobile/android/base/db/LocalBrowserDB.java
@@ +55,5 @@
>  import android.text.TextUtils;
>  import android.util.Log;
>  
>  public class LocalBrowserDB {
> +    private boolean enabled = true;

This should be volatile.

@@ +101,5 @@
>      private final Uri mUpdateHistoryUriWithProfile;
>      private final Uri mFaviconsUriWithProfile;
>      private final Uri mThumbnailsUriWithProfile;
>      private final Uri mReadingListUriWithProfile;
> +    private volatile SuggestedSites mSuggestedSites;

Move up above the Uris.

@@ +109,5 @@
> +                    Bookmarks.GUID,
> +                    Bookmarks.URL,
> +                    Bookmarks.TITLE,
> +                    Bookmarks.TYPE,
> +                    Bookmarks.PARENT };

Please maintain the column indenting.

@@ +128,5 @@
>          mUpdateHistoryUriWithProfile = mHistoryUriWithProfile.buildUpon().
> +                appendQueryParameter(BrowserContract.PARAM_INCREMENT_VISITS, "true").
> +                appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true").build();
> +
> +        // TODO: We should ensure these have hold of the profile name, so that they can correctly

Is this still a TODO? We give them mProfile. Or do you mean "make sure these classes are profile-aware"?

@@ +266,5 @@
>       * Add bookmarks from the provided distribution.
>       * Takes an offset; returns a new offset.
>       */
>      public int addDistributionBookmarks(ContentResolver cr, Distribution distribution, int offset) {
> +        if (!enabled) {

See earlier comment about switching out mDB rather than using a flag.

@@ +484,5 @@
>      }
>  
>      private Uri bookmarksUriWithLimit(int limit) {
>          return mBookmarksUriWithProfile.buildUpon().appendQueryParameter(BrowserContract.PARAM_LIMIT,
> +                String.valueOf(limit)).build();

Chaining in stacks:

 return mBookmarksUriWithProfile.buildUpon()
                                .appendQueryParameter(BrowserContract.PARAM_LIMIT,
String.valueOf(limit))
                                .build();

@@ +500,5 @@
>      }
>  
>      private Uri getAllHistoryUri() {
>          Uri.Builder uriBuilder = mHistoryUriWithProfile.buildUpon()
> +                .appendQueryParameter(BrowserContract.PARAM_SHOW_DELETED, "1");

Define a private static helper, withDeleted(uri)?

@@ +545,5 @@
>          return cr.query(combinedUriWithLimit(limit),
> +                projection,
> +                selection,
> +                selectionArgs,
> +                sortOrder);

Undo!

@@ +609,5 @@
>          }
>  
>          return filterAllSites(cr,
> +                new String[] { Combined._ID,
> +                        Combined.URL,

Again, I have a preference for column alignment here.

@@ +1447,5 @@
>          if (url != null) {
>              // Bookmarks are defined by their URL and Folder.
>              builder.withSelection(Bookmarks.URL + " = ? AND "
> +                            + Bookmarks.PARENT + " = ?",
> +                    new String[] { url,

Aieeee

@@ +1593,5 @@
>                  .appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true").build();
>          cr.update(uri,
> +                values,
> +                Bookmarks.POSITION + " = ? AND " +
> +                        Bookmarks.PARENT + " = ?",

Aieeeee

@@ +1642,5 @@
> +        mSuggestedSites = suggestedSites;
> +    }
> +
> +    public boolean hasSuggestedImageUrl(String url) {
> +        return mSuggestedSites.contains(url);

Null check on mSuggestedSites?

@@ +1664,5 @@
> +    }
> +
> +    private static void appendUrlsFromCursor(List<String> urls, Cursor c) {
> +        c.moveToPosition(-1);
> +        while (c.moveToNext()) {

I have a gentle preference for:

if (!c.moveToFirst()) {
    return;
}

do {
    ...
} while (c.moveToNext());

::: mobile/android/base/db/Searches.java
@@ +1,1 @@
> +package org.mozilla.gecko.db;

License block.

@@ +13,5 @@
> +public class Searches {
> +    private final Uri uriWithProfile;
> +
> +    public Searches(String mProfile) {
> +        if (!TextUtils.isEmpty(mProfile)) {

Same point as earlier.

Maybe add an appendProfileWithDefault method to DBUtils that does this for you?

::: mobile/android/base/db/TabsAccessor.java
@@ +60,5 @@
> +    private final Uri clientsUriWithProfile;
> +
> +    public TabsAccessor(String mProfile) {
> +        if (TextUtils.isEmpty(mProfile)) {
> +            mProfile = GeckoProfile.DEFAULT_PROFILE;

Same point, different manifestation.

::: mobile/android/base/db/TabsProvider.java
@@ +24,5 @@
>  import android.os.Build;
>  import android.text.TextUtils;
>  
>  public class TabsProvider extends PerProfileDatabaseProvider<TabsProvider.TabsDatabaseHelper> {
> +    static final String DATABASE_NAME = "tabsAccessor.db";

This seems like a breaking change… what's the reasoning?

::: mobile/android/base/db/URLMetadata.java
@@ +36,5 @@
>      private static final String LOGTAG = "GeckoURLMetadata";
> +    private final Uri uriWithProfile;
> +
> +    public URLMetadata(String mProfile) {
> +        if (!TextUtils.isEmpty(mProfile)) {

Remove the !, invert the conditionals, early return.

Or just:

  final String profile = TextUtils.isEmpty(mProfile) ? GeckoProfile.DEFAULT_PROFILE : mProfile;
  uriWithProfile = DBUtils.appendProfile(profile, URLMetadataTable.CONTENT_URI);

::: mobile/android/base/favicons/LoadFaviconTask.java
@@ +92,2 @@
>          ContentResolver resolver = context.getContentResolver();
> +        return db.getFaviconForUrl(resolver, faviconURL);

If you null-check the save, null check the load, too.

@@ +322,5 @@
>      public final void execute() {
>          ThreadUtils.assertOnUiThread();
>  
>          try {
> +            final LocalBrowserDB db = GeckoProfile.get(context).getDb();

Maybe provide the LocalBrowserDB as a constructor argument?

::: mobile/android/base/gfx/BitmapUtils.java
@@ +152,5 @@
>                       }
>                   }
>               });
> +         final GeckoProfile profile = GeckoProfile.get(context);
> +         ThumbnailHelper.getInstance().getAndProcessThumbnailFor(tab, profile.getDb());

Nah, have this method take a LocalBrowserDB as the first argument instead of the unused Context.

::: mobile/android/base/home/BookmarksPanel.java
@@ +175,5 @@
>       */
>      private static class BookmarksLoader extends SimpleCursorLoader {
>          private final FolderInfo mFolderInfo;
>          private final RefreshType mRefreshType;
> +        private final GeckoProfile mProfile;

Store the DB, not the profile.

@@ +183,5 @@
>              final Resources res = context.getResources();
>              final String title = res.getString(R.string.bookmarks_title);
>              mFolderInfo = new FolderInfo(Bookmarks.FIXED_ROOT_ID, title);
>              mRefreshType = RefreshType.CHILD;
> +            mProfile = GeckoProfile.get(context);

I'd be inclined to have the caller provide the profile -- or, better, the DB.

Not every caller will have the profile, but they'll sure have the DB. That's what we're really working with most of the time.

Also, why not have this constructor call its more detailed sibling constructor?

@@ +202,5 @@
>          @Override
>          public void onContentChanged() {
>              // Invalidate the cached value that keeps track of whether or
>              // not desktop bookmarks exist.
> +            mProfile.getDb().invalidateCachedState();

Urgh. Maybe fix this naming while we're here?

::: mobile/android/base/home/HistoryPanel.java
@@ +182,4 @@
>  
>          public HistoryCursorLoader(Context context) {
>              super(context);
> +            mProfile = GeckoProfile.get(context);

Same comment: pass in the DB, store the DB not the profile.

::: mobile/android/base/home/HomeFragment.java
@@ +337,5 @@
>              mContext = context;
>              mUrl = url;
>              mType = type;
>              mPosition = position;
> +            mProfile = GeckoProfile.get(context);

Same.

::: mobile/android/base/home/ReadingListPanel.java
@@ +172,5 @@
>          }
>  
>          @Override
>          public Cursor loadCursor() {
> +            return mProfile.getDb().getReadingList(getContext().getContentResolver());

Same.

::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
@@ +381,3 @@
>          public RemoteTabsCursorLoader(Context context) {
>              super(context);
> +            mProfile = GeckoProfile.get(context);

Same.

::: mobile/android/base/home/SearchLoader.java
@@ +92,5 @@
>          public SearchCursorLoader(Context context, String searchTerm, EnumSet<FilterFlags> flags) {
>              super(context);
>              mSearchTerm = searchTerm;
>              mFlags = flags;
> +            mProfile = GeckoProfile.get(context);

Same.

::: mobile/android/base/home/TopSitesPanel.java
@@ +412,5 @@
>  
>          public TopSitesLoader(Context context) {
>              super(context);
>              mMaxGridEntries = context.getResources().getInteger(R.integer.number_of_top_sites);
> +            mProfile = GeckoProfile.get(context);

Same.

@@ +464,5 @@
>          private Map<String, ThumbnailInfo> mThumbnailInfos;
>  
>          public TopSitesGridAdapter(Context context, Cursor cursor) {
>              super(context, cursor, 0);
> +            mProfile = GeckoProfile.get(context);

Same.

@@ +717,5 @@
>  
>          public ThumbnailsLoader(Context context, ArrayList<String> urls) {
>              super(context);
>              mUrls = urls;
> +            mProfile = GeckoProfile.get(context);

Same.

::: mobile/android/base/tabs/RemoteTabsContainerPanel.java
@@ +138,5 @@
>              return FirefoxAccounts.getFirefoxAccount(getContext());
>          }
>  
> +        public RemoteTabsSyncObserver(final GeckoProfile profile) {
> +            this.profile = profile;

Same.

::: mobile/android/base/tests/DatabaseHelper.java
@@ +48,5 @@
>       */
>      protected void addOrUpdateMobileBookmark(String title, String url) {
>          final ContentResolver resolver = mActivity.getContentResolver();
> +        final GeckoProfile profile = GeckoProfile.get(mActivity);
> +        profile.getDb().addBookmark(resolver, title, url);

This'll come up a lot in tests.

I'd suggest that, at the same level as mActivity, we should have a 'getProfileDB' method:

    protected LocalBrowserDB getProfileDB() {
        return GeckoProfile.get(mActivity).getDB();
    }

then everywhere in tests we can just change

    BrowserDB.addBookmark(...)

to

    getProfileDB().addBookmark(...)

@@ +148,5 @@
> +                        browserData.add(cursor.getString(cursor.getColumnIndex("url")));
> +                    }
> +
> +                    if (!cursor.isLast()) {
> +                        cursor.moveToNext();

This conditional seems broken; you should test the return value of moveToNext.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8500757 [details] [diff] [review]
Patch

My feedback on the previous patch was actually a pretty detailed review, so please chomp through that and post an updated patch for a final skim.

Thanks in advance for your diligence :D

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch

Cleaned up quite a bit. I made these databases go through a "Base" class (I hate that naming) so that consumers would be forced to implement "disabled" methods (see, the name Base != Disabled, hence I kinda hate the naming, but can't find something I like better).

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8501530 [details] [diff] [review]
Patch

Review of attachment 8501530 [details] [diff] [review]:
-----------------------------------------------------------------

Initial pass review:

* There's a thread-safety issue in the disabled switching. r- for that, but we're not far away.
* I'd like to see a separation between disabled and base/interface.

I'll swing by with another comment after thinking more about how to do disabling sanely.

::: mobile/android/base/BrowserApp.java
@@ +1499,5 @@
> +            final ContentResolver cr = getContentResolver();
> +            Telemetry.HistogramAdd("PLACES_PAGES_COUNT", db.getCount(cr, "history"));
> +            Telemetry.HistogramAdd("PLACES_BOOKMARKS_COUNT", db.getCount(cr, "bookmarks"));
> +            Telemetry.HistogramAdd("FENNEC_FAVICONS_COUNT", db.getCount(cr, "favicons"));
> +            Telemetry.HistogramAdd("FENNEC_THUMBNAILS_COUNT", db.getCount(cr, "thumbnails"));

Filed Bug 1080232 to see if we can improve this idiom.

@@ +2058,5 @@
>  
> +        final GeckoProfile profile = getProfile();
> +        // Don't bother storing search queries in guest mode
> +        if (profile.inGuestMode()) {
> +            return;

Woo fixes

::: mobile/android/base/GeckoProfile.java
@@ +52,5 @@
>      private final File mMozillaDir;
>      private final boolean mIsWebAppProfile;
>      private final Context mApplicationContext;
>  
> +    private BrowserDB mDb;

mDB?

@@ +94,5 @@
>          }
>  
>          // If the guest profile should be used return it.
>          if (GuestSession.shouldUse(context, "")) {
> +            return GeckoProfile.createGuestProfile(context);

This'll conflict a little with the stuff I just landed, I think. And I'm not confident that this is correct -- it should be getOrCreate.

Coincidentally this'll work as phrased if the context is always GeckoApp, but otherwise it'll create more than once.

@@ +312,5 @@
>      }
>  
> +    public void setDBEnabled(final boolean enabled) {
> +        if (enabled) {
> +            // Setting this to Null will cause us to create a LocalBrowserDB lazily on first access.

s/Null/null

@@ +324,5 @@
> +    public BrowserDB getDB() {
> +        if (mDb == null) {
> +            mDb = new LocalBrowserDB(mName);
> +        }
> +        return mDb;

This isn't thread-safe. We need to do one of the following:

* make this non-lazy, basically going back to the way the patch was before (with enabled being volatile)
* make mDB volatile *and* simplify the access pattern to avoid multiple reads and writes
* make mDB an AtomicReference
* synchronize getDB (which is probably a bad perf tradeoff).

::: mobile/android/base/GeckoView.java
@@ +136,5 @@
>              // If you want to use GeckoNetworkManager, start it.
>  
>              GeckoLoader.loadMozGlue(context);
> +            final GeckoProfile profile = GeckoProfile.get(context);
> +            profile.setDBEnabled(false);

I suspect part of the annoyance with this is a kind of modeling error -- we're implementing an application-level decision (is this the kind of GeckoView that writes browser history and bookmarks to its profile DB?) through a flag on the profile.

Still noodling on a better way to do it.

::: mobile/android/base/GlobalHistory.java
@@ +45,2 @@
>      private final Queue<String> mPendingUris;           // URIs that need to be checked
>      private SoftReference<Set<String>> mVisitedCache;   // cache of the visited URI list

Can we make these package-scoped, then?

@@ +47,5 @@
>      private boolean mProcessing; // = false             // whether or not the runnable is queued/working
>  
> +    private class NotifierRunnable implements Runnable {
> +        private final ContentResolver mContentResolver;
> +        private final BrowserDB mDb;

mDB?

::: mobile/android/base/MemoryMonitor.java
@@ +221,4 @@
>          public StorageReducer(final Context context) {
>              this.mContext = context;
> +            // Since this may be called while Gecko is in the background, we don't want to risk accidentally
> +            // using the wrong context. If the profile we get is a guest profile,

Fragmentary comment; clean up, plz

@@ +244,5 @@
>                  return;
>              }
>  
> +            final ContentResolver cr = mContext.getContentResolver();
> +            if (mDb != null) {

This would only be null if profile.getDB() returns null. If that's the case, then there's lots of code that'll break. Why the null check here?

::: mobile/android/base/Tabs.java
@@ +683,5 @@
>       * those requests are removed.
>       */
>      private void queuePersistAllTabs() {
> +        final Handler backgroundHandler = ThreadUtils.getBackgroundHandler();
> +        // Note: Its safe to modify the runnable here because all of the callers are on the same thred.

thread

@@ +689,5 @@
> +            backgroundHandler.removeCallbacks(mPersistTabsRunnable);
> +            mPersistTabsRunnable = null;
> +        }
> +
> +        mPersistTabsRunnable = new PersistTabsRunnable(mAppContext,     getTabsInOrder());

whitespace

::: mobile/android/base/db/BaseTabsAccessor.java
@@ +1,1 @@
> +package org.mozilla.gecko.db;

license block

::: mobile/android/base/db/BrowserDB.java
@@ +1,1 @@
>  package org.mozilla.gecko.db;

Don't kill the license block.

@@ +28,5 @@
> +    public BaseTabsAccessor getTabsAccessor() {
> +        if (tabsAccessor == null) {
> +            tabsAccessor = new BaseTabsAccessor();
> +        }
> +        return tabsAccessor;

This pattern isn't thread safe. All of the accessors in this file need tweaking for thread safety.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +54,5 @@
>  import android.provider.Browser;
>  import android.text.TextUtils;
>  import android.util.Log;
>  
> +public class LocalBrowserDB extends BrowserDB {

I have a gentle preference for separating BrowserDB (interface) from StubBrowserDB and LocalBrowserDB (implementations).

With inheritance, if LocalBrowserDB forgets to implement a method, it'll quietly use the stub one from BrowserDB. That's probably not desirable; we want a compiler error.

The only changes should be:

* Rename BrowserDB.java to StubBrowserDB.java, and rename the class.
* Introduce an interface containing the public methods of LocalBrowserDB.
* Instantiate StubBrowserDB instead of BrowserDB in the disabling case.

@@ +101,1 @@
>          mProfile = profile;

... which is an indicator that this shouldn't be inheriting.

@@ +115,5 @@
> +                appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true").build();
> +    }
> +
> +    @Override
> +    public BaseSearches  getSearches() {

Whitespace.

@@ +119,5 @@
> +    public BaseSearches  getSearches() {
> +        if (search == null) {
> +            search = new Searches(mProfile);
> +        }
> +        return search;

Again, thread safety.

@@ +492,5 @@
>      }
>  
> +    private static Uri withDeleted(final Uri uri) {
> +        final Uri.Builder uriBuilder = uri.buildUpon()
> +                .appendQueryParameter(BrowserContract.PARAM_SHOW_DELETED, "1");

Just:

  return uri.buildUpon()
            .appendQueryParameter(BrowserContract.PARAM_SHOW_DELETED, "1")
            .build();

::: mobile/android/base/db/URLMetadata.java
@@ +191,2 @@
>                                   .appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true")
>                                   .build();

Please column-align on the dots.

@@ +212,5 @@
>          Uri uri = URLMetadataTable.CONTENT_URI;
>          if (!TextUtils.isEmpty(profile)) {
> +            DBUtils.appendProfile(profile, uri);
> +        } else {
> +            DBUtils.appendProfile(GeckoProfile.DEFAULT_PROFILE, uri);

Hey! We have a util for this now!

::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
@@ +390,5 @@
>          }
>      }
>  
>      private class CursorLoaderCallbacks implements LoaderCallbacks<Cursor> {
> +        private BrowserDB mDB;

final

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8501530 [details] [diff] [review]
Patch

Review of attachment 8501530 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoProfile.java
@@ +322,5 @@
> +    }
> +
> +    public BrowserDB getDB() {
> +        if (mDb == null) {
> +            mDb = new LocalBrowserDB(mName);

OK, here are some thoughts on this.

I don't think it's viable complexity to use an AtomicReference, but nothing else will do if we allow switching here.

So let's change the problem. We don't actually want to call setDBEnabled at arbitrary times -- we never re-enable a profile for DB writes, or turn it off after startup.

We were using it as a toggle because BrowserDB was fixed to basically be a static instance of LocalBrowserDB, and we wanted to turn that functionality off in GeckoView.

We're no longer constrained to this approach, because we no longer have a static BrowserDB.

The right thing to do, then, is to make the *kind of BrowserDB* (the factory, if you will) an argument during construction of the profile instance, which we typically do only once.

And we *know* that already, at compile-time -- if the instantiating class is a GeckoView, we use StubBrowserDB, otherwise we use LocalBrowserDB.

So let's make the GeckoProfile constructor take an argument (either a BrowserDBFactory or just a boolean with an if-statement, because we're lazy... it would even be possible to do this with generics!), pass it via GeckoProfile.get, and we'll use different arguments for GeckoView and GeckoApp.

setDBEnabled will disappear entirely, mDB will become final, and we'll decide which BrowserDB implementation to use for it *in the constructor*, based on the argument we were given by our consumer.

How does that sound to you, Wes?

Comment 8

[Richard Newman [:rnewman]]

Ah munnah steal this, 'cos it'll fix a low background hum of crashes.

Comment 9

[Richard Newman [:rnewman]]

Try push for the rebase, before I start applying my own comments:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=abb203971a4b

Wow that was a painful rebase. Thanks, hg.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Part 1: make all per-profile DB access go through a profile.

Rebased patch. Gonna put a Part 2 on top of it.

Comment 11

[Richard Newman [:rnewman]]

Attachment: Part 2: address initial review comments, fix small bugs. v1

Comment 12

[Richard Newman [:rnewman]]

Attachment: Bu - Part 3: eliminate the need to disable BrowserDB. v1

Comment 13

[Richard Newman [:rnewman]]

Attachment: Part 1: make all per-profile DB access go through a profile. v3

Some small fixes to a test.

Comment 14

[Richard Newman [:rnewman]]

Attachment: Part 2: address initial review comments, fix small bugs. v2

Now with 50% more `hg add`.

Comment 15

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa934611f746

Comment 16

[Richard Newman [:rnewman]]

Latest try push includes a little safety check I added to detect if we pull a profile from sProfileCache and ask for a different profile directory with the same name.

Tests do, routinely.

This is a bad thing: the profile does not expect the directory to change after creation, and this makes our tests vulnerable to subtle changes in order of evaluation.

Still fiddling.

Comment 17

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3bf9c10d559

Comment 18

[Richard Newman [:rnewman]]

Attachment: Part 0: minor sanity fixes in FennecNativeDriver. v1

I somehow hit these when running from Eclipse, which triggered a run without a config. The error handling here is suspect; this patch fixes it.

Comment 19

[Richard Newman [:rnewman]]

Attachment: Part 2: address initial review comments, fix small bugs. v3

This addresses all of my trivial comments, leaving the mDB access and concurrency issues to later parts.

Comment 20

[Richard Newman [:rnewman]]

Attachment: Part 3: rework access to BrowserDB. v2

This patch eliminates the need to disable BrowserDB by specifying the type of the database during profile creation. The default is StubBrowserDB, so that GeckoView consumers have no additional work to do.

It also reworks stubbing for URLMetadata, TabsAccessor, and Searches, and fixes the concurrency issues that were present in the original patch.

Comment 21

[Richard Newman [:rnewman]]

Attachment: Part 4: fix a bunch of imports. v1

I skimmed through Eclipse's warnings and fixed a few.

Comment 22

[Richard Newman [:rnewman]]

Comment on attachment 8543079 [details] [diff] [review]
Part 2: address initial review comments, fix small bugs. v3

Most of this is pretty formulaic. You might want to skim Part 3 before reviewing this, 'cos that part continues the evolution (e.g., mDB becomes final).

Comment 23

[Richard Newman [:rnewman]]

Comment on attachment 8543080 [details] [diff] [review]
Part 3: rework access to BrowserDB. v2

Believe it or not, I used hg mv for the Base* files. Apparently hg mv doesn't understand when you move B to C and A to B in a single commit.

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 8543080 [details] [diff] [review]
Part 3: rework access to BrowserDB. v2

Review of attachment 8543080 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoProfile.java
@@ +180,5 @@
> +    // Note that the profile cache respects only the profile name!
> +    // If the directory changes, the returned GeckoProfile instance will be mutated.
> +    // If the factory differs, it will be *ignored*.
> +    public static GeckoProfile get(Context context, String profileName, File profileDir, BrowserDB.Factory dbFactory) {
> +        Log.v(LOGTAG, "Fetching profile: '" + profileName + "', '" + profileDir + "'");

Oops, forgot to remove this. Imagine that it's gone.

::: mobile/android/base/webapp/WebappImpl.java
@@ +66,5 @@
> +    public BrowserDB.Factory getBrowserDBFactory() {
> +        return new BrowserDB.Factory() {
> +            @Override
> +            public BrowserDB get(String profileName, File profileDir) {
> +                return new StubBrowserDB(profileName);

This isn't strictly necessary (it's the default), but it's clearer.

Imagine that this whole method is just

  return StubBrowserDB.getFactory()

though!

Comment 25

[Richard Newman [:rnewman]]

Attachment: Folded patch.

Oh, and for the record: I plan to fold parts 1-3 together before landing. This is that folded patch, if you want to see how parts turned out.

Comment 26

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18b4632ef5c2

Comment 27

[Wesley Johnston (:wesj)]

Comment on attachment 8543079 [details] [diff] [review]
Part 2: address initial review comments, fix small bugs. v3

Review of attachment 8543079 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/db/BrowserDB.java
@@ +31,5 @@
>      }
>  
> +    public abstract BaseSearches getSearches();
> +    public abstract BaseTabsAccessor getTabsAccessor();
> +    public abstract BaseURLMetadata getURLMetadata();

Not this patch, but are these supposed to return BaseX, or just an X (i.e. an interface)?

::: mobile/android/base/db/LocalBrowserDB.java
@@ +518,5 @@
>  
>      private static Uri withDeleted(final Uri uri) {
> +        return uri.buildUpon()
> +                  .appendQueryParameter(BrowserContract.PARAM_SHOW_DELETED, "1")
> +                  .build();

All these withX() methods should probably become DBUtils methods. But I bet you just moved that to a different patch. Otherwise, "good first bug"!

@@ -518,5 @@
>              int limit, CharSequence urlFilter, String selection, String[] selectionArgs) {
>          // The combined history/bookmarks selection queries for sites with a url or title containing
>          // the constraint string(s), treating space-separated words as separate constraints
>          if (!TextUtils.isEmpty(constraint)) {
> -          String[] constraintWords = constraint.toString().split(" ");

final?

::: mobile/android/base/db/StubBrowserDB.java
@@ +73,5 @@
> +
> +    @RobocopTarget
> +    public Cursor filter(ContentResolver cr, CharSequence constraint, int limit,
> +                         EnumSet<BrowserDB.FilterFlags> flags) {
> +        return null;

Can you make sure we've got a note on the interface that this (and other methods) can return null? Most of them seem to be called from CursorLoader's which sometimes do null checks:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/SimpleCursorLoader.java#46

and sometimes don't:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TransitionAwareCursorLoaderCallbacks.java#19

but maybe the loader bails out somewhere before that... the Loader/CursorLoader interface may be the worst thing I've ever seen....

Bringing in the Android support annotations library is pretty easy and would give us @nullable support. We should do that....
http://tools.android.com/tech-docs/support-annotations

@@ +132,5 @@
> +        return null;
> +    }
> +
> +    private synchronized long getFolderIdFromGuid(final ContentResolver cr, final String guid) {
> +        return -1;

We should make this a const. good-first-bug?

@@ +263,5 @@
> +    public Cursor getTopSites(ContentResolver cr, int minLimit, int maxLimit) {
> +        return null;
> +    }
> +
> +    public static Factory getFactory() {

Do you use/need this?

Comment 28

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #27)

> Not this patch, but are these supposed to return BaseX, or just an X (i.e.
> an interface)?

That gets cleaned up in Part 3, where I re-did the stubbing to use a base interface and two implementations. The final code returns the interface for each method.

> All these withX() methods should probably become DBUtils methods. But I bet
> you just moved that to a different patch. Otherwise, "good first bug"!

Filed Bug 1118971. Thanks for the suggestion!
 
> final?

Yup!

> Can you make sure we've got a note on the interface that this (and other
> methods) can return null?

Yeah, I'll do a little audit and comment.

> but maybe the loader bails out somewhere before that... the
> Loader/CursorLoader interface may be the worst thing I've ever seen....

Damn straight.

> We should make this a const. good-first-bug?

Filed Bug 1118977.

> > +    public static Factory getFactory() {
> 
> Do you use/need this?

Yeah, we use it to override getBrowserDBFactory in WebappImpl.java.

Comment 29

[Wesley Johnston (:wesj)]

Comment on attachment 8543080 [details] [diff] [review]
Part 3: rework access to BrowserDB. v2

Review of attachment 8543080 [details] [diff] [review]:
-----------------------------------------------------------------

I don't love factories, and I don't quite get why we need this. i.e. rather than disabling/enabling the db directly now, this sets a different factory instead. What's the win? The rest of the renaming looks good though, and I don't have a strong enough hatred for factories to hold this up longer :)

aside: It would have been nice to split up so that that bit was easier to see and talk about though. hg qcref makes that much easier than git ;)

::: mobile/android/base/GeckoProfile.java
@@ +221,5 @@
> +                return profile;
> +            }
> +
> +            if (sAcceptDirectoryChanges) {
> +                if (AppConstants.RELEASE_BUILD) {

Any reason not to log this everywhere?

@@ +371,5 @@
>          // Even if this is a dir, it should now be empty and delete should work
>          return file.delete();
>      }
>  
> +    private GeckoProfile(Context context, String profileName, File profileDir, BrowserDB.Factory dbFactory) throws NoMozillaDirectoryException {

Why do you pass the factory here?

::: mobile/android/base/db/BrowserProvider.java
@@ +1366,5 @@
>                  + " AND " + Bookmarks.URL + " IS NOT NULL)";
>  
>          return deleteFavicons(uri, faviconSelection, null) +
>                 deleteThumbnails(uri, thumbnailSelection, null) +
> +               getURLMetadataTable().deleteUnused(getWritableDatabase(uri));

Make sure you file something to turn this List<Table> into a LinkedHashMap so that we can index but also walk it in order (I just realized iOS will have problems there too...)

::: mobile/android/base/db/LocalBrowserDB.java
@@ +98,5 @@
>      private final Uri mReadingListUriWithProfile;
>  
> +    private LocalSearches searches;
> +    private LocalTabsAccessor tabsAccessor;
> +    private LocalURLMetadata urlMetadata;

Why not leave these classes generic protocols? I guess they're hidden anyway...

::: mobile/android/base/db/TabsAccessor.java
@@ +20,5 @@
> +     * A thin representation of a remote client.
> +     * <p>
> +     * We use the hash of the client's GUID as the ID elsewhere.
> +     */
> +    public static class RemoteClient implements Parcelable {

This is really weird to me to have an entire subclass inside an interface. Can we pull it out?

::: mobile/android/base/webapp/WebappImpl.java
@@ +66,5 @@
> +    public BrowserDB.Factory getBrowserDBFactory() {
> +        return new BrowserDB.Factory() {
> +            @Override
> +            public BrowserDB get(String profileName, File profileDir) {
> +                return new StubBrowserDB(profileName);

Why isn't it just StubBrowserDB.getFactory()?

Comment 30

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #29)

Thanks for the review!


> I don't love factories, and I don't quite get why we need this. i.e. rather
> than disabling/enabling the db directly now, this sets a different factory
> instead. What's the win?

If we enable or disable, we hit painful concurrency issues. See Comment 6.

But we only ever enable or disable once, and it's a property of the app: BrowserApp wants a LocalBrowserDB, and GeckoView and WebappImpl don't want a BrowserDB at all.

We simply don't need the complexity of being able to turn off the BrowserDB at runtime, and we can improve clarity (and avoid scattered checks) just by using a different implementation when we create the profile.

This pass-a-factory approach is just the idiomatic Java way of allowing the caller to specify a particular implementation of an interface: "give me a GeckoProfile, and when you make the BrowserDB (which wants to know the profile name and profile dir, which are only calculated inside the constructor), please use this class".

This is way simpler in JS, of course.


> Any reason not to log this everywhere?

Only that eventually I want the non-release path to throw!


> > +    private GeckoProfile(Context context, String profileName, File profileDir, BrowserDB.Factory dbFactory) throws NoMozillaDirectoryException {
> 
> Why do you pass the factory here?

Just good design. Which kind of DB you get is an argument to a public static getter (a validating caching wrapper around the constructor).

sDBFactory is a convenience; you could just as easily imagine us getting rid of the three-argument GeckoProfile.get and requiring that the application provide a factory for every profile access.

But that would involve threading this knowledge throughout the app, wherever it needed a profile, and would thus infect GeckoView users. So we preserve the set-early-then-use-convenience-method approach, and avoid the need to touch callers that just want the app default.


> Make sure you file something to turn this List<Table> into a LinkedHashMap
> so that we can index but also walk it in order (I just realized iOS will
> have problems there too...)

Do you mean turn Table[] into LinkedHashMap?


> Why not leave these classes generic protocols? I guess they're hidden
> anyway...

Yeah, I like using the most specific type in the implementation. Accurately represents reality!


> This is really weird to me to have an entire subclass inside an interface.
> Can we pull it out?

Sure!


> Why isn't it just StubBrowserDB.getFactory()?

See Comment 24! Made that change after I uploaded this patch, so I just left a comment for you instead.

Comment 31

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=841797470180

Comment 32

[Richard Newman [:rnewman]]

This appears to only break one test, which is pretty good going:

19:34:21  WARNING -  TEST-UNEXPECTED-FAIL | testClearPrivateData | We could not move to the first item in the database - moveToFirst failed
19:34:21     INFO -  0 ERROR Exception caught during test! - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testClearPrivateData | We could not move to the first item in the database - moveToFirst failed
19:34:21     INFO -  	at junit.framework.Assert.fail(Assert.java:47)
19:34:21     INFO -  	at org.mozilla.gecko.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:128)
19:34:21     INFO -  	at org.mozilla.gecko.FennecMochitestAssert.ok(FennecMochitestAssert.java:150)
19:34:21     INFO -  	at org.mozilla.gecko.tests.DatabaseHelper.getBrowserDBUrls(DatabaseHelper.java:143)
19:34:21     INFO -  	at org.mozilla.gecko.tests.testClearPrivateData$1.test(testClearPrivateData.java:54)
19:34:21     INFO -  	at org.mozilla.gecko.tests.BaseTest$2.isSatisfied(BaseTest.java:338)
19:34:21     INFO -  	at com.jayway.android.robotium.solo.Waiter.waitForCondition(Waiter.java:370)
19:34:21     INFO -  	at com.jayway.android.robotium.solo.Solo.waitForCondition(Solo.java:426)
19:34:21     INFO -  	at org.mozilla.gecko.tests.BaseTest.waitForTest(BaseTest.java:335)
19:34:21     INFO -  	at org.mozilla.gecko.tests.testClearPrivateData.verifyHistoryCount(testClearPrivateData.java:51)
19:34:21     INFO -  	at org.mozilla.gecko.tests.testClearPrivateData.clearHistory(testClearPrivateData.java:38)

Comment 33

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/84aee740b55a
https://hg.mozilla.org/integration/fx-team/rev/20a66f476346
https://hg.mozilla.org/integration/fx-team/rev/5fbb06da804d
https://hg.mozilla.org/integration/fx-team/rev/cb1e6846d838

Comment 34

[Carsten Book [:Tomcat]]

sorry had to back this out for ongoing test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1661368&repo=fx-team

Comment 35

[Richard Newman [:rnewman]]

Guess someone had better fix Bug 997663 so I can land this!

Comment 36

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7c849b12b7e

Comment 37

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3de5562ed3bd

Comment 38

[Richard Newman [:rnewman]]

Attachment: Part 4: fix profile access in BrowserApp. v1

This ensures that we don't start off using the default profile, then switch to whichever one we were told to use in our initial intent.

Both guest mode and tests work with this change.

Comment 39

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e15b20fcce60

Comment 40

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/08c6d198e650
https://hg.mozilla.org/projects/cypress/rev/2ca02f5edc11
https://hg.mozilla.org/projects/cypress/rev/69755ce6a394
https://hg.mozilla.org/projects/cypress/rev/65d55b6ef0df

Comment 41

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/48f0e0b28a46
https://hg.mozilla.org/integration/fx-team/rev/3fc5da45d29e
https://hg.mozilla.org/integration/fx-team/rev/d4af48565428
https://hg.mozilla.org/integration/fx-team/rev/0211ab7ba0d8
https://hg.mozilla.org/integration/fx-team/rev/fbdcb7d48dd9

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48f0e0b28a46
https://hg.mozilla.org/mozilla-central/rev/3fc5da45d29e
https://hg.mozilla.org/mozilla-central/rev/d4af48565428
https://hg.mozilla.org/mozilla-central/rev/0211ab7ba0d8
https://hg.mozilla.org/mozilla-central/rev/fbdcb7d48dd9