768532 - Database located outside of profile, pre-gingerbread

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Geoff Brown [:gbrown] (pto Mar 24)]

mochitest and other test frameworks specify -profile <profile-path> when launching Fennec to direct that a specific profile is used for tests. The profile is normally created by the test framework and cleaned up by the test framework using simple file operations (rm -r <profile-path>). 

One reason for using -profile is to ensure that a test or group of tests run with a known profile that is consistent from one run to the next. This is currently compromised on pre-Gingerbread (including the Tegras that run all production tests) because the database is located outside of the profile.

BrowserProvider.getDatabasePath() has this code:

        // On Android releases older than 2.3, it's not possible to use
        // SQLiteOpenHelper with a full path. Fallback to using separate
        // db files per profile in the app directory.
        if (isTest) {
            return DATABASE_NAME;
        } else if(Build.VERSION.SDK_INT <= 8) {
            return "browser-" + profile + ".db";
        }

isTest is used by some robocop tests to work-around the restriction for those particular tests.

It is not immediately obvious to me why "On Android releases older than 2.3, it's not possible to use SQLiteOpenHelper with a full path". If we can remove this restriction, we won't need to update all the test frameworks to delete browser-default.db between tests...and we might even be able to remove the isTest complexity.

(Note that there are currently other problems with -profile, such as argument parsing -- see bug 735461.)

Comment 1

[Lucas Rocha (:lucasr)]

(In reply to Geoff Brown [:gbrown] from comment #0)
> mochitest and other test frameworks specify -profile <profile-path> when
> launching Fennec to direct that a specific profile is used for tests. The
> profile is normally created by the test framework and cleaned up by the test
> framework using simple file operations (rm -r <profile-path>). 
> 
> One reason for using -profile is to ensure that a test or group of tests run
> with a known profile that is consistent from one run to the next. This is
> currently compromised on pre-Gingerbread (including the Tegras that run all
> production tests) because the database is located outside of the profile.

The reason I didn't use a separate profile was that we didn't support it very well by the  time I wrote the BrowserProvider tests. What value is passed to this -profile option? A path or a profile name? BrowserProvider currently takes a profile name as argument.

Is the -profile argument actually used in Fennec's front-end throughout? I mean, will the passed profile be properly used by the app? Last time I checked, we were not doing anything with it. Maybe this is different now?
 
> BrowserProvider.getDatabasePath() has this code:
> 
>         // On Android releases older than 2.3, it's not possible to use
>         // SQLiteOpenHelper with a full path. Fallback to using separate
>         // db files per profile in the app directory.
>         if (isTest) {
>             return DATABASE_NAME;
>         } else if(Build.VERSION.SDK_INT <= 8) {
>             return "browser-" + profile + ".db";
>         }
> 
> isTest is used by some robocop tests to work-around the restriction for
> those particular tests.
> 
> It is not immediately obvious to me why "On Android releases older than 2.3,
> it's not possible to use SQLiteOpenHelper with a full path". If we can
> remove this restriction, we won't need to update all the test frameworks to
> delete browser-default.db between tests...and we might even be able to
> remove the isTest complexity.

Looking at the code, I think there's a bug there. If I'm not mistaken, Android 8 (2.2.*) does support full database paths in SQLiteOpenHelper. So, we should probably turn "<= 8" into "< 8" for normal runs (isTest == false && Android < 8). If you run this change on Try and it works for you, feel free to send a patch. This should cover all UI tests.

However, the reason I special-cased the database path on tests (isTest == true) is that we use an IsolatedContext environment to run content provider tests (see ContentProvideTest.java) and there's a bug in Android's RenamingDelegatingContext that doesn't rename full database paths correctly. The ContentProvideTest infra is meant to be used on UI-less tests that should not affect (or be affected by) UI stuff. ContentProviderTest takes care of removing the database file.

In summary: if you confirm that changing from Android <= 8 to Android < 8 works fine on Try, this should probably make all UI tests behave as you want (all files inside the profile directory). For content provider tests, I don't think we should change anything because we are already running them in an isolated context, the way it should be.

Comment 2

[Geoff Brown [:gbrown] (pto Mar 24)]

(In reply to Lucas Rocha (:lucasr) from comment #1)
> (In reply to Geoff Brown [:gbrown] from comment #0)
> The reason I didn't use a separate profile was that we didn't support it
> very well by the  time I wrote the BrowserProvider tests. What value is
> passed to this -profile option? A path or a profile name? BrowserProvider
> currently takes a profile name as argument.
>
> Is the -profile argument actually used in Fennec's front-end throughout? I
> mean, will the passed profile be properly used by the app? Last time I
> checked, we were not doing anything with it. Maybe this is different now?
 
It's -profile <path> is the traditional usage in desktop firefox and in the test suites. Currently we don't use it -- but that's changing in bug 735461 (new patch on the way for that). I intend to do use GeckoProfile to associate the specified profile path with a "default" profile name -- and BrowserProvider should be able to continue to work with profile names.
 
> In summary: if you confirm that changing from Android <= 8 to Android < 8
> works fine on Try, this should probably make all UI tests behave as you want
> (all files inside the profile directory). For content provider tests, I
> don't think we should change anything because we are already running them in
> an isolated context, the way it should be.

Agreed.

Here's an initial try run: https://tbpl.mozilla.org/?tree=Try&rev=5d0ce9f29740

It's not as green as I would like, but a lot of tests seem to be failing today in other builds too....I'll keep testing, but I think the change to < 8 is okay!

Comment 3

[Geoff Brown [:gbrown] (pto Mar 24)]

Attachment: use database path on Froyo

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 637663 [details] [diff] [review]
use database path on Froyo

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

I think we need a migration path for Froyo users in the field. We have to move the current out-of-profile db file to the profile directory for existing users. Otherwise their history and bookmarks will simply disappear after an update. Maybe this code could be added to the ProfileMigrator.

Comment 5

[Brian Nicholson (:bnicholson)]

This bug is the reason the test case for bug 811445 is failing. Using the posted patch, the test does pass. I'll see if I can add the upgrade logic to this.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Use profile database path on Android 2.2

Modified version of gbrown's patch, which checks for the existence of the old database when getDatabaseHelperForProfile() is called. We can't use the onUpgrade() path to do this since we need to move the database before we even get a handle to it. The exists() call shouldn't be too expensive since we only do this once per session; future calls to getDatabaseHelperForProfile() will return the existing dbHelper.

Side note - I've noticed a bug in our existing code where the following could happen:
1) The user has Froyo and installs Firefox
2) The Firefox profile is created in the old location
3) The user upgrades to >Android 2.2
4) Firefox now checks for the in-profile location

