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

RESOLVED FIXED in Firefox 13

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
6 years ago
10 months ago

People

(Reporter: cpeterson, Assigned: blassey)

Tracking

Trunk
Firefox 14
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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?
(Reporter)

Comment 2

6 years ago
mobile/android/base/ProfileMigrator.java
>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.
(Reporter)

Comment 4

6 years ago
> 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 :-/
(Reporter)

Comment 6

5 years ago
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.
(Reporter)

Comment 8

5 years ago
> 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.
(Reporter)

Updated

5 years ago
Component: General → IME
(Reporter)

Comment 9

5 years ago
This is not an IME bug.
Component: IME → General
(Reporter)

Comment 10

5 years ago
This bug depends on or is a dupe of bug 734624.
Depends on: 734624
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.
(Reporter)

Comment 12

5 years ago
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.
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.
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.
(Reporter)

Comment 16

5 years ago
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.

Updated

5 years ago
Attachment #608388 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/263150bd32bb
https://hg.mozilla.org/mozilla-central/rev/263150bd32bb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/releases/mozilla-aurora/rev/7573b4f06d48
status-firefox13: --- → fixed
status-firefox14: --- → fixed
You need to log in before you can comment on or make changes to this bug.