740218 - Fennec form history content provider insert is slow on Transformer Prime

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Nick Alexander :nalexander [he/him]]

Attachment: Patch to add timing around insert

On my Transformer Prime it takes about 4s (wall clock) per insert.  That's in a bulkInsert, so the CP should be reachable the whole time and no process switching should happen.

Attached patch and log demonstrate timing.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Log of inserting

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #1)
> Created attachment 610372 [details]
> Log of inserting

Sorry, this log was not in a bulkInsert.  (I have tried in a bulkInsert, which does 
not improve matters.)

The relevant times, in milliseconds:

    763:W FOO2(3637)                  insert db.insert 5786
    788:W FOO2(3637)                  insert db.insert 4391
    814:W FOO2(3637)                  insert db.insert 3689
    840:W FOO2(3637)                  insert db.insert 4332
    865:W FOO2(3637)                  insert db.insert 4371
    893:W FOO2(3637)                  insert db.insert 4345
    919:W FOO2(3637)                  insert db.insert 4379
    944:W FOO2(3637)                  insert db.insert 4322

Comment 3

[Richard Newman [:rnewman]]

This will cause first syncs to fail if the user has more than 75 form entries. (I have about two thousand.) Aiee! :D

Comment 4

[Richard Newman [:rnewman]]

Just so it's clear: the three choices we have here are:

* Fix this bug
* Find the list of devices which are slow, and blacklist them for Fennec
* Pref off form history sync for beta.

We cannot ship form history sync if inserts take more than ~10msec, because the Sync thread will get killed by the OS before it finishes, and thus you'll never have a successful sync. We'll sit there spinning for three hours, getting killed each time, maybe making slow progress.

At 4,000msec per insert, I wouldn't even get 5% through syncing my profile.

Comment 5

[Nick Alexander :nalexander [he/him]]

+1 for further testing to find out if it is device specific.

Comment 6

[Nick Alexander :nalexander [he/him]]

Wes, can you weigh in with: what interface Android Sync should be using to get best perf -- specifically, is there a batching interface that we should be using?

What can be done from your end to improve this?  liuche is having perf problems with password inserts as well.

Comment 7

[Wesley Johnston (:wesj)]

I don't think there's a batching interface. This will have to be done on our end.

This was marked blocking-. Should it be higher priority or was that a statement that we don't really care about form history sync? Password sync I expect has less entries than form history making this point a bit less important for it?

I'm going to try and reproduce this on my end when I get a chance, and get some profiling data, but it seems likely the problem is due to us open and closing the database for every call. If that turns out to be the problem, we'll need to hold the open database connection until the client asks us to close it.

Comment 8

[Tony Chung [:tchung]]

There is now more information since comment 4 to support another triage look.  Renom'ing.

Comment 9

[Wesley Johnston (:wesj)]

Update, I wrote some tests to time the content provider on my phone (a DroidX) by calling into it from GeckoApp (same process). The results show my first few inserts taking around 500ms each, and subsequent ones taking 170-200ms. nalexander ran the same tests on the TransformPrime and found the first few inserts taking a 1-2s, but subsequent ones taking ~100ms.

I did some additional timing on my phone, and of those 100-400ms times, 90% of the time is spent in sqlite3_step (i.e. not spent opening the database file).

PasswordProvider runs in its own process, so I'm going to repeat the test with it and see if I can reproduce the slowness being introduced by IPC.

Comment 10

[Richard Newman [:rnewman]]

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

> I did some additional timing on my phone, and of those 100-400ms times, 90%
> of the time is spent in sqlite3_step (i.e. not spent opening the database
> file).

That's still much longer than we'd really like; our total _end to end_ wall clock times for bookmark inserts through that CP consistently average around 70ms, so that's the ballpark for which I'd like to aim.

But 200ms is way better than 4000ms!

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch 1/4

I'm going to do this in pieces. This one just allows us to hold a reference to an open database in java.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch 2/4 - Transactions

