739514 - Initial Fennec history visits being created without a GUID

Status: RESOLVED FIXED

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

Description

[Richard Newman [:rnewman]]

In particular, about:firefox, about:home, Customize, and Support.

Having these without a GUID makes Sync a sad panda. And by "sad panda" I mean we throw an IllegalArgumentException and abort the sync.


20:37:29 <@mfinkle> rnewman, honestly we need to revert the history adds now that bookmarks show up in the awesomebar


Recommended course of action:

* Delete these (0 visits) or assign a GUID if there is none. This will require a database migration.
* Don't create them on startup, per mfinkle.
* During said migration, require history entries to have non-null GUIDs.


I will probably wire in a quick hack to kill all of these at the start of a sync just to unblock us.

Comment 1

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

Attachment: patch

This patch removes the code that adds a fake history visit from the default bookmarks. I verifed that the default bookmarks still show up on the home page and the awesomebar (all pages), but do not show up in history.

Comment 2

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

Crap, I need to add the migration support.

Comment 3

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

Attachment: patch 2

Adds a simple migration, removing the NULL GUIDs

Comment 4

[Wesley Johnston (:wesj)]

If we don't want to allow null, we should modify the db table: i.e.

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

should read History.GUID + " TEXT NOT NULL," I think?

Seems like BrowserProvider should also automagically generate GUID's for you if you don't supply them. I don't see why callers should need to provide this at least? I guess that can be a separate bug though.

Comment 5

[Richard Newman [:rnewman]]

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

Yes and yes. (Though as I mentioned, you'll need a migration to do the former.)

See BrowserProvider.java.in, insertInTransaction:

                if (!values.containsKey(Bookmarks.GUID)) {
                  values.put(Bookmarks.GUID, Utils.generateGuid());
                }

Comment 6

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

I passed on making the table changes because:
* Bookmarks should probably change too
* I'm lazy and wanted to go to sleep

We can get a new patch up to put the "NOT NULL" constraints and do the "RENAME TABLE"/"INSERT INTO" dance.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch 1/3 - Delete these form history items

I'm not sure the right way to do this in pieces, since the first two are both migration bits, but we'll just understand they will be qfolded when I land them.

This is just mfinkle's patch with a small change to use "guid IS NULL" instead of "guid=NULL" which was failing.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch 2/3 - Update schema

This updates our schema to include guid TEXT NOT NULL when the bookmarks and history tables are created. I added a bit to also drop and recreate the combined view. Bookmarks migration doesn't do that. I'm not really familiar with how those are optimized underneath, so feel free to tell me that's not necessary?

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 3/3 - Use insertInTransaction

insertInTransaction creates the guids for us.

This would have avoided me this pain initially. I tried using our built in insert methods originally, but when creating bookmarks, they try to get a db, which ends up calling the database creation code again for some reason, and we crash in a firy loop. I modified it to take in a database here which removes some a redundant getWritableDatabase call we had anyway.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 609835 [details] [diff] [review]
Patch 2/3 - Update schema

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

Looks good but needs to change images table as well.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +76,5 @@
>      static final String TABLE_HISTORY = "history";
>      static final String TABLE_IMAGES = "images";
>  
>      static final String TABLE_BOOKMARKS_TMP = TABLE_BOOKMARKS + "_tmp";
> +    static final String TABLE_HISTORY_TMP = TABLE_HISTORY + "_tmp";

You have to do the same for the images table as well.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 609833 [details] [diff] [review]
Patch 1/3 - Delete these form history items

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

Looks good.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 609837 [details] [diff] [review]
Patch 3/3 - Use insertInTransaction

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

Feels wrong to change insertInTransaction to be used in the SQLiteOpenHelper as this is a method used only in the context of the ContentProvider calls. It adds a misleading exception to how the content provider is structured. If it's just about generating the GUIDs, call Utils.generateGuid() in createDefaultBookmarks() directly. It's just one extra call anyway.

Comment 13

[Lucas Rocha (:lucasr)]

Although I appreciate that you split your patches (\o/) for the patch review, they should be folded into one single changeset before pushing because you want the DB migration to be atomically committed to the repo.

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Upgrade the db

Updates the image table to. No need for patch 3 if we're just going to use Utils.generateGuid(), since we are already using Utils.generateGuid() (except for images, where I added it).

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 610937 [details] [diff] [review]
Patch 2/2 - Upgrade the db

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

Looks good but I'd like to have an answer to my question about images with null GUID before giving r+. Feels like an unnecessary update.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +773,5 @@
> +            Cursor cursor = db.query(TABLE_IMAGES, columns, Images.GUID + " IS NULL", null, null ,null, null, null);
> +            ContentValues values = new ContentValues();
> +            if (cursor.moveToFirst()) {
> +                do {
> +                    values.put(Images.GUID, Utils.generateGuid());

Is there actually any image with null GUIDs? How can they happen? Why is this necessary?

Comment 16

[Wesley Johnston (:wesj)]

> Is there actually any image with null GUIDs? How can they happen? Why is
> this necessary?

The initial (incorrect) bookmarks creation code was creating images with null guids. This patch modifies it to add:

values.put(Images.GUID, Utils.generateGuid());

Comment 17

[Wesley Johnston (:wesj)]

s/"bookmarks creation code"/"default bookmarks creation code"/ I mean

Comment 18

[Wesley Johnston (:wesj)]

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

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 20

[Wesley Johnston (:wesj)]

Attachment: Correct version

The db version jumped while I was landing this, so I adjusted before landing. Apparently I didn't though.

Comment 21

[:Margaret Leibovic]

Comment on attachment 612750 [details] [diff] [review]
Correct version

Don't you want to turn DELETED_RECORDS_PURGE_LIMIT back to 5, not 6?

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Correct version

Uhm.... yep.

Comment 23

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/657e55debef7

Comment 24

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/657e55debef7