735461 - Robocop: Fennec does not interpret -profile argument correctly

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Geoff Brown [:gbrown]]

When Fennec is started for a robocop test, it appears to contain the bookmarks, history, and top sites associated with the default profile on the phone.

Robocop creates, populates, and destroys its own profile directory at <tests>/profile, eg. /mnt/sdcard/tests/profile ... but it doesn't seem to use it!

A typical command line for launching Fennec from Robocop, as seen by devicemanagerADB.launchProcess:

['am', 'instrument', '-w', '-e', 'class', 'org.mozilla.fennec_mozdev.tests.testLoad', 'org.mozilla.roboexample.test/android.test.InstrumentationTestRunner']

Note that there is no -profile argument.

Comment 1

[Geoff Brown [:gbrown]]

remoteautomation is aware of the remote profile, but ignores it, here:

https://hg.mozilla.org/mozilla-central/annotate/956e2f735262/build/mobile/remoteautomation.py#l125

...so it almost looks intentional...but I can't think of why.

Comment 2

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #1)
> ...so it almost looks intentional...but I can't think of why.

Oh, of course -- this is just starting InstrumentationTestRunner, which wouldn't know what to do with -profile. The -profile stuff is in:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/BaseTest.java.in#54

Comment 3

[Geoff Brown [:gbrown]]

Robocop tests specify "-profile <dir>" on the Fennec command line, but there is some confusion in Fennec about the interpretation of these arguments. In Gecko (nsAppRunner), I think -profile <dir> is interpreted as intended -- use the specified directory as the profile directory.

But since bug 708651, Fennec (GeckoApp / GeckoProfile) interprets <dir> as a profile name:
https://hg.mozilla.org/mozilla-central/annotate/c71845b3b2a6/mobile/android/base/GeckoApp.java#l1657

Complicating issues, '/' is not expected in the profile name, so the -profile argument is mostly ignored.

Comment 4

[Geoff Brown [:gbrown]]

Attachment: wip patch

Comment 5

[Geoff Brown [:gbrown]]

More considerations:
1. Apparently we need to support both -profile <profile-dir> and -P <profile-name>
2. Database data is stored outside of the profile.

Comment 6

[Geoff Brown [:gbrown]]

Attachment: patch 1/2: support -profile and -P command line arguments

This accepts both -profile <profile-dir> and -P <profile-name> arguments. The profile name is used by BrowserDB to select a database name. This allows a test harness to select a database other than the default.

Comment 7

[Geoff Brown [:gbrown]]

Attachment: patch 2/2: update robocop to use and cleanup its own DB with the -P argument

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 610014 [details] [diff] [review]
patch 2/2: update robocop to use and cleanup its own DB with the -P argument

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

let's not change the tests to work around this bug, instead fix the bug

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 610013 [details] [diff] [review]
patch 1/2: support -profile and -P command line arguments

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

this code looks approximately correct, but as we just discussed over vidyo LocalBrowserDB needs to base the content provider URI on the profile path not the profile name

Comment 10

[Geoff Brown [:gbrown]]

Attachment: suppo

Comment 11

[Geoff Brown [:gbrown]]

Attachment: support -profile and -P command line arguments

(Sorry - fingers slipped!)

I feel better about this patch. Now the test strategy stays the same and none of the test harness code needs to change: tests pass -profile <dir> and then clean up <dir> when done. When -profile is specified, the database is stored in the profile directory, so now history, bookmarks, and topsites are clean when running robocop tests.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 610948 [details] [diff] [review]
support -profile and -P command line arguments

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

r- to see another patch that drops the BrowserContract.PARAM_PROFILE

