Note: There are a few cases of duplicates in user autocompletion which are being worked on.

NullPointerException in BrowserProvider.getDatabasePath

RESOLVED FIXED

Status

()

Firefox for Android
General
P2
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rnewman, Assigned: wesj)

Tracking

(Depends on: 1 bug)

unspecified
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, fennec11+)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

6 years ago
Follow on from Bug 704490.

We're seeing a NPE fetching the profile DBs.

12-06 13:38:15.365: E/DatabaseUtils(1157): java.lang.NullPointerException
12-06 13:38:15.365: E/DatabaseUtils(1157):     at
org.mozilla.gecko.db.BrowserProvider.getDatabasePath(BrowserProvider.java:311)
12-06 13:38:15.365: E/DatabaseUtils(1157):     at
org.mozilla.gecko.db.BrowserProvider.getDatabaseHelperForProfile(BrowserProvider.java:287)
12-06 13:38:15.365: E/DatabaseUtils(1157):     at
org.mozilla.gecko.db.BrowserProvider.getReadableDatabase(BrowserProvider.java:347)
12-06 13:38:15.365: E/DatabaseUtils(1157):     at
org.mozilla.gecko.db.BrowserProvider.query(BrowserProvider.java:649)
12-06 13:38:15.365: E/DatabaseUtils(1157):     at
android.content.ContentProvider$Transport.query(ContentProvider.java:175)
12-06 13:38:15.365: E/DatabaseUtils(1157):     at
android.content.ContentProviderNative.onTransact(ContentProviderNative.java:112)
12-06 13:38:15.365: E/DatabaseUtils(1157):     at
android.os.Binder.execTransact(Binder.java:338)

This only happens if Fennec has never been launched. Hypothesis is that the profile directory doesn't exist.

Fix would be to check for null at the appropriate point, and either throw a suitable exception or (for the default profile) go through Fennec's usual profile creation code.
Priority: -- → P2
I've just noticed that this problem is more serious than it looks. We're storing local DB in the profile directory but this directory doesn't exist when Fennec launched for the first time. This results in Fennec crashing every time you launch it for the first time when using local DB because we're trying to access the profile directory before Gecko creates it.

We have two options here:

1. Simply store the databases the app directory instead of profile dir. We'd still have one database per profile but they wouldn't reside on their respective profile directories.

2. Create profile directory if it doesn't exist in BrowserProvider (before Gecko starts). I'm not sure if this is safe to do as I don't know how Gecko would handle a pre-existing profile directory created before it starts. Mark, Brad, any clues here?

I'd personally prefer option 1 for now as it's simple. Unless there's a simple way to do 2 or a bad enough drawback for doing 1.
Forgot to mention that the code for doing option 1 is already in place but it's only used in Android older than 2.3 due a API limitation (can't use absolute paths in SQLiteOpenHelper) on older releases. So, the patch here would be pretty much about enabling it for all cases.
I'd like to see option 2. We can create the folder (with a suitable name) and profiles.ini to make Gecko use the new folder.

Updated

6 years ago
Depends on: 690901
(Reporter)

Comment 4

6 years ago
Ping on this? Initial Sync setup will get a NPE if Fennec hasn't been run since installation.
Assignee: lucasr.at.mozilla → nobody
Assignee: nobody → wjohnston
(Assignee)

Comment 5

6 years ago
Created attachment 583342 [details] [diff] [review]
Patch

Patch to create a new profile directory if we don't have one. Uses a salt similar to the one firefox uses to create the dir name and generates profiles.ini with the new profile info filled in.

All seems fine here, but I don't have a way to build sync to test. rnewman, you want to give this a whirl for me?
Attachment #583342 - Flags: review?(mark.finkle)
(Reporter)

Comment 6