Support for transactions in sqlitebridge

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch 3/4 - Bulkinsert

OK. I was going to bust this in two, but I think that's a bit silly. This switches our content providers to use the new sqlitebridge stuff, and implements bulkInsert wrapped in a transaction. With this, inserts on my phone drop to ~50ms each.

I'm not sure the right way to close this when the content provider is "closed" because they don't really have a close method. We could put something in finalize(), but no one recommends doing that ever. I implemented this shutdown method, but I'm not convinced its ever called by anyone (I haven't seen any logging from it ever appear). I'll keep digging. Lucas, you have any ideas?

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch 5/4 - Use transactions on insert

I should say, bulkInserts on my phone drop to about 30-50ms/insert. We're not really getting any boost at all for normal inserts.

Those actually do an additional "DELETE FROM table WHERE guid = ?" call as well, and because of that they should be wrapped in a transaction anyway. This is hacky though, because I don't want to add support for nested transactions yet (we probably should though!).

Comment 15

[Richard Newman [:rnewman]]

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

> OK. I was going to bust this in two, but I think that's a bit silly. This
> switches our content providers to use the new sqlitebridge stuff, and
> implements bulkInsert wrapped in a transaction. With this, inserts on my
> phone drop to ~50ms each.

Lovely!

> I'm not sure the right way to close this when the content provider is
> "closed" because they don't really have a close method. We could put
> something in finalize(), but no one recommends doing that ever. I
> implemented this shutdown method, but I'm not convinced its ever called by
> anyone (I haven't seen any logging from it ever appear). I'll keep digging.
> Lucas, you have any ideas?

shutdown exists so that test code can clean up. Android will ordinarily manage the lifecycle of your CP for you.

This kinda sucks in Android. The usual recommendation is "always close the database when you're done with it", but that's pretty awful for performance reasons. Other hacks include returning cursors that automatically close the DB when they're done.

In your case I would suggest doing both: implement a shutdown method, have both it and the finalizer call a thread-safe, super-paranoid method (e.g., check for nulls!) that closes your DB handles.

(Don't just call shutdown from the finalizer; you don't need or want to invoke super.shutdown().)

That is, you're using a finalizer to perform the single and precise operation of closing the database handle.

Last-ditch cleanup of resources is exactly what a finalizer is for, and in this case we'll never have more than one CP instance on the heap, so we don't have to worry about the uncertain lifetime of the object chewing up file handles, say.

This approach isn't incompatible with more eagerly closing the handle if you want to do that somewhere, too.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 611637 [details] [diff] [review]
Patch 1/4

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

::: mobile/android/base/sqlite/SQLiteBridge.java
@@ +233,5 @@
> +        try {
> +            bridge = new SQLiteBridge(path);
> +            bridge.mDbPointer = bridge.openDatabase(path);
> +        } catch(SQLiteBridgeException ex) {
> +        }

The original SQLiteDatabase throws on an error:
SQLiteException 	if the database cannot be opened

Add a followup bug to implement the flags.

@@ +237,5 @@
> +        }
> +        return bridge;
> +    }
> +
> +    public static SQLiteBridge openDatabase(String path, SQLiteDatabase.CursorFactory factory, int flags, DatabaseErrorHandler errorHandler)

This is API Level 11 material. I don't think we need it especially as we don't support the extras anyway.

@@ +242,5 @@
> +        throws SQLiteBridgeException {
> +        return openDatabase(path, factory, flags);
> +    }
> +
> +    public void close() {

See rnewmans comments about finalizers. Right now if this object gets GC-cleaned, we'll leak a DB handle.

::: mozglue/android/SQLiteBridge.cpp
@@ +236,5 @@
> +    sqlite3 *db = (sqlite3*)jDb;
> +    f_sqlite3_close(db);
> +}
> +
> +jobject

static?

Comment 17

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 611638 [details] [diff] [review]
Patch 2/4 - Transactions

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