::: mobile/android/base/GeckoApp.java
@@ +1621,5 @@
>          }
>  
> +        if (profilePath != null) {
> +            BrowserDB.init((String)null, profilePath);
> +        } else {

if (profilePath != null)
    BrowserDB.init((String)null, profilePath);
else if (profileName != null)
    BrowserDB.init(profileName, (String)null);
else
    BrowserDB.init(BrowserContract.DEFAULT_PROFILE, (String)null);

or... add these checks in BrowserDB.init and just pass profileName and profileDir directly

::: mobile/android/base/db/LocalBrowserDB.java
@@ +142,5 @@
>      private Uri appendProfile(Uri uri) {
> +        if (mProfilePath != null) {
> +            return uri.buildUpon().appendQueryParameter(BrowserContract.PARAM_PROFILE_PATH, mProfilePath).build();
> +        }
> +        return uri.buildUpon().appendQueryParameter(BrowserContract.PARAM_PROFILE, mProfileName).build();

let's drop the BrowserContract.PARAM_PROFILE constant entirely and do a look up of the profile path from the profile name if the profile path is null.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 610948 [details] [diff] [review]
support -profile and -P command line arguments

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

r- to see another patch that drops the BrowserContract.PARAM_PROFILE

::: mobile/android/base/GeckoApp.java
@@ +1621,5 @@
>          }
>  
> +        if (profilePath != null) {
> +            BrowserDB.init((String)null, profilePath);
> +        } else {

if (profilePath != null)
    BrowserDB.init((String)null, profilePath);
else if (profileName != null)
    BrowserDB.init(profileName, (String)null);
else
    BrowserDB.init(BrowserContract.DEFAULT_PROFILE, (String)null);

or... add these checks in BrowserDB.init and just pass profileName and profileDir directly

::: mobile/android/base/db/LocalBrowserDB.java
@@ +142,5 @@
>      private Uri appendProfile(Uri uri) {
> +        if (mProfilePath != null) {
> +            return uri.buildUpon().appendQueryParameter(BrowserContract.PARAM_PROFILE_PATH, mProfilePath).build();
> +        }
> +        return uri.buildUpon().appendQueryParameter(BrowserContract.PARAM_PROFILE, mProfileName).build();

let's drop the BrowserContract.PARAM_PROFILE constant entirely and do a look up of the profile path from the profile name if the profile path is null.

Comment 14

[Geoff Brown [:gbrown]]

Attachment: support -profile and -P command line arguments

> if (profilePath != null)
>     BrowserDB.init((String)null, profilePath);
> else if (profileName != null)
>     BrowserDB.init(profileName, (String)null);
> else
>     BrowserDB.init(BrowserContract.DEFAULT_PROFILE, (String)null);

Good idea - thanks!

> let's drop the BrowserContract.PARAM_PROFILE constant entirely and do a look 
> up of the profile path from the profile name if the profile path is null.

This is more complicated than you might expect. There is additional logic in the provider that sometimes uses a different path than the directory associated with the named profile:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#921

That logic could be moved into LocalBrowserDB (or perhaps all the way back to GeckoApp, since we also need a Context to use the GeckoProfile), but I think it's better (more flexible) to keep it the way it is:
 - LocalBrowserDB provides the profile request info (name or path)
 - the provider determines the appropriate database location for the request

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Geoff Brown [:gbrown] from comment #14)
> > let's drop the BrowserContract.PARAM_PROFILE constant entirely and do a look 
> > up of the profile path from the profile name if the profile path is null.
> 
> This is more complicated than you might expect. There is additional logic in
> the provider that sometimes uses a different path than the directory
> associated with the named profile:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/
> BrowserProvider.java.in#921]
here is the code in question:
921     public String getDatabasePath(String profile, boolean isTest) {
922         trace("Getting database path for profile: " + profile);
923 
924         // On Android releases older than 2.3, it's not possible to use
925         // SQLiteOpenHelper with a full path. Fallback to using separate
926         // db files per profile in the app directory.
927         if (isTest) {
928             return DATABASE_NAME;
929         } else if(Build.VERSION.SDK_INT <= 8) {
930             return "browser-" + profile + ".db";
931         }
932 
933         File profileDir = GeckoProfile.get(mContext, profile).getDir();
<snip>

How about we delete lines 927 - 931 and add this after 933:

if (Build.VERSION.SDK_INT <= 8)
    return "browser-" + profileDir..getAbsolutePath().replace("/", "_") + ".db";

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 611989 [details] [diff] [review]
support -profile and -P command line arguments

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

clearing the review based on my previous comment.

Comment 17

[Geoff Brown [:gbrown]]

Attachment: suppor -profile and -P command line arguments

> if (Build.VERSION.SDK_INT <= 8)
>     return "browser-" + profileDir..getAbsolutePath().replace("/", "_") + ".db";

That gives us a unique name, but it's not the same name: so older phones will lose their existing history / bookmarks / etc. - is that going to be okay?


In testing this, I realized that the older phones, including the tegras, will have their DB location constrained to something outside of the requested -profile. So this patch will change the tests from using the default database to using a test database...but that test database will not be cleaned up - not much of a win. More work to do...

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Geoff Brown [:gbrown] from comment #17)
> Created attachment 612355 [details] [diff] [review]
> suppor -profile and -P command line arguments
> 
> > if (Build.VERSION.SDK_INT <= 8)
> >     return "browser-" + profileDir..getAbsolutePath().replace("/", "_") + ".db";
> 
> That gives us a unique name, but it's not the same name: so older phones
> will lose their existing history / bookmarks / etc. - is that going to be
> okay?
Yup, it will only effect nightly and aurora users. Also, anyone wiht sync set up will recover.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Geoff, since this seems to have stalled out let's get the fix landed for everything but Froyo and file a follow up but to fix Froyo.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Making sure that Wes and Vlad are aware of the changes since it affects profile and startup params. Chromeless activities might be affected by these changes (maybe in a positive way).

See bug 715307 for a patch about reading the profile from profile.ini
See bug 740586 about the chromeless window patch

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This looks like it has part of the patch from bug 715307 in it, right?

Comment 22

[Geoff Brown [:gbrown]]

Attachment: support -profile and -P command line arguments

Previous patch updated for bitrot.

Comment 23

[Geoff Brown [:gbrown]]

(In reply to Vladimir Vukicevic (:vlad) from comment #21)
> This looks like it has part of the patch from bug 715307 in it, right?

I don't think so -- it looks different to me. I was unaware of that bug before today.

Comment 24

[Geoff Brown [:gbrown]]

Comment on attachment 628004 [details] [diff] [review]
support -profile and -P command line arguments

So this is the patch exactly as it was during the last round of review comments, just updated for bitrot.

Looking at it with fresh eyes, I feel better about it. It doesn't break any tests. For Gingerbread and beyond, it fixes the -profile handling; for Froyo, it does not but we can come back to that...and also it changes the db name, causing a one-time loss of history. Good enough?

Comment 25

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #24)
> ...It doesn't break any tests. 

Now I am not so sure. I have good results when I run tests locally, but a try run is showing random failures in robocop (mochitest-robotium): https://tbpl.mozilla.org/?tree=Try&rev=0dd1c4608640

Comment 26

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #25)
> try run is showing random failures in robocop (mochitest-robotium):

I have investigated testBookmark and testBookmarklet failures. BrowserProvider sometimes gets a uri that does not contain profilePath= and that isn't handled well, typically resulting in a NullPointerException.

Comment 27

[Geoff Brown [:gbrown]]

Attachment: support -profile and -P arguments

Here's a new, simpler approach: When -profile <path> is specified, create a GeckoProfile entry for {name="default", path=<path>} and leave BrowserProvider, etc untouched. 

With the current code, this does not solve our testing problem on the tegras, because databases are created outside of the profile on Froyo. However, with the proposed patch for bug 768532, testing shows that the database moves to the requested path of /mnt/sdcard/tests/profile.

Comment 28

[Geoff Brown [:gbrown]]

Try run is not as green as I would like, but I think it is okay (I'll keep testing):

https://tbpl.mozilla.org/?tree=Try&rev=815db8392a4d

Comment 29

[Geoff Brown [:gbrown]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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