I haven't seen many bug reports about disappearing profiles, so maybe this isn't a big issue.

Comment 7

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> 2) The Firefox profile is created in the old location

s/profile/database/

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> Side note - I've noticed a bug in our existing code where the following
> could happen:
> 1) The user has Froyo and installs Firefox
> 2) The Firefox profile is created in the old location
> 3) The user upgrades to >Android 2.2
> 4) Firefox now checks for the in-profile location
> 
> I haven't seen many bug reports about disappearing profiles, so maybe this
> isn't a big issue.

I guess this is because upgrades are rare on devices running 2.2 these days.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 686324 [details] [diff] [review]
Use profile database path on Android 2.2

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

Looks good. Just want to understand why checking for Android < 8 still makes sense before giving r+.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1689,5 @@
>  
>              // When running inside a test or on Android releases older than 8,
>              // the returned database path is just filename, not the full path.
>              // We need the full path when unlocking the database.
> +            if (isTest || Build.VERSION.SDK_INT < 8) {

We don't really support Android < 8, I guess you can remove this version check here?

::: mobile/android/base/db/TabsProvider.java.in
@@ +241,5 @@
>  
>              // When running on Android releases older than 8, the returned
>              // database path is just filename, not the full path. We need
>              // the full path when unlocking the database.
> +            if (Build.VERSION.SDK_INT < 8) {

Do we actually support Android versions older than 8? This check is moot otherwise.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #9)
> Comment on attachment 686324 [details] [diff] [review]
> Use profile database path on Android 2.2
> 
> Review of attachment 686324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Just want to understand why checking for Android < 8 still makes
> sense before giving r+.

Yeah, I was thinking the same thing last night. I'll just remove these bits altogether.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Use profile database path on Android 2.2, v2

Removes pre-Android 2.2 code since we don't support those devices.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 686651 [details] [diff] [review]
Use profile database path on Android 2.2, v2

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

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1683,5 @@
> +                File oldPath = mContext.getDatabasePath("browser-" + profile + ".db");
> +                if (oldPath.exists()) {
> +                    oldPath.renameTo(new File(databasePath));
> +                }
> +            }

Don't you have to add this same code on TabsProvider too?

Comment 13

[Geoff Brown [:gbrown] (pto Mar 24)]

Comment on attachment 686651 [details] [diff] [review]
Use profile database path on Android 2.2, v2

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

Thanks much Brian. This looks quite tidy. I agree with Lucas; r+ with his comment addressed.

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: Use profile database path on Android 2.2, v3

I was going to factor these methods into the DBUtils class, but the isTest check makes it kind of messy.

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 686739 [details] [diff] [review]
Use profile database path on Android 2.2, v3

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

Looks good.

Comment 16

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fefd7cb475aa

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fefd7cb475aa