::: mobile/android/base/sqlite/SQLiteBridge.java
@@ +225,5 @@
>          }
>          return sqliteCall(mDb, aQuery, aParams, mQueryResults);
>      }
>  
> +    public void beginTransaction() throws SQLiteBridgeException {

The original throws IllegalStateException in these circumstances. Consider doing the same for interface compatibility. (On the other hand, maybe it serves as a warning for callers that you don't actually support nested transactions?)

@@ +254,5 @@
> +          } else {
> +              execSQL("ROLLBACK TRANSACTION");
> +          }
> +        } catch(SQLiteBridgeException ex) {
> +            // log this

Yeah, good idea...

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 611642 [details] [diff] [review]
Patch 3/4 - Bulkinsert

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

Needs to split this patch in 2. I'd like gcp to have a look at this patch.

::: mobile/android/base/db/GeckoProvider.java.in
@@ +61,5 @@
>      private HashMap<String, SQLiteBridge> mDatabasePerProfile;
>      protected Context mContext = null;
>  
> +    @Override
> +    public void shutdown() {

How is this change related to the bulkinsert?

@@ +67,5 @@
> +
> +        Collection<SQLiteBridge> bridges = mDatabasePerProfile.values();
> +        Iterator<SQLiteBridge> it = bridges.iterator();
> +
> +        while(it.hasNext()) {

while (...

@@ +111,4 @@
>              dbNeedsSetup = version != mDBVersion;
>          } catch(SQLiteBridgeException ex) {
> +            // close the database
> +            bridge.close();

Same here. How is this related to bulkInsert? Why is this change needed? Add a comment explaning.

@@ +265,5 @@
> +        try {
> +            db.beginTransaction();
> +            String table = getTable(uri);
> +            for (ContentValues initialValues : allValues) {
> +              ContentValues values = initialValues == null ? new ContentValues() : new ContentValues(initialValues);

nit: add empty line here.

@@ +267,5 @@
> +            String table = getTable(uri);
> +            for (ContentValues initialValues : allValues) {
> +              ContentValues values = initialValues == null ? new ContentValues() : new ContentValues(initialValues);
> +              setupDefaults(uri, values);
> +              onPreInsert(values, uri, db);

nit: add empty line here.

@@ +276,5 @@
> +            db.setTransactionSuccessful();
> +        } catch(SQLiteBridgeException ex) {
> +            Log.e(mLogTag, "Error inserting in db", ex);
> +        } finally {
> +            db.endTransaction();

There's some hack you need to do here to ensure there's always an open transaction no matter what happens here. gcp knows the details.

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 611657 [details] [diff] [review]
Patch 5/4 - Use transactions on insert

># HG changeset patch
># Parent 93a835746caff51f24563a5f28fb940eb7db918e
>
>diff --git a/mobile/android/base/db/FormHistoryProvider.java.in b/mobile/android/base/db/FormHistoryProvider.java.in
>--- a/mobile/android/base/db/FormHistoryProvider.java.in
>+++ b/mobile/android/base/db/FormHistoryProvider.java.in
>@@ -143,31 +143,29 @@ public class FormHistoryProvider extends
>         }
>     }
> 
>     public void initGecko() {
>         GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("FormHistory:Init", null));
>     }
> 
>     @Override
>-    public void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db) {
>+    public void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db)
>+        throws SQLiteBridgeException {
>         if (!values.containsKey(FormHistory.GUID)) {
>             return;
>         }
>+
>         String guid = values.getAsString(FormHistory.GUID);
>-        try {
>-            if (guid == null) {
>-                db.delete(TABLE_DELETED_FORM_HISTORY, WHERE_GUID_IS_NULL, null);
>-                return;
>-            }
>-            String[] args = new String[] { guid };
>-            db.delete(TABLE_DELETED_FORM_HISTORY, WHERE_GUID_IS_VALUE, args);
>-        } catch(SQLiteBridgeException ex) {
>-            Log.w(getLogTag(), "Error removing entry with GUID " + guid, ex);
>+        if (guid == null) {
>+            db.delete(TABLE_DELETED_FORM_HISTORY, WHERE_GUID_IS_NULL, null);
>+            return;
>         }
>+        String[] args = new String[] { guid };
>+        db.delete(TABLE_DELETED_FORM_HISTORY, WHERE_GUID_IS_VALUE, args);
>      }
> 
>     @Override
>     public void onPreUpdate(ContentValues values, Uri uri, SQLiteBridge db) { }
> 
>     @Override
>     public void onPostQuery(Cursor cursor, Uri uri, SQLiteBridge db) { }
> }
>diff --git a/mobile/android/base/db/GeckoProvider.java.in b/mobile/android/base/db/GeckoProvider.java.in
>--- a/mobile/android/base/db/GeckoProvider.java.in
>+++ b/mobile/android/base/db/GeckoProvider.java.in
>@@ -234,22 +234,36 @@ public abstract class GeckoProvider exte
>         // If we can not get a SQLiteBridge instance, its likely that the database
>         // has not been set up and Gecko is not running. We return null and expect
>         // callers to try again later
>         if (db == null)
>             return null;
> 
>         setupDefaults(uri, values);
> 
>-        onPreInsert(values, uri, db);
>+        boolean useTransaction = !db.inTransaction();
>+        try {
>+            if (useTransaction) {
>+                db.beginTransaction();
>+            }
> 
>-        try {
>+            // onPreInsert does a check for the item in the deleted table in some cases
>+            // so we put it inside this transaction
>+            onPreInsert(values, uri, db);
>             id = db.insert(getTable(uri), null, values);
>+
>+            if (useTransaction) {
>+                db.setTransactionSuccessful();
>+            }
>         } catch(SQLiteBridgeException ex) {
>             Log.e(mLogTag, "Error inserting in db", ex);
>+        } finally {
>+            if (useTransaction) {
>+                db.endTransaction();
>+            }
>         }
> 
>         return ContentUris.withAppendedId(uri, id);
>     }
> 
>     @Override
>     public int bulkInsert(Uri uri, ContentValues[] allValues) {
>         long id = -1;
>@@ -332,14 +346,15 @@ public abstract class GeckoProvider exte
>     public abstract String getTable(Uri uri);
> 
>     public abstract String getSortOrder(Uri uri, String aRequested);
> 
>     public abstract void setupDefaults(Uri uri, ContentValues values);
> 
>     public abstract void initGecko();
> 
>-    public abstract void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db);
>+    public abstract void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db)
>+      throws SQLiteBridgeException;
> 
>     public abstract void onPreUpdate(ContentValues values, Uri uri, SQLiteBridge db);
> 
>     public abstract void onPostQuery(Cursor cursor, Uri uri, SQLiteBridge db);
> }
>diff --git a/mobile/android/base/db/PasswordsProvider.java.in b/mobile/android/base/db/PasswordsProvider.java.in
>--- a/mobile/android/base/db/PasswordsProvider.java.in
>+++ b/mobile/android/base/db/PasswordsProvider.java.in
>@@ -209,29 +209,26 @@ public class PasswordsProvider extends G
>         } else {
>           if (profilePath != null) result = NSSBridge.decrypt(mContext, profilePath, initialValue);
>           else result = NSSBridge.decrypt(mContext, initialValue);            
>         }
>         return result;
>     }
> 
>     @Override
>-    public void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db) {
>+    public void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db)
>+        throws SQLiteBridgeException {
>         if (values.containsKey(Passwords.GUID)) {
>             String guid = values.getAsString(Passwords.GUID);
>-            try {
>-                if (guid == null) {
>-                    db.delete(TABLE_DELETED_PASSWORDS, WHERE_GUID_IS_NULL, null);
>-                    return;
>-                }
>-                String[] args = new String[] { guid };
>-                db.delete(TABLE_DELETED_PASSWORDS, WHERE_GUID_IS_VALUE, args);
>-            } catch(SQLiteBridgeException ex) {
>-                Log.w(getLogTag(), "Error removing entry with GUID " + guid, ex);
>+            if (guid == null) {
>+                db.delete(TABLE_DELETED_PASSWORDS, WHERE_GUID_IS_NULL, null);
>+                return;
>             }
>+            String[] args = new String[] { guid };
>+            db.delete(TABLE_DELETED_PASSWORDS, WHERE_GUID_IS_VALUE, args);
>         }
> 
>         if (values.containsKey(Passwords.ENCRYPTED_PASSWORD)) {
>             String res = doCrypto(values.getAsString(Passwords.ENCRYPTED_PASSWORD), uri, true);
>             values.put(Passwords.ENCRYPTED_PASSWORD, res);
>         }
> 
>         if (values.containsKey(Passwords.ENCRYPTED_USERNAME)) {

Comment 20

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 611642 [details] [diff] [review]
Patch 3/4 - Bulkinsert

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

::: mobile/android/base/db/GeckoProvider.java.in
@@ +276,5 @@
> +            db.setTransactionSuccessful();
> +        } catch(SQLiteBridgeException ex) {
> +            Log.e(mLogTag, "Error inserting in db", ex);
> +        } finally {
> +            db.endTransaction();

This is OK. The issue Lucas refers to is specific to applyBatch, not bulkInsert. Compare to the code here:

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

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch 1/4 v2

I think this addresses the comments. There's an isOpen() call in the finalizer, because I only want to show that logging if the db is open (i.e. the caller has screwed up some how).

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch 1/4 v2.1

Grr. This was in my next patch by accident. Just moving the database declaration outside the try catch in openDatabase.

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch 2/4

Just updating. I'm not sure if we want to match the SQLiteDatabase exception structure or not. I didn't here yet.

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch 3/4

This uses the new method of opening database in GeckoProvider, and closes it in a finalizer. I think, because we have one handle per SQLiteBridge, we don't need to worry about synchronizing this or anything. The only person calling into it is this particular GeckoProvider, and they are going away when the finalizer is called. I'll file a follow up to implement a fancy timeout for closing it as well.

Comment 25

[Wesley Johnston (:wesj)]

Attachment: Patch 4/4 - Bulk insert

The things I do for you lucas. Some day I would also like a lesson from you on where empty lines should and should not go.

This implements bulkInsert for us. gcp tells me I don't need his hack here.

Comment 26

[Wesley Johnston (:wesj)]

Comment on attachment 611657 [details] [diff] [review]
Patch 5/4 - Use transactions on insert

Whoops. This isn't obsolete. Its unreviewed.

Comment 27

[Wesley Johnston (:wesj)]

Comment on attachment 611657 [details] [diff] [review]
Patch 5/4 - Use transactions on insert

lucas is PTO for a bit. moving these to rnewman

Comment 28

[Lucas Rocha (:lucasr)]

Comment on attachment 612079 [details] [diff] [review]
Patch 4/4 - Bulk insert

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

The bulkInsert() part looks good to me. I don't understand how the DB closing change related to it. Based on the discussion on the bug, this seems to be the agree route to take. So giving my r+ and requesting feedback from rnewman.

::: mobile/android/base/db/GeckoProvider.java.in
@@ +115,5 @@
>              dbNeedsSetup = version != mDBVersion;
>          } catch(SQLiteBridgeException ex) {
> +            // if we managed to open the database, close it
> +            if (bridge != null)
> +                bridge.close();

How is this related to the bulkInsert change?

@@ +139,5 @@
>              bridge = null;
>              initGecko();
>          }
> +        if (bridge != null)
> +            mDatabasePerProfile.put(databasePath, bridge);

How is this related to the bulkInsert() change?

@@ +272,5 @@
> +        try {
> +            db.beginTransaction();
> +            String table = getTable(uri);
> +            for (ContentValues initialValues : allValues) {
> +              ContentValues values = initialValues == null ? new ContentValues() : new ContentValues(initialValues);

Can't pass null to the ContentValues constructor? If you can, you could just use initialValues and let the contructor handle that for you. If you can't, surround the the initialValues == null ? ... with parens for better readability?

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 612078 [details] [diff] [review]
Patch 3/4

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

I'm assuming this is only needed for content providers using the SQLite bridge, right?

::: mobile/android/base/db/GeckoProvider.java.in
@@ +115,4 @@
>              dbNeedsSetup = version != mDBVersion;
>          } catch(SQLiteBridgeException ex) {
> +            // close the database
> +            bridge.close();

Is bridge guaranteed to be non-null here?

Comment 30

[Richard Newman [:rnewman]]

Will get to this this afternoon.

Comment 31

[Richard Newman [:rnewman]]

Comment on attachment 612078 [details] [diff] [review]
Patch 3/4

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

shutdown() needs some love to be safe.

::: mobile/android/base/db/GeckoProvider.java.in
@@ +62,5 @@
>      protected Context mContext = null;
>  
> +    @Override
> +    public void shutdown() {
> +        Collection<SQLiteBridge> bridges = mDatabasePerProfile.values();

This is called from the finalizer of an abstract class that doesn't set itself up, so you cannot count on any of the member variables -- including mDatabasePerProfile -- being initialized. That is, this apparently safe code is guaranteed to throw a NPE:

  public class FooProvider extends GeckoProvider {
    public FooProvider() {}
  }

  FooProvider p = new FooProvider();
  p = null;

This is what I meant by being really, really careful when you write this stuff.

Being as this is called from the finalizer, which will 100% be called if you ever manually call shutdown(), you should null out mDatabasePerProfile at the end of shutdown(), and -- as above -- check for null before attempting to work on it. That will avoid this work being done twice.

@@ +115,2 @@
>              dbNeedsSetup = version != mDBVersion;
>          } catch(SQLiteBridgeException ex) {

Space between 'catch' and parens.

@@ +115,4 @@
>              dbNeedsSetup = version != mDBVersion;
>          } catch(SQLiteBridgeException ex) {
> +            // close the database
> +            bridge.close();

Nope. Definitely needs a null check.

Comment 32

[Richard Newman [:rnewman]]

Comment on attachment 612079 [details] [diff] [review]
Patch 4/4 - Bulk insert

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

Marking as r- so we can fix the error handling.

::: mobile/android/base/db/GeckoProvider.java.in
@@ +115,5 @@
>              dbNeedsSetup = version != mDBVersion;
>          } catch(SQLiteBridgeException ex) {
> +            // if we managed to open the database, close it
> +            if (bridge != null)
> +                bridge.close();

Yeah, this should be lifted into the previous patch.

@@ +263,5 @@
> +        int rowsAdded = 0;
> +        final SQLiteBridge db = getDatabase(uri);
> +
> +        // If we can not get a SQLiteBridge instance, its likely that the database
> +        // has not been set up and Gecko is not running. We return null and expect

Comment is incorrect; we return 0.

@@ +266,5 @@
> +        // If we can not get a SQLiteBridge instance, its likely that the database
> +        // has not been set up and Gecko is not running. We return null and expect
> +        // callers to try again later
> +        if (db == null)
> +            return rowsAdded;

Don't be so C++ey! Move `rowsAdded` and `id` to after this clause, check for the DB at the front of the method, return constant 0.

@@ +270,5 @@
> +            return rowsAdded;
> +
> +        try {
> +            db.beginTransaction();
> +            String table = getTable(uri);

Move this out of the try block, check the result, and make `table` final. If getTable fails, you just opened a transaction for nothing...

@@ +272,5 @@
> +        try {
> +            db.beginTransaction();
> +            String table = getTable(uri);
> +            for (ContentValues initialValues : allValues) {
> +              ContentValues values = initialValues == null ? new ContentValues() : new ContentValues(initialValues);

If one of `allValues` is null, I think someone's made an error somewhere, and you should throw an IllegalArgumentException, no?

Perhaps `new ContentValues(null)` will do that for you :)

@@ +277,5 @@
> +              setupDefaults(uri, values);
> +              onPreInsert(values, uri, db);
> +              id = db.insert(table, null, values);
> +              if (id > 0)
> +                rowsAdded++;

When would this not be the case? Surely a failure to insert is an exception?

@@ +280,5 @@
> +              if (id > 0)
> +                rowsAdded++;
> +            }
> +            db.setTransactionSuccessful();
> +        } catch(SQLiteBridgeException ex) {

Space before parens.

@@ +281,5 @@
> +                rowsAdded++;
> +            }
> +            db.setTransactionSuccessful();
> +        } catch(SQLiteBridgeException ex) {
> +            Log.e(mLogTag, "Error inserting in db", ex);

Why not rethrow or pass through the exception?

If you don't, you must return 0 -- you have *not* marked the transaction as successful (unless the exception was thrown by setTransactionSuccessful!), and so *no* rows have been added. Falling through and returning `rowsAdded` is wrong.

Comment 33

[Richard Newman [:rnewman]]

Comment on attachment 611657 [details] [diff] [review]
Patch 5/4 - Use transactions on insert

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

Good apart from the weird return. Another round, please, publican!

::: mobile/android/base/db/GeckoProvider.java.in
@@ +256,2 @@
>          } catch(SQLiteBridgeException ex) {
>              Log.e(mLogTag, "Error inserting in db", ex);

This seems wrong to me.

I think you're relying on this method returning

  content://foo/bar/-1

to signal to calling code that already has a transaction that this inner operation failed.

Unfortunately, that is cruel to calling code that you don't control: they'll get that weird URI if onPreInsert throws a SQLiteBridgeException.

If we're *already* in a transaction (e.g., bulk), and a SQLiteBridgeException is thrown, control will return to the caller through an ordinary return -- you capture the exception.

I think you should rethrow the exception here. If you don't (for example, because you don't want to turn it into a RuntimeException) then you should consider returning null instead of a non-null value.

Comment 34

[Wesley Johnston (:wesj)]

Attachment: Patch 3/5

Thanks for the help! This does the null check. Moves in the bridge checks that snuck into other patches (I am not a fan of micro-patches for this very reason). Also did a few other catch( spacing fixes.

Comment 35

[Wesley Johnston (:wesj)]

Attachment: Patch 4/5 - Bulk insert

We'll throw on new ContentProvider(null) now, which is fine with me if the sync folks are ok with it. Also throws a RuntimeException if something fails.

I'll write some tests for this function....

Comment 36

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5 - Use transactions on insert

This throws now on errors. I went ahead and put in rethrow for all of our db operations if there's an actual error interacting with the database (invalid columns, invalid bind parameters, etc, etc).

If we can't get the database (invalid uri), we still return null (or 0). We could throw InvalidArgumentException there as well....

Comment 37

[Richard Newman [:rnewman]]

Comment on attachment 612920 [details] [diff] [review]
Patch 3/5

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

::: mobile/android/base/db/GeckoProvider.java.in
@@ +71,5 @@
> +
> +        while (it.hasNext()) {
> +            SQLiteBridge bridge = it.next();
> +            if (bridge != null) {
> +                bridge.close();

I imagine it's possible for bridge.close() to throw an exception (though SQLiteBridge.close() is a no-op in mozilla-inbound?!).

Perhaps wrap this in a try..catch with an empty catch body? Just thinking of the case where one bridge can't be closed, and the others are thus left open.

Comment 38

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #37)
> though SQLiteBridge.close() is a no-op in mozilla-inbound?!).

Ah, it's added in part 2.

Comment 39

[Wesley Johnston (:wesj)]

Yeah. close doesn't throw right now. We don't even check the return value. It apparently can return BUSY it we have open statements or blobs.

Comment 40

[Richard Newman [:rnewman]]

Comment on attachment 612923 [details] [diff] [review]
Patch 4/5 - Bulk insert

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

r+ with that change.

::: mobile/android/base/db/GeckoProvider.java.in
@@ +286,5 @@
> +            }
> +            db.setTransactionSuccessful();
> +        } catch (SQLiteBridgeException ex) {
> +            Log.e(mLogTag, "Error inserting in db", ex);
> +            throw new RuntimeException(ex);

You should consider having SQLiteBridgeException extend RuntimeException, not Exception.

http://stackoverflow.com/questions/27578/when-to-choose-checked-and-unchecked-exceptions

That will allow you to simply rethrow here and have callers catch the direct exception, rather than introspecting a RuntimeException.

Comment 41

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #39)
> Yeah. close doesn't throw right now. We don't even check the return value.
> It apparently can return BUSY it we have open statements or blobs.

Throwing inside a finalizer doesn't do anything -- it can't. But you also can't fail to clean up.

So you have to think about a strategy for avoiding leaks if you can help it. Not super urgent, but this will bite you at scale.

Comment 42

[Richard Newman [:rnewman]]

Comment on attachment 612926 [details] [diff] [review]
Patch 5/5 - Use transactions on insert

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

::: mobile/android/base/db/GeckoProvider.java.in
@@ +231,5 @@
>          try {
>              deleted = db.delete(getTable(uri), selection, selectionArgs);
>          } catch (SQLiteBridgeException ex) {
>              Log.e(mLogTag, "Error deleting record", ex);
> +            throw new RuntimeException(ex);

Same point here about extending RuntimeException and just rethrowing.

@@ +254,5 @@
> +        boolean useTransaction = !db.inTransaction();
> +        try {
> +            if (useTransaction) {
> +                db.beginTransaction();
> +           }

Indentation.

@@ +268,3 @@
>          } catch (SQLiteBridgeException ex) {
>              Log.e(mLogTag, "Error inserting in db", ex);
> +            throw new RuntimeException(ex);

And same.

@@ +328,5 @@
>          try {
>              updated = db.update(getTable(uri), values, selection, selectionArgs);
>          } catch (SQLiteBridgeException ex) {
>              Log.e(mLogTag, "Error updating table", ex);
> +            throw new RuntimeException(ex);

Same.

@@ +353,5 @@
>              cursor = db.query(getTable(uri), projection, selection, selectionArgs, null, null, sortOrder, null);
>              onPostQuery(cursor, uri, db);
>          } catch (SQLiteBridgeException ex) {
>              Log.e(mLogTag, "Error querying database", ex);
> +            throw new RuntimeException(ex);

Same.

@@ +368,5 @@
>  
>      public abstract void initGecko();
>  
> +    public abstract void onPreInsert(ContentValues values, Uri uri, SQLiteBridge db)
> +        throws SQLiteBridgeException;

Can avoid this if we make it a RuntimeException, or leave it as documentation...

Comment 43

[Wesley Johnston (:wesj)]

Switch to use RuntimeException:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d6cc2e4726

Comment 44

[Wesley Johnston (:wesj)]

Grr.. other junk snuck into my push. Had to back out. Will land again momentarily:
backout - https://hg.mozilla.org/integration/mozilla-inbound/rev/c347f2c72963

Comment 45

[Wesley Johnston (:wesj)]

and backin:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f385e9f904

Comment 46

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #44)
> Grr.. other junk snuck into my push. Had to back out.

`hg out -p` is your friend :D

Comment 47

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/f5d6cc2e4726
https://hg.mozilla.org/mozilla-central/rev/c347f2c72963
https://hg.mozilla.org/mozilla-central/rev/f0f385e9f904

Comment 48

[Tony Chung [:tchung]]

Nick or Richard,
damn, i lost the irc thread that you were telling me how to efficiently verify this fix.  Can you comment in here with qa+ directions so my team can verify correctly?  Thanks.