1172077 - Merge tabs.db into browser.db

Status: RESOLVED FIXED

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

Description

[Richard Newman [:rnewman]]

As nalexander summarized:

08:54:40 < R4md4c> nalexander: Why did Fennec go with two separate DBs in the first place ? Why not just one DB and several tables ?
08:54:48 < nalexander> R4md4c: hysterical raisins.
08:55:00 < nalexander> R4md4c: at the time, we didn't understand CP's and the trade offs that well.
08:55:29 < nalexander> R4md4c: also, browser.db has a long history.  It originally was the *system* browser database, which doesn't hold tabs.
08:55:34 < nalexander> R4md4c: so a new DB was needed.  IIRC.
08:55:53 < nalexander> R4md4c: rnewman and I discussed this -- we know of no particular reason that a separate DB was chosen.

Comment 1

[Ahmed Khalil]

Attachment: tabs_provider_merging.patch

I've wrote a patch that could make the tabs and clients tables to be written to the browser.db, instead of being written into a separate DB file.

Comment 2

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

Comment on attachment 8616135 [details] [diff] [review]
tabs_provider_merging.patch

In general this is looking OK. Is this patch enough to make everything work?

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8616135 [details] [diff] [review]
tabs_provider_merging.patch

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

Ahmed: First, sorry for the slow (!) review.  Slow, even by my standards.

Second, this looks great.

Third, I kinda can't believe it Just Works, with essentially no change to TabsProvider.  The magic of the ContentResolver URI indirection and matching, I guess?  But in any case I'd like to see a JUnit 3 tests.  I realize now I never landed some of your older code!  (Bug 1159020)  Can you make sure that applies over top of this, and that the tests in that patch work with your changes?  That'd certainly convince me this is working.

Again, sorry for the slow review.

Comment 4

[Ahmed Khalil]

