Last Comment Bug 716729 - Identify root cause of favicon crash on some Honeycomb/ICS devices' Bookmarks database
: Identify root cause of favicon crash on some Honeycomb/ICS devices' Bookmarks...
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: P3 normal (vote)
: Firefox 14
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
Mentors:
Depends on: 712791 734624
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-09 16:04 PST by Chris Peterson [:cpeterson]
Modified: 2012-04-04 10:20 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Patch 1. Consider deleted entries when migrating (2.61 KB, patch)
2012-03-22 10:41 PDT, Gian-Carlo Pascutto [:gcp]
lucasr.at.mozilla: review+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2012-01-09 16:04:42 PST
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) 2012-01-10 03:06:07 PST
Chris, what do you mean by "bookmarks migration"? Moving bookmarks from fennec to Fennec's native local DB?
Comment 2 Chris Peterson [:cpeterson] 2012-01-10 10:52:31 PST
mobile/android/base/ProfileMigrator.java
Comment 3 Gian-Carlo Pascutto [:gcp] 2012-01-10 11:05:01 PST
>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] 2012-01-10 11:46:45 PST
> 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] 2012-03-05 09:32:55 PST
This is becoming a bit of a pain for adding transaction support :-/
Comment 6 Chris Peterson [:cpeterson] 2012-03-05 10:47:21 PST
gcp, this favicon crash is causing problems for transaction support? Should we increase the priority of this bug?
Comment 7 Gian-Carlo Pascutto [:gcp] 2012-03-05 10:55:21 PST
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] 2012-03-05 13:10:47 PST
> 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] 2012-03-13 12:52:13 PDT
This is not an IME bug.
Comment 10 Chris Peterson [:cpeterson] 2012-03-20 11:33:31 PDT
This bug depends on or is a dupe of bug 734624.
Comment 11 Gian-Carlo Pascutto [:gcp] 2012-03-20 12:05:35 PDT
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] 2012-03-20 15:09:25 PDT
Reassigning to blassey because related bug 734624 is assigned to him.
Comment 13 Lucas Rocha (:lucasr) 2012-03-21 08:41:18 PDT
(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] 2012-03-22 10:41:50 PDT
Created attachment 608388 [details] [diff] [review]
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] 2012-03-22 10:49:26 PDT
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] 2012-03-22 15:30:25 PDT
I can reproduce this exception when doing Google searches on my Droid Pro.
Comment 17 Lucas Rocha (:lucasr) 2012-03-26 08:52:36 PDT
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] 2012-03-26 08:57:20 PDT
>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] 2012-03-26 10:08:14 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/263150bd32bb
Comment 20 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-03-27 05:24:28 PDT
https://hg.mozilla.org/mozilla-central/rev/263150bd32bb
Comment 21 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-04 10:20:25 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/7573b4f06d48

Note You need to log in before you can comment on or make changes to this bug.