Closed Bug 716729 Opened 10 years ago Closed 9 years ago
Identify root cause of favicon crash on some Honeycomb/ICS devices' Bookmarks database
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.
Chris, what do you mean by "bookmarks migration"? Moving bookmarks from fennec to Fennec's native local DB?
>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.
> 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.
Assignee: nobody → cpeterson
Priority: -- → P3
This is becoming a bit of a pain for adding transaction support :-/
gcp, this favicon crash is causing problems for transaction support? Should we increase the priority of this bug?
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.
> 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.
This is not an IME bug.
Component: IME → General
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.
Reassigning to blassey because related bug 734624 is assigned to him.
Assignee: cpeterson → blassey.bugs
(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.
This doesn't solve the issue I'm seeing, but the bug that lucasr pointed out is a real one.
Attachment #608388 - Flags: review?(lucasr.at.mozilla)
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.
I can reproduce this exception when doing Google searches on my Droid Pro.
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?
>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.
Attachment #608388 - Flags: review?(lucasr.at.mozilla) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.