(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 8616135 [details] [diff] [review]
> tabs_provider_merging.patch
> 
> Review of attachment 8616135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ahmed: First, sorry for the slow (!) review.  Slow, even by my standards.
> 
> Second, this looks great.
> 
> Third, I kinda can't believe it Just Works, with essentially no change to
> TabsProvider.  The magic of the ContentResolver URI indirection and
> matching, I guess?  But in any case I'd like to see a JUnit 3 tests.  I
> realize now I never landed some of your older code!  (Bug 1159020)  Can you
> make sure that applies over top of this, and that the tests in that patch
> work with your changes?  That'd certainly convince me this is working.
> 
> Again, sorry for the slow review.

Hi Nick, 

In the patch I've posted it is rebased on top your last review[1] that had the TestRemoteTabs.java and I've run your JUnit 3 test and the result was a success. So I guess the patch is working, but perhaps a try build would make us more certain ?


[1] https://reviewboard.mozilla.org/r/9595/

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Ahmed Khalil from comment #4)
> (In reply to Nick Alexander :nalexander from comment #3)
> > Comment on attachment 8616135 [details] [diff] [review]
> > tabs_provider_merging.patch
> > 
> > Review of attachment 8616135 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Ahmed: First, sorry for the slow (!) review.  Slow, even by my standards.
> > 
> > Second, this looks great.
> > 
> > Third, I kinda can't believe it Just Works, with essentially no change to
> > TabsProvider.  The magic of the ContentResolver URI indirection and
> > matching, I guess?  But in any case I'd like to see a JUnit 3 tests.  I
> > realize now I never landed some of your older code!  (Bug 1159020)  Can you
> > make sure that applies over top of this, and that the tests in that patch
> > work with your changes?  That'd certainly convince me this is working.
> > 
> > Again, sorry for the slow review.
> 
> Hi Nick, 
> 
> In the patch I've posted it is rebased on top your last review[1] that had
> the TestRemoteTabs.java and I've run your JUnit 3 test and the result was a
> success. So I guess the patch is working, but perhaps a try build would make
> us more certain ?

Yep, awesome.  I just landed the underlying work -- good job, BTW -- so I'd love to see a try build for this.  As for migration, we need to add an interface to reset some Sync timestamps, and then force a Sync during the DB migration.  This could be tricky.

Reading links doesn't show a great point for this.  I think we might want to implement Bug 815773 to do this.

Comment 6

[Ahmed Khalil]

Attachment: tabs_provider_merging.patch

Hi Nick, I've added the migration that we've agreed on, where it reads all the records from "tabs.db" and insert them into "browser.db"

Let me know what you think about it.

Comment 7

[Ahmed Khalil]

Attachment: tabs_provider_merging.patch

Here is a more pruned version from the same patch as it contained modified lines that I've never changed and no idea why did Android Studio formatted these lines for me.

Comment 8

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

Are we sure we need to do migration? I think the application deletes all rows and inserts new records when writing the open tabs out to the DB.

Are we worried about a Sync happening before the application has a chance to push data into the new DB table?

Comment 9

[Richard Newman [:rnewman]]

I think it's easier to do a migration than it is to tell Sync to start over. See Comment 5.

Comment 10

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

(In reply to Richard Newman [:rnewman] from comment #9)
> I think it's easier to do a migration than it is to tell Sync to start over.
> See Comment 5.

I have seen Fennec crash heroically during migrations. Removing the complexity might be a win, and stamping out complexity is my mission.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8622805 [details] [diff] [review]
tabs_provider_merging.patch

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

This is a good approach.  Let's do two things:

1) Extract a DatabaseUtils.copyTable or something, to make the internals available to others.

2) Make the migration function just call a general function that takes the DB file as a parameter.  I want to be able to test easier.

So migrate -> copy-and-delete-function -> {call two copy helper functions}.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +939,5 @@
> +    private void upgradeDatabaseFrom23to24(SQLiteDatabase db) {
> +        createTabsTable(db);
> +        createClientsTable(db);
> +
> +        Cursor tabsCursor = null, clientsCursor = null;

nit: separate lines, please.

@@ +959,5 @@
> +
> +            ContentValues contentValues = new ContentValues();
> +            while (tabsCursor.moveToNext()) {
> +                contentValues.clear();
> +                DatabaseUtils.cursorRowToContentValues(tabsCursor, contentValues);

nifty.  Are you confident that there's no "bleed", i.e. that old values are removed (even when there's a NULL in the table?)

@@ +966,5 @@
> +
> +            while (clientsCursor.moveToNext()) {
> +                contentValues.clear();
> +                DatabaseUtils.cursorRowToContentValues(clientsCursor, contentValues);
> +                db.insertOrThrow(TabsProvider.TABLE_CLIENTS, null, contentValues);

Do we care about throwing?  It might be best to ignore a bad row.

@@ +970,5 @@
> +                db.insertOrThrow(TabsProvider.TABLE_CLIENTS, null, contentValues);
> +            }
> +
> +            // If no exception has occurred till now, we delete the old tabs.db file.
> +            FileUtils.delete(oldTabsDBFile);

We never want this to live after we /try/ to migrate.

@@ +975,5 @@
> +
> +            // We are done, commit!
> +            db.setTransactionSuccessful();
> +        } catch (SQLException e) {
> +            debug("Exception occurred while trying to copy from tabs.db to browser.db: " + e);

Log.e, for sure.  Might as well know.

@@ +977,5 @@
> +            db.setTransactionSuccessful();
> +        } catch (SQLException e) {
> +            debug("Exception occurred while trying to copy from tabs.db to browser.db: " + e);
> +        } catch (IOException e) {
> +            debug("Exception happened while trying to delete tabs.db file: " + e);

ditto, Log.e.

@@ +986,5 @@
> +            if (clientsCursor != null) {
> +                clientsCursor.close();
> +            }
> +            if (oldTabsDB != null) {
> +                oldTabsDB.close();

Presumably you need to delete the file *after* you close.

Comment 12

[Ahmed Khalil]

Attachment: tabs_provider_merging.patch

Separated the migrate method into a separate function so that testing could be easy, also added a utility method to copy tables from a SQLiteDatabase to another.

Comment 13

[Ahmed Khalil]

Attachment: tabs_provider_merging.patch

Sorry for the amount of white spaces changes that happens without me knowing, it won't happen again as I know now how to resolve them.

Comment 14

[Nick Alexander :nalexander [he/him]]

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

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8623392 [details] [diff] [review]
tabs_provider_merging.patch

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

I made some small changes to this, tested locally quite a bit, and pushed a try build.  Let's see how that goes, and then we're ready to land!

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +197,5 @@
> +                " ON " + TABLE_CLIENTS + "(" + BrowserContract.Clients.GUID + ")");
> +    }
> +
> +    private void createTabsTable(SQLiteDatabase db) {
> +        debug("Creating tabs.db: " + db.getPath());

This is no longer accurate.

Comment 16

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/6b80a1818bf1

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b80a1818bf1