6 years ago
(In reply to Wesley Johnston (:wesj) from comment #5)

> All seems fine here, but I don't have a way to build sync to test. rnewman,
> you want to give this a whirl for me?

I'll give this a try in the morning (it's past 1am now, and I'm fading). Thanks, Wesley!
Comment on attachment 583342 [details] [diff] [review]
Patch


>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java

>-    static File sHomeDir = null;
>+    static public File sHomeDir = null;

You won't need this change if we move createProfile (and saltProfileName) to GeckoAppShell

>diff --git a/mobile/android/base/db/BrowserProvider.java.in b/mobile/android/base/db/BrowserProvider.java.in

>+import java.io.FileWriter;
>+import java.io.IOException;

>+import java.util.Date;

Move to GeckoAppShell

>+            Log.d(LOGTAG, "Creating directory for profile: " + profile);
>+            profileDir = createProfile(profile);

GeckoAppShell.createProfile(profile)

>+    static final char kTable[] = { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n',
>+                                   'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z',
>+                                   '1', '2', '3', '4', '5', '6', '7', '8', '9', '0' };
>+    private static File createProfile(final String profileName) {

Surprise! Let's move this handy utility to GeckoAppShell. We already have getProfileFolder() there and we should be able to call the code from this provider without Fennec running.

>+        // Profile directory may have been deleted or not created yet.
>+        File mozDir = new File(GeckoAppShell.sHomeDir, "mozilla");

Drop GeckoAppShell. after the move

>+        if (!mozDir.exists()) {
>+            mozDir.mkdir();
>+        }

{} not needed

>+        String saltedName = SaltProfileName(profileName);

SaltProfileName -> saltProfileName

>+    private static String SaltProfileName(final String aName)
>+    {

Put the { on the previous line

r+ cause the code looks good. Just fix the nits and move the methods to GeckoAppShell

Let's wait to land until Richard gives you a "thumbs up"
Attachment #583342 - Flags: review?(mark.finkle) → review+
(Reporter)

Comment 8

6 years ago
Comment on attachment 583342 [details] [diff] [review]
Patch

Assuming my rebuild didn't do something weird, the problem persists.

D/GeckoBrowserProvider(  682): Querying with URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
D/GeckoBrowserProvider(  682): Getting readable database for URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
D/GeckoBrowserProvider(  682): Getting database helper for profile: null
D/GeckoBrowserProvider(  682): No profile provided, using default
D/GeckoBrowserProvider(  682): Getting database path for profile: default
W/GlobalSession(  682): Aborting sync: Got session error.
W/GlobalSession(  682): java.lang.NullPointerException
W/GlobalSession(  682): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.getDatabasePath(BrowserProvider.java:425)
W/GlobalSession(  682): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.getDatabaseHelperForProfile(BrowserProvider.java:405)
W/GlobalSession(  682): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.getReadableDatabase(BrowserProvider.java:496)
W/GlobalSession(  682): 	at org.mozilla.fennec_rnewman.db.BrowserProvider.query(BrowserProvider.java:887)
W/GlobalSession(  682): 	at android.content.ContentProvider$Transport.query(ContentProvider.java:175)
W/GlobalSession(  682): 	at android.content.ContentResolver.query(ContentResolver.java:304)

The reason is most likely this:

        File profileDir = GeckoApp.mAppContext.getProfileDir(profile);

plus

    public static GeckoApp mAppContext;

That is: it's not (just) getFooPath returning null, it's the missing context (and, presumably, other static fields on GeckoApp).

GeckoApp.mAppContext is set in GeckoApp.onCreate, which is not invoked. Nor should it be -- that loads mozutils, shows about:home, etc. etc. (Naïvely it seems to me that a lot of that stuff should be in onStart or onResume, no?)

Fully initializing GeckoApp is probably not a valid solution to this problem: the content provider gets called from background threads at all hours of the day and night to perform synchronization, so the best approach is to lift all of the profile-creating code into a separate, dependency-free class and have both GeckoApp and the assorted Fennec content providers call *that* class.

That way you know there are no hidden dependencies on GeckoApp static fields that are scattered around Fennec's startup path.

Content providers should load and perform the absolute bare minimum to do their work. Otherwise we're going to catch flak for using too much battery.
Attachment #583342 - Flags: feedback-
(Assignee)

Comment 9

6 years ago
Created attachment 583600 [details] [diff] [review]
Patch

I'm not exactly sure this is the right way to get the org.mozilla.fennec_* context (which we need to get the home dir?). ContentProvider has a getContext method, which "Retrieves the Context this provider is running in." which seems to work.

Happy to look for alternatives if that's not right.

I also added a GeckoDirProvider, with some static helper methods. We're only using for this one main case right now (BrowserProvider.java). Then, because I'm lazy, I just reused it in GeckoApp so people calling the old method can use this new code. With a little fleshing out, we could change some code in GeckoAppShell from geckoApp.getExternalFilesDir() to GeckoDirProvider.getExternalFilesDir(geckoApp), but that didn't seem worth the effort.
Attachment #583342 - Attachment is obsolete: true
Attachment #583600 - Flags: review?(mark.finkle)
Attachment #583600 - Flags: feedback?(rnewman)
Comment on attachment 583600 [details] [diff] [review]
Patch


>diff --git a/mobile/android/base/GeckoDirProvider.java b/mobile/android/base/GeckoDirProvider.java
>+    private static String SaltProfileName(final String aName)
>+    {

* SaltProfileName -> saltProfileName
* Move the { up to the end of the function declaration

>diff --git a/mobile/android/base/Makefile.in b/mobile/android/base/Makefile.in
>   GeckoConnectivityReceiver.java \
>+	GeckoDirProvider.java \

Use 2 spaces not TAB 

>diff --git a/mobile/android/base/db/BrowserProvider.java.in b/mobile/android/base/db/BrowserProvider.java.in

>+import java.io.FileWriter;
>+import java.io.IOException;

>+import java.util.Date;

Do you still need these imports?

r+ assuming you tested this with Fennec not running and it worked, without starting Fennec
Attachment #583600 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 583616 [details] [diff] [review]
Patch

Grrr.. I forgot to hg qref that last patch. This is basically the same with mark's comments addressed and a little fix that makes this actually compile.
Attachment #583600 - Attachment is obsolete: true
Attachment #583600 - Flags: feedback?(rnewman)
Attachment #583616 - Flags: review+
Attachment #583616 - Flags: feedback?(rnewman)
(Reporter)

Comment 12

6 years ago
Comment on attachment 583616 [details] [diff] [review]
Patch

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

::: mobile/android/base/GeckoDirProvider.java
@@ +55,5 @@
> +        // XXX: TO-DO read profiles.ini to get the default profile
> +        return getProfileDir(aContext, "default");
> +    }
> +
> +    static public File getProfileDir(final Context aContext, final String profileName) {

This method does several things, but not consistently.

I encourage you to write a big block JavaDoc that explains:

* What are the allowed values for each argument
* What is the return value in which situations
* What side-effects occur in which situations.

Some of my thoughts are inline below.

I strongly encourage you to throw appropriate exceptions instead of returning null. You're just going to cause NPEs higher up the chain, right?

I suggest splitting the logic into these pieces, each of which can be a separate method:

* Ensure the Mozilla profiles directory exists. If it doesn't, and can't be created, then throw and give up, because we're screwed.
* Does a directory exist for the named profile? If it doesn't, return null. Throw for IO exceptions and all other exceptional cases.
* If a directory doesn't exist, nothing exceptional happened along the way, and we want it to exist, then create and return a directory for the profile. If you can't create a directory, throw -- never return null here.

So:

public static File getProfileDir(final Context aContext, final String profileName, boolean create)
  throws IllegalArgumentException, IOException {

  // Throws IllegalArgumentException, IOException
  File mozDir = GeckoDirProvider.ensureMozillaDirectory(aContext);

  // Returns null if success, but does not exist.
  File profileDir = GeckoDirProvider.getProfileDir(aContext, mozDir, profileName);
  if ((profileDir == null) && create) {
    // Throws if cannot create.
    return GeckoDirProvider.createProfileDir(aContext, mozDir, profileName);
  }
  return profileDir;
}

A nice UnableToCreateProfileDirectoryException with a reason would be a neat way to wrap those problems, too.

@@ +58,5 @@
> +
> +    static public File getProfileDir(final Context aContext, final String profileName) {
> +        Log.i(LOGTAG, "Get profile dir for " + profileName);
> +        if (aContext == null)
> +            throw new NullPointerException();

Please don't throw a NPE. Yes, checked exceptions suck, but callers of this method -- or callers of those! -- won't be expecting a NPE to be thrown here.

If you require aContext to be non-null, throw an IllegalArgumentException with a reason, and log what's going on.

@@ +62,5 @@
> +            throw new NullPointerException();
> +
> +        File filesDir = aContext.getFilesDir();
> +        if (filesDir == null)
> +            return null;

I don't believe this'll ever return null; it'll throw a FileNotFoundException instead.

Regardless, returning null isn't useful. This method as written returns null if:

* There's no filesDir in the context (this clause)
* createProfile returns null, which occurs if:
  * There's an IO error 
  * There's already a profiles.ini file.
* There's a fallthrough to the "directory exists but not profile" case.

It would be good to differentiate these to allow for sane handling.

@@ +66,5 @@
> +            return null;
> +
> +        File mozDir = new File(filesDir, "mozilla");
> +        if (!mozDir.exists())
> +            mozDir.mkdir();

Why are you not using

  http://developer.android.com/reference/android/content/Context.html#getDir%28java.lang.String,%20int%29

here?

"Retrieve, creating if needed, a new directory in which the application can place its own custom data files. You can use the returned File object to create and access files in this directory. Note that files created through a File object will only be accessible by your own application; you can only set the mode of the entire directory, not of individual files."

You're also not checking the boolean return value of mkdir! That means we can fail to create mozDir, which will cause listFiles to return null, and we'll call createProfile with a non-existent directory. Oops. This should be an error case.

@@ +78,5 @@
> +        if (profiles == null || profiles.length == 0)
> +            return createProfile(mozDir, profileName);
> +
> +        if (profiles != null && profiles.length > 0)
> +            return profiles[0];

Gosh, these lines look wrong.

listFiles returns null if there's an IO error or mozDir doesn't exist. You probably don't want to try to create a profile if there's an IO error or the parent directory doesn't exist, but that's exactly what you try to do.

If the directory does exist, but there are no returned profiles, you just return null.

So:

* If there's no profile directory, you create one and create a profile.
* If there is a profile directory, but this profile doesn't exist, you return null.

That means the first time you hit this method, the named profile is created, but the second time you get null and nothing happens, and the caller doesn't know why! That's inconsistent, and this method shouldn't be making that decision.

@@ +93,5 @@
> +        while (profile.exists()) {
> +            saltedName = saltProfileName(profileName);
> +            profile = new File(aRootDir, saltedName);
> +        }
> +        profile.mkdir();

You *have* to check the return value of mkdir.

@@ +111,5 @@
> +          }
> +        } else {
> +            // XXX: TO-DO If we already have an ini file, we should append the
> +            //      new profile information to it here
> +            return null;

So every subsequent attempt to create a profile is going to create the directory then fail silently?

If you're not going to do this work now, could you please file a follow-up bug and check for the profiles.ini file *before* creating the directory?!

@@ +117,5 @@
> +        return profile;
> +    }
> +
> +    private static String saltProfileName(final String aName) {
> +        Random randomGenerator = new Random(new Date().getTime());

System.nanoTime(), please. Two parallel calls in the same millisecond will get the same random seed.

@@ +122,5 @@
> +
> +        String salt = "";
> +        int i;
> +        for (i = 0; i < 8; ++i)
> +            salt += kTable[randomGenerator.nextInt(100) % kTable.length];

What's wrong with randomGenerator.nextInt(kTable.length) ?

You should also use a StringBuilder or a buffer with new String(buffer) instead of string concatenation.

@@ +124,5 @@
> +        int i;
> +        for (i = 0; i < 8; ++i)
> +            salt += kTable[randomGenerator.nextInt(100) % kTable.length];
> +
> +        return salt + "." + aName;

StringBuilder ftw.
Attachment #583616 - Flags: feedback?(rnewman) → feedback-
(Assignee)

Comment 13

6 years ago
Created attachment 583708 [details] [diff] [review]
Patch

This is Does this look better to you rnewman?

I think I've factored out most of this to throw... perhaps a bit to much. I'm ignoring when it does for the most part though.

I also noticed some strict mode warnings I wasn't seeing before(?) because I'm writing to a file on the main thread? I pushed writing profiles.ini to a background thread by using an actual Thread Object, something I really don't want to do... AsyncTask wasn't cooperating with me or my brain though.

If I really do that (or something else) I need to lock the profile dir somehow I guess. I think adding a "lock" file in it will do that?
Attachment #583616 - Attachment is obsolete: true
Attachment #583708 - Flags: feedback?(rnewman)
(Assignee)

Comment 14

6 years ago
Created attachment 583854 [details] [diff] [review]
Patch

Dang it. Hg patch queue fail. THIS has all the changes.
Attachment #583708 - Attachment is obsolete: true
Attachment #583708 - Flags: feedback?(rnewman)
(Assignee)

Updated

6 years ago
Attachment #583854 - Flags: feedback?(rnewman)
(Reporter)

Comment 15

6 years ago
Comment on attachment 583854 [details] [diff] [review]
Patch

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

I just skimmed this, because it's past 1am here. Generally the right direction, but you need to reorganize some of the logic still further, and more exception handling work to go.

You also need to make Fennec's startup be callback-driven. You can't synchronously perform I/O on the main thread on Android. If you aren't creating a ProfileLookupDelegate interface, you're probably not going far enough.

::: mobile/android/base/GeckoApp.java
@@ +785,5 @@
>      public File getProfileDir(final String profileName) {
>          if (mProfileDir == null && !mUserDefinedProfile) {
> +            try {
> +                mProfileDir = GeckoDirProvider.getProfileDir(mAppContext, profileName);
> +            } catch(IOException ex) {

Space before (.

@@ +791,1 @@
>              }

So you're going to muffle IOExceptions, causing the profile dir to be silently null, but other exceptions are going to trickle past.

Probably best to just make this method throw.

::: mobile/android/base/GeckoDirProvider.java
@@ +52,5 @@
> +abstract public class GeckoDirProvider
> +{
> +    private static final String LOGTAG = "GeckoDirProvider";
> +
> +    /* Get the default Mozilla profile directory for a given Activity instance.

Nit: use JavaDoc comment style.

/**
 * Get the default Mozilla profile directory for the given context.
 * ...
 * Throws if the profile directory could not be created.
 *
 * @param aContext
 *        The context ...
 * @return
 *        The profile directory. Will never be null.
 */

@@ +54,5 @@
> +    private static final String LOGTAG = "GeckoDirProvider";
> +
> +    /* Get the default Mozilla profile directory for a given Activity instance.
> +     * If no profiles currently exist, will create and return a profile directory.
> +     * Otherwise will return null.

I don't think this is sane logic. If we're asking for a profile directory, and we're not offered a create parameter, the profile should be created or an exception should be thrown.

Put differently: what is a caller to do if this returns null? Give up? Call again and hope for creation?

@@ +67,5 @@
> +    }
> +
> +    /* Get a particular profile directory for a given Activity instance.
> +     * If no profile directory currently exists, will create and return a profile directory.
> +     * Otherwise will return null;

Same as above.

@@ +85,5 @@
> +     *
> +     * Params:
> +     *   aContext - A context for the Activity we want a profile for. Must not be null.
> +     *   aProfileName - The name of the profile to open. Must be a non-empty string
> +     *   aCreate - The name of the profile to open. Must be a non-empty string

@param aCreate
       True if the profile should be created if it does not already exist.
       If false, and the profile does not exist, this method returns null.

@@ +143,5 @@
> +        File filesDir = aContext.getFilesDir();
> +
> +        File mozDir = new File(filesDir, "mozilla");
> +        if (!mozDir.exists() && aCreate)
> +            mozDir.mkdir();

Check return value.

@@ +205,5 @@
> +                } catch(IOException ex) {
> +                    Log.e(LOGTAG, "Error writing profiles.ini file");
> +                }
> +            }
> +        }).run();

I think you want .start() here.

You also want some kind of callback: if the write fails, your exception will be muffled in that thread with just a useless log message.

This is why I built Sync to be entirely asynchronous and callback-oriented. Lots of delegates, lots of boilerplate, but I don't have to stress about "oh, how am I supposed to avoid strict warnings about main thread IO?". You will have to do the same thing for Fennec.

We can't write JavaScript in Java, I'm afraid :/

@@ +211,5 @@
> +        return profile;
> +    }
> +
> +    private static String saltProfileName(final String aName) {
> +        Random randomGenerator = new Random(new Date().getTime());

System.nanoTime().

::: mobile/android/base/db/BrowserProvider.java.in
@@ +423,5 @@
>  
> +        File profileDir = null;
> +        try {
> +            profileDir = GeckoDirProvider.getProfileDir(mContext, profile, true);
> +        } catch(IOException ex) {

Space before (.

@@ +431,2 @@
>          if (profileDir == null) {
>              Log.d(LOGTAG, "Couldn't find directory for profile: " + profile);

You need to abort or throw in some way here. Otherwise you're going to roll right on through the rest of the code.

::: mobile/android/base/sync/syncadapter/SyncAdapter.java
@@ +132,4 @@
>    public Object syncMonitor = new Object();
>    private SyncResult syncResult;
>  
> +  public boolean shouldPerformSync = true;

*ahem*

::: mobile/android/sync/manifests/SyncAndroidManifest_activities.xml.in
@@ +10,5 @@
> +                <intent-filter>
> +                        <category android:name="android.intent.category.DEFAULT" />
> +                        <data android:scheme="sync" android:host="org.mozilla.android" android:path="/setup"/>
> +                </intent-filter>
> +        </activity>

This is part of your test code, a revert of my pref-off commit. Please don't include it in your final patch.
Attachment #583854 - Flags: feedback?(rnewman)
(Reporter)

Comment 16

6 years ago
One small bit of additional color, here:

> > +    private static String saltProfileName(final String aName) {
> > +        Random randomGenerator = new Random(new Date().getTime());
> 
> System.nanoTime().

... and even if you *do* want the current time in milliseconds since epoch, instantiating a Date object is an inefficient way to do it. Use System.currentTimeMillis().
Hardware: All → ARM
I am a bit worried about the complications we are adding here. Let's get something working using the minimal amount of code. Bug 711156 is showing a potential startup time regression from a single File.exists call that is always hit during startup. This code has the same call, and a few more.
Blocks: 711156
(Reporter)

Comment 18

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #17)
> I am a bit worried about the complications we are adding here. Let's get
> something working using the minimal amount of code. Bug 711156 is showing a
> potential startup time regression from a single File.exists call that is
> always hit during startup. This code has the same call, and a few more.

Can you clarify what you mean here? Fennec's main activity *cannot* control its database creation, nor can it perform main thread IO. Both of those need to be fixed. The current code is also wrong and unsafe in detail, so it doesn't particularly matter if it's fast or not IMO.
(Reporter)

Comment 19

6 years ago
(Expanding on my comment, now that I'm not trying to type on my phone.)

This is not to say that I don't think perf is important — I absolutely think that calls into the content provider interface should do as little work as possible, if only because each sync is going to involve hundreds of calls into the content provider.

Making this fast might require the use of flags in shared preferences or persisting an object in memory to avoid IO, and caching of DB handles, but that stuff is strictly an optimization on top of actually being correct. wesj's draft patch is really no different to the way Fennec currently works; it just puts the logic in a shared place.

Correct in this case means little stuff like checking return values, and big stuff like off-main-thread IO, and not depending on Fennec initialization to create profiles that back a content provider.
I wasn't as clear as I could be in my comment. We plan to use the code here for starting Fennec and accessing via a Content Provider. Both paths need to worry about performance, but the background Sync patch might need to worry about "startup" performance less than the Fennec app path.

The Fennec app startup path needs to make sure we avoid any pitfalls that are common enemies of speed. Doing File.exists() is an enemy of speed. We might be adding more enemies as well, in order to get the "get or create profile" behavior working. We just need to be mindful.

Should the code be correct and safe, but also fast. One example is this snippet:

> +        File filesDir = aContext.getFilesDir();
> +
> +        File mozDir = new File(filesDir, "mozilla");
> +        if (!mozDir.exists() && aCreate)
> +            mozDir.mkdir();

We'd like to remove the mozDir.exists() call altogether. In current Fennec, it's very likely causing a startup time regression. How can we do that? In JS code we have been skipping this call and letting try/catch handling deal with the failure. The one time it fails will be slow, but all the other times it will be faster.
(Reporter)

Comment 21

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #20)
> I wasn't as clear as I could be in my comment. We plan to use the code here
> for starting Fennec and accessing via a Content Provider. Both paths need to
> worry about performance, but the background Sync patch might need to worry
> about "startup" performance less than the Fennec app path.

Perhaps; perhaps not. At present our logs look like this, over and over:

  ...
  Querying with URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
  Getting readable database for URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
  Getting database helper for profile: null
  No profile provided, using default
  Successfully created database helper for profile: default
  Query is on bookmarks: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
  Using default sort order on query: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
  Finally running the built query: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
  GC_CONCURRENT freed 996K, 9% free 12901K/14023K, paused 4ms+13ms
  Query timer: AndroidBrowserRepositoryDataAccessor.fetch took 44ms.
  Calling insert on URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
  Getting writable database for URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
  Getting database helper for profile: null
  No profile provided, using default
  Successfully created database helper for profile: default
  Beginning insert transaction: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
  Calling insert in transaction on URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
  Getting writable database for URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
  Getting database helper for profile: null
  No profile provided, using default
  Successfully created database helper for profile: default
  Insert on BOOKMARKS: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?sync=true&show_deleted=true
  Extracting image values for URI: http://www.mozilla.com/en-US/firefox/community/
  Inserting bookmark in database with URL: http://www.mozilla.com/en-US/firefox/community/
  ...

so anything which avoids overhead on each CP call, or at the start of a sync, would be great. I haven't measured how much time in a sync is taken up with DB overhead, but there is definitely opportunity for optimization here. (See Bug 713542 for the Sync side. I haven't filed a bug for Fennec, and will not do so until some profiling has been done or someone complains! I'm still on the "make it work, make it right" part, and will get to "make it fast" as time allows.)


> The Fennec app startup path needs to make sure we avoid any pitfalls that
> are common enemies of speed. Doing File.exists() is an enemy of speed. We
> might be adding more enemies as well, in order to get the "get or create
> profile" behavior working. We just need to be mindful.

Agreed.


> We'd like to remove the mozDir.exists() call altogether. In current Fennec,
> it's very likely causing a startup time regression. How can we do that? In
> JS code we have been skipping this call and letting try/catch handling deal
> with the failure. The one time it fails will be slow, but all the other
> times it will be faster.

Firstly, all of this is necessary because Fennec is manually managing databases. A pure Android CP simply implements an onCreate method that takes an empty database and creates tables appropriately; Android itself takes care of when this needs to be called. You might be able to find a way to fit Fennec more tidily into the system, and thus take advantage of having the system manage these things for you.

If that's not an option, one approach is to be very direct: try to open the database from its complete path. If that succeeds, you don't need to do any of this extra work. The CP can additionally cache that database handle so that getReadable etc. are quick.

The quickest path is that your CP is already in memory, so you already have a handle on the DB. The second quickest path is that you go directly to the DB on the filesystem.

You can then make the fallback case thorough and safe: it'll only be called once.

(File.mkdir() on Unix is a native wrapper for system mkdir with 0777 permissions, so that'll fail if the directory already exists; the call to exists() is thus necessary.)

Another approach is to use shared preferences or some other system feature as a whiteboard: those prefs are in memory, so checking them will be somewhat quicker than going to the filesystem.


Regardless of all this, there are a lot of improvements that can be made to the code as it exists — such as checking return values, commenting, etc. — that won't impact the IO profile of the code.


Rambling more broadly:

I'm not too fussed whether as a short-term approach the existing code is patched up to cover most of my review comments, and the whole section rewritten in the next month with more of an eye toward perf and correctness, or if it gets done right first and lands in a week or two. It just needs to get done before we can unleash Sync on the unsuspecting public, because a NPE during your first sync isn't great. That was my primary goal in filing this bug; I honestly didn't intend to write paragraphs of review comments about the code.

I'm actually more worried that there's more Java code in the tree that's like this, presumably having slipped through code review under time pressure. This one bug covers one little method that will throw random exceptions because of unchecked return values, performs main thread IO, and reinvents existing efficient Android API calls. That's scary if it's representative.
(Assignee)

Comment 22

6 years ago
Created attachment 585577 [details] [diff] [review]
Patch

Blassey suggested just forcing a call to this very early in startup, and blocking by making it synchronized as an alternative to making this async.

I'm doing that in BrowserProvider.onCreate. That's the only place where I'm passing in aCreate = true so that we'll create the directory. We also shouldn't do any File.exists() calls unless aCreate is true. So that means we check for the file existing on startup which is kinda bad.

I tried to build a clever way around that so that it will at least only happen on first run startup (by first calling ensureMozillaDirectory with aCreate=false, then trying to get the profile directory from that potentially non-existent directory. if that fails we re-get the mozillaDirectory with aCreate=true). I'm a bit worried I'm missing something there though. The other calls happen on the main thread, but don't pass aCreate=true, and should not do any file i/o. 

> So you're going to muffle IOExceptions, causing the profile dir to be
> silently null, but other exceptions are going to trickle past.
> 
> Probably best to just make this method throw.

I can walk up the tree and make the callers aware of the throw, but most callers are already aware of the null return value, and I'm not sure what many of them are going to do to fix it. Seems like a lot of work for nothing. I'm curious what blassey thinks.

> This is part of your test code, a revert of my pref-off commit. Please don't
> include it in your final patch.

I left this in for anyone testing. I'll remove it before I push the patch.
Attachment #583854 - Attachment is obsolete: true
Attachment #585577 - Flags: review?(blassey.bugs)
(Assignee)

Comment 23

6 years ago
Created attachment 585600 [details] [diff] [review]
Patch

Same patch with the enable sync pieces yanked out.
Attachment #585577 - Attachment is obsolete: true
Attachment #585577 - Flags: review?(blassey.bugs)
Attachment #585600 - Flags: review?(blassey.bugs)
Comment on attachment 585577 [details] [diff] [review]
Patch

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

::: mobile/android/base/GeckoApp.java
@@ +443,1 @@
>                                      Log.e(LOGTAG, "onPrepareOptionsMenu: Unable to set icon", e);

this seems like more of a warning than and error, please change while you're here

@@ +783,3 @@
>  
>      public File getProfileDir(final String profileName) {
>          if (mProfileDir == null && !mUserDefinedProfile) {

if (mProfileDir != null)
    return mProfileDir; 

(I'll pretty much always prefer an early return)

Also, get rid of mUserDefinedProfile. Just set mProfileDir when -profile is passed in (see where mUserDefinedProfile gets set)

@@ +783,5 @@
>  
>      public File getProfileDir(final String profileName) {
>          if (mProfileDir == null && !mUserDefinedProfile) {
> +            try {
> +                // since this is on the main thread, do not attempt to create the directory

s/is/could be?

::: mobile/android/base/GeckoDirProvider.java
@@ +67,5 @@
> +     *       The profile directory.
> +     */
> +    static public File getProfileDir(final Context aContext, final Boolean aCreate)
> +            throws IllegalArgumentException, IOException {
> +        // XXX: TO-DO read profiles.ini to get the default profile

At this point since you're writing so much damn code, you really should just read the profiles.ini file

@@ +101,5 @@
> +            if (profileDir != null)
> +                return profileDir;
> +
> +            // we do not want to call File.exists on startup, so we first don't
> +            // attempt to create the mozilla directory.

I don't understand this, please explain

@@ +135,5 @@
> +    private static File ensureMozillaDirectory(final Context aContext, final Boolean aCreate)
> +            throws IOException, IllegalArgumentException {
> +        if (aContext == null)
> +            throw new IllegalArgumentException("Must provide a valid context");
> +        File filesDir = aContext.getFilesDir();

This does not match up with GeckoAppShell.sHomeDir (it can either be getFilesDir() or getExternalFilesDir()). Please use that directly.
Attachment #585577 - Attachment is obsolete: false
Attachment #585600 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 25

6 years ago
Created attachment 585865 [details] [diff] [review]
Patch

> Also, get rid of mUserDefinedProfile. Just set mProfileDir when -profile is
> passed in (see where mUserDefinedProfile gets set)
Fixed. Filed a follow up (below) to do this more correctly.

> At this point since you're writing so much damn code, you really should just
> read the profiles.ini file
filed bug 715307 for reading profiles.ini

> I don't understand this, please explain
I just removed this. These calls are coming from a background thread and I think we're fine as we can hope to be.

> This does not match up with GeckoAppShell.sHomeDir (it can either be
> getFilesDir() or getExternalFilesDir()). Please use that directly.
Moved this into GeckoDirProvider. I know you suggested creating a static init(aContext) in GeckoAppShell where we could initialize all the static variables it has? I started to do that and decided I've already written to much code for this patch, and I kinda like unifying where this dir stuff is.
Attachment #585577 - Attachment is obsolete: true
Attachment #585600 - Attachment is obsolete: true
Attachment #585865 - Flags: review?(blassey.bugs)
(Assignee)

Comment 26

6 years ago
Created attachment 585889 [details] [diff] [review]
Patch

We don't need aCreate really. In most cases, we are going to call this early on a background thread (either from the BrowserDBProvider or the profileMigrator). GeckoDirProvider will cache the directory and from then on we should have no worries.
Attachment #585865 - Attachment is obsolete: true
Attachment #585865 - Flags: review?(blassey.bugs)
Attachment #585889 - Flags: review?(blassey.bugs)
Comment on attachment 585889 [details] [diff] [review]
Patch

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

r+ with comments addressed, happy to re-review if you'd like though

::: mobile/android/base/GeckoApp.java
@@ +398,3 @@
>  
>          String args = intent.getStringExtra("args");
>          if (args != null && args.contains("-profile")) {

one thing I just realized is you need to set mLastUri, mLastTitle and mLastScreenshot to null if -profile is passed now because you're creating a placeholdersurface even when its a non-default profile

@@ +398,4 @@
>  
>          String args = intent.getStringExtra("args");
>          if (args != null && args.contains("-profile")) {
> +            Pattern p = Pattern.compile("(?:-profile\\s*)(\\w*)(\\s*)");

the part of the brain that handles regex's is broken for me, so I'm just going to trust that you've tested this.

@@ +400,5 @@
>          if (args != null && args.contains("-profile")) {
> +            Pattern p = Pattern.compile("(?:-profile\\s*)(\\w*)(\\s*)");
> +            Matcher m = p.matcher(args);
> +            if (m.find())
> +                getProfileDir(m.group(1));

looks like you can pass a path to the profile dir as the argument to -profile. Our automation passes:
'-no-remote -profile /mnt/sdcard/tests/profile/'

there are two flags to support:
-profile <path to profile>
-P <profile name>

::: mobile/android/base/GeckoDirProvider.java
@@ +66,5 @@
> +     *       The profile directory.
> +     */
> +    static public File getProfileDir(final Context aContext)
> +            throws IllegalArgumentException, IOException {
> +        // XXX: TO-DO read profiles.ini to get the default profile

put the bug number in the comment

@@ +98,5 @@
> +            if (profileDir != null)
> +                return profileDir;
> +
> +            // we do not want to call File.exists on startup, so we first don't
> +            // attempt to create the mozilla directory.

this comment still doesn't make sense. just remove it?

@@ +155,5 @@
> +        if (aProfileName == null || aProfileName.trim().equals(""))
> +            throw new IllegalArgumentException("Profile name: '" + aProfileName + "' is not valid");
> +
> +        // XXX: TO-DO If we already have an ini file, we should append the
> +        //      new profile information to it. For now we just throw an exception

file a follow up bug for this and put the bug number in the comment

::: mobile/android/base/db/BrowserProvider.java.in
@@ +518,5 @@
>  
>          synchronized (this) {
> +            GeckoAppShell.getHandler().post(new Runnable() {
> +                public void run() {
> +                    // Kick this off early. It is synchronized so that other callers will wait

move this out of the synchronized(this) block.
Attachment #585889 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 28

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b48ad2fa6178
Whiteboard: [inbound]
Backed out of inbound on suspicion of causing the talos and mochitest failures here (the original push's test run isn't appearing):
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=45e9963f21e9

https://hg.mozilla.org/integration/mozilla-inbound/rev/93fb2f8cd624

Updated

6 years ago
Whiteboard: [inbound]
(Assignee)

Comment 30

6 years ago
I think the issue here is us setting mLastUri, mLastTitle, and mLastViewport = null. Removing that locally fixes tests for me. Just pushed to try with a patch that restore mUserDefinedProfile instead.
(Assignee)

Comment 31

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/82883346ddfe

This was happy on try. Hopefully it also enjoys inbound.
Whiteboard: [inbound]
tracking-fennec: --- → 11+
(Assignee)

Comment 32

6 years ago
https://hg.mozilla.org/mozilla-central/rev/82883346ddfe
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
status-firefox11: --- → affected
status-firefox12: --- → fixed
Comment on attachment 585889 [details] [diff] [review]
Patch

[Approval Request Comment]
Core to enable local history/bookmark DBs and enable Sync
Attachment #585889 - Flags: approval-mozilla-aurora?
Comment on attachment 585889 [details] [diff] [review]
Patch

[Triage Comment]
Mobile only - approving for Aurora.
Attachment #585889 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/d59b0dbcef0f
status-firefox11: affected → fixed
You need to log in before you can comment on or make changes to this bug.