Closed Bug 739514 Opened 8 years ago Closed 8 years ago

Initial Fennec history visits being created without a GUID

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- beta+

People

(Reporter: rnewman, Assigned: wesj)

References

Details

Attachments

(3 files, 5 obsolete files)

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.
Blocks: 739515
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: wjohnston → mark.finkle
Attachment #609610 - Flags: review?(wjohnston)
Crap, I need to add the migration support.
Attached patch patch 2 (obsolete) — Splinter Review
Adds a simple migration, removing the NULL GUIDs
Attachment #609610 - Attachment is obsolete: true
Attachment #609610 - Flags: review?(wjohnston)
Attachment #609613 - Flags: review?(wjohnston)
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.
(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());
                }
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.
blocking-fennec1.0: ? → beta+
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.
Assignee: mark.finkle → wjohnston
Attachment #609613 - Attachment is obsolete: true
Attachment #609613 - Flags: review?(wjohnston)
Attachment #609833 - Flags: review?(lucasr.at.mozilla)
Attached patch Patch 2/3 - Update schema (obsolete) — Splinter Review
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?
Attachment #609835 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #609837 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #609835 - Flags: review?(lucasr.at.mozilla) → review-
Comment on attachment 609833 [details] [diff] [review]
Patch 1/3 - Delete these form history items

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

Looks good.
Attachment #609833 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #609837 - Flags: review?(lucasr.at.mozilla) → review-
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.
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).
Attachment #609835 - Attachment is obsolete: true
Attachment #609837 - Attachment is obsolete: true
Attachment #610937 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #610937 - Flags: review?(lucasr.at.mozilla) → review-
> 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());
s/"bookmarks creation code"/"default bookmarks creation code"/ I mean
Attachment #610937 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/aca7aac62fa6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Attached patch Correct version (obsolete) — Splinter Review
The db version jumped while I was landing this, so I adjusted before landing. Apparently I didn't though.
Attachment #612750 - Flags: review?(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?
Attached patch Correct versionSplinter Review
Uhm.... yep.
Attachment #612750 - Attachment is obsolete: true
Attachment #612750 - Flags: review?(margaret.leibovic)
Attachment #612761 - Flags: review?(margaret.leibovic)
Attachment #612761 - Flags: review?(margaret.leibovic) → review+
You need to log in before you can comment on or make changes to this bug.