716729 - Identify root cause of favicon crash on some Honeycomb/ICS devices' Bookmarks database

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

Crash bugs 712791 and 711977 were caused by Android's Bookmarks database throwing mysterious SQLiteConstraintExceptions ("error code 19: constraint failed") on some Honeycomb and ICS devices. A short-term fix for the crash was to catch and ignore SQLiteConstraintExceptions, but we should really identify the root cause of the exception.

I suspect the problem might be related to bookmarks migration. Google changed the Bookmarks database schema in Honeycomb, so Android has migration code for pre-Honeycomb databases. Native Fennec has its own bookmarks migration code (unrelated to Google's schema changes) for XUL Fennec bookmarks.

Comment 1

[Lucas Rocha (:lucasr)]

Chris, what do you mean by "bookmarks migration"? Moving bookmarks from fennec to Fennec's native local DB?

Comment 2

[Chris Peterson [:cpeterson]]

mobile/android/base/ProfileMigrator.java

Comment 3

[Gian-Carlo Pascutto [:gcp]]

>I suspect the problem might be related to bookmarks migration. Google changed the 
>Bookmarks database schema in Honeycomb, so Android has migration code for 
>pre-Honeycomb databases. Native Fennec has its own bookmarks migration code 
>(unrelated to Google's schema changes) for XUL Fennec bookmarks.

This paragraph makes no sense whatsoever to me. The XUL->Native migration code simply doesn't care or know in the slightest what is storing the bookmarks/favicons, let alone how. So why do you think there is a relation?

My best guess at the SQLiteConstraintExceptions is that parts of our code are trying to add Favicons that already exist, or that don't conform to whatever format/size Android allows us to store.

Comment 4

[Chris Peterson [:cpeterson]]

> why do you think there is a relation?

I was just listing code paths that may touch favicon items stored in Android's Bookmarks database.

> My best guess at the SQLiteConstraintExceptions is that parts of our code are trying to 
> add Favicons that already exist

This is my guess, too. AndroidBrowserDB.updateFaviconForUrl() tries to update() existing favicons, but perhaps the query is looking for the wrong key.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

This is becoming a bit of a pain for adding transaction support :-/

Comment 6

[Chris Peterson [:cpeterson]]

gcp, this favicon crash is causing problems for transaction support? Should we increase the priority of this bug?

Comment 7

[Gian-Carlo Pascutto [:gcp]]

If I do a batched update (which uses transactions), then this error can be thrown in the middle of what is really a "valid" transaction. So the code doing the batched update needs to handle it explicitly, recover and restart in at least 2 places.

I know about this so it was easily handled, but the problem is starting to "leak out" as many callers must know about it.

Also, it's probably indicative of a bug in our db handling? I think it would be good to avoid getting many corrupted/malformed DB's out there before its too late.

Comment 8

[Chris Peterson [:cpeterson]]

> Also, it's probably indicative of a bug in our db handling? I think it would be good to 
> avoid getting many corrupted/malformed DB's out there before its too late.

That is a very good point. I don't have the device that reproduces this problem, so maybe we should reassign this bug to someone who does.

I believe the problem is related to some software configuration (not specific to particular HW or OS version) because I had a repro'ing device, but the problem went away when I inadvertently flashed a clean OS install when rooting the device.

Comment 9

[Chris Peterson [:cpeterson]]

This is not an IME bug.

Comment 10

[Chris Peterson [:cpeterson]]

This bug depends on or is a dupe of bug 734624.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

http://pastebin.mozilla.org/1528268

I'm hitting this in profile migration. It would be strange for the cause of that to be a race in saveFaviconToDb, because that code isn't called. So there must be a second bug there.

Reading through bug 734624 and the ContentProvider code, the only 2 constraints are that URL must be UNIQUE and IS_DELETED NOT NULL.

Thinking about it, the likely cause is that multiple bookmarks/history entries have the same favicon URL. The code checks for this, but this check queries the database and will not see the queued adds in a transaction. However...

...the actual error in on an UPDATE, which is only triggered when the URL is found.
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ProfileMigrator.java#409

The update is called with the exact same URL that the query to determine whether it exists uses, and IS_DELETED is set. Consequently, none of this makes sense.

Comment 12

[Chris Peterson [:cpeterson]]

Reassigning to blassey because related bug 734624 is assigned to him.

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #11)
> > ...the actual error in on an UPDATE, which is only triggered when the URL is
> found.
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> ProfileMigrator.java#409
> 
> The update is called with the exact same URL that the query to determine
> whether it exists uses, and IS_DELETED is set. Consequently, none of this
> makes sense.

The query for the existing bookmark must include SHOW_DELETED in this particular case. Right now, it will not return any bookmark, even if there's a bookmark for that URL that is marked as deleted in the database. So, the bug is in the ProfileMigrator.

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Consider deleted entries when migrating

This doesn't solve the issue I'm seeing, but the bug that lucasr pointed out is a real one.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Okay, I think I found the real problem. This is a mismatch between places and our new DB schema.

The places DB stores the favicons with only 1 URL: the one of the favicon itself. Places then refers to the favicon by rowid in the places table.

This means that every favicon has ONE favicon_url and ONE guid.

Our new browser.db stores both the url_key of the site and the url of the favicon, with a GUID, in 1 row.

This means that if two sites with a different url_key use the same favicon, those favicons would have *the same* GUID in the places.db, because they're the same entry, but this is not possible in our current schema. The GUID clash causes the constraint error.

Comment 16

[Chris Peterson [:cpeterson]]

I can reproduce this exception when doing Google searches on my Droid Pro.

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 608388 [details] [diff] [review]
Patch 1. Consider deleted entries when migrating

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

Patch lacks a bit of context, how does it ensure the same favicons for different pages will have different GUIDs?

Comment 18

[Gian-Carlo Pascutto [:gcp]]

>Patch lacks a bit of context, how does it ensure the same favicons for different 
>pages will have different GUIDs?

It doesn't - it has nothing to do with that (bug 717428). The patch is a direct fix for your comment 13.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/263150bd32bb

Comment 20

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/263150bd32bb

Comment 21

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/7573b4f06d48