1038960 - Unable to pin top site: foreign key constraint failed; bookmark is missing pinned parent?

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Activity Stream, defect, P5)

Description

[Richard Newman [:rnewman]]

https://crash-stats.mozilla.com/report/index/0542af43-25fb-4189-80f3-e1bb02140715
https://crash-stats.mozilla.com/report/index/fd77e865-7696-4537-90c7-aa4032140715

android.database.sqlite.SQLiteConstraintException: foreign key constraint failed (code 19)
	at android.database.sqlite.SQLiteConnection.nativeExecuteForLastInsertedRowId(Native Method)
	at android.database.sqlite.SQLiteConnection.executeForLastInsertedRowId(SQLiteConnection.java:782)
	at android.database.sqlite.SQLiteSession.executeForLastInsertedRowId(SQLiteSession.java:788)
	at android.database.sqlite.SQLiteStatement.executeInsert(SQLiteStatement.java:86)
	at android.database.sqlite.SQLiteDatabase.insertWithOnConflict(SQLiteDatabase.java:1469)
	at android.database.sqlite.SQLiteDatabase.insertOrThrow(SQLiteDatabase.java:1365)
	at org.mozilla.gecko.db.BrowserProvider.insertBookmark(BrowserProvider.java:942)
	at org.mozilla.gecko.db.BrowserProvider.updateInTransaction(BrowserProvider.java:541)
	at org.mozilla.gecko.db.AbstractTransactionalProvider.update(AbstractTransactionalProvider.java:269)
	at android.content.ContentProvider$Transport.update(ContentProvider.java:287)
	at android.content.ContentResolver.update(ContentResolver.java:1316)
	at org.mozilla.gecko.db.LocalBrowserDB.pinSite(LocalBrowserDB.java:1515)
	at org.mozilla.gecko.db.BrowserDB.pinSite(BrowserDB.java:400)
	at org.mozilla.gecko.home.TopSitesPanel$3.run(TopSitesPanel.java:332)
	at android.os.Handler.handleCallback(Handler.java:733)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:136)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)

Comment 1

[Richard Newman [:rnewman]]

This failing constraint implies one of two things:

* The pinned items root has been removed or never created.
* The pinned items root was created with a different ID.

Comment 2

[Richard Newman [:rnewman]]

Any ideas, chaps?

Comment 3

[Lucas Rocha (:lucasr)]

The pinned sites folder is created on database creation or in a schema migration. Maybe this crash happens if a migration fails before the pinned sites folder is created? e.g. when you upgrade from a really old version of Fennec. IIRC, we added pinned sites support in Fx14.

Comment 4

[Thomas Stache]

Just tested this on another device, same crash:
https://crash-stats.mozilla.com/report/index/9108bfba-00f6-499f-bb47-044c32140716
(btw, still being attributed to bug 961526)

First was a Nexus7, this was a Nexus4 which I factory-restored like 1-2 months ago. So no update from really-old-firefox.

Comment 5

[Thomas Stache]

(In reply to Richard Newman [:rnewman] from comment #1)
> This failing constraint implies one of two things:
> 
> * The pinned items root has been removed or never created.
> * The pinned items root was created with a different ID.

Maybe relevant:
the devices are synced with the FxAccount system. I went through a bookmark restore from the desktop relatively recently because I used an old profile on one PC which uploaded a lot of stale data. 
Can Sync cause the above?

Comment 6

[Richard Newman [:rnewman]]

It shouldn't, but that's good to know when trying to repro.

Comment 7

[Markus Popp]

Here is another crash report from when I tried to pin a site to the Fennec Aurora (as of 2014-08-25) home screen on my Nexus 5:

https://crash-stats.mozilla.com/report/index/432d524a-82cf-4051-bafe-237f62140826

Comment 8

[Markus Popp]

Maybe what I just found is interesting for tracking this bug down.

On both my Nexus 5 and Nexus 7 I had Fennec Aurora 33 installed with all my bookmarks and passwords synced and Firebug installed (so my full featured Firefox Mobile) and I also had Fennec Nightly 34 which is basically a clean install. Pinning a site on Aurora's home screen crashed Aurora while pinning a site on Nightly's home screen worked fine.

I just updated Aurora to 34 and Nightly to 35. With Nightly (unsurprisingly) nothing changed, however I expected that the crash would disappear on Aurora since this is now version 34 which previously (being Nightly) didn't crash. But it still does. So this makes it look as if my synced bookmarks and/or passwords might play a role in this and supporting the theory which Thomas Stache mentioned in Comment 5.

Everything I described is the same on Nexus 5 and Nexus 7, which have Aurora and Nightly set up the same way.

Comment 9

[Markus Popp]

On September 3rd I removed my Fx Sync account and created a new one. All worked fine until yesterday when the same problem surfaced again:

https://crash-stats.mozilla.com/report/index/ef38407a-3277-4123-976e-68c932141008

Now I have my next account.

Another Sync problem which I saw coming back: the order of folders and bookmarks messing up. For now this is also fixed (for myself) by creating this new account. But there certainly is another problem, so I should probably let you know.

Comment 10

[Richard Newman [:rnewman]]

(In reply to Markus Popp from comment #9)

> Another Sync problem which I saw coming back: the order of folders and
> bookmarks messing up. For now this is also fixed (for myself) by creating
> this new account. But there certainly is another problem, so I should
> probably let you know.

Long-known, systemic, still unfixed, as I'm sure you're aware.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 12

[Richard Newman [:rnewman]]

Assigned, and we should figure this out. Clearing P5.

Comment 14

[Markus Popp]

I think I found a way to make the crash reproducible:

* sync a set of bookmarks from Desktop to Firefox Android
* export bookmarks to JSON file
* make changes to bookmarks in JSON file
* import updated JSON file back to Firefox on Desktop
* sync bookmarks to Firefox Android again

As long as I update bookmarks individually right in Firefox (Desktop), there is no problem. But if bookmarks get changed outside of Firefox and imported back and synced, the crashes start to appear.

Only solution to fix it still seems to be to create a new Sync account and sync into a cleaned instance of Firefox Android.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I think I reproduced this (without crash) in Activity stream:

Reproduction notes:
- I can only reproduce on one device (my N5)
- This is a local build
- This profile had a lot of sync data with a bunch of imported bookmarks from desktop
- I disabled sync recently and cleared history data

---

It looks like we're trying to insert a bookmark:

> at org.mozilla.gecko.db.BrowserProvider.insertBookmark(BrowserProvider.java:1631)
> ...
> at org.mozilla.gecko.db.LocalBrowserDB.pinSiteForAS(LocalBrowserDB.java:1743)

who doesn't have a parent (via BrowserDatabaseHelper.createBookmarksTable):

> "FOREIGN KEY (" + Bookmarks.PARENT + ") REFERENCES " +
>                 TABLE_BOOKMARKS + "(" + Bookmarks._ID + ")" +
>                 ");");

Looking at LocalBrowserDb.pinSiteForAS, we use:

> values.put(Bookmarks.PARENT, Bookmarks.FIXED_PINNED_LIST_ID);

Where Bookmarks.FIXED_PINNED_LIST_ID = -3. I'm guessing this bookmark._id doesn't exist for some reason.

---

Richard, do you have an opinion about how we would move forward here if we chose to?

---

Adding to AS triage list. According to [1], this crash has happened 47 times in the past 7 days, from the old top sites.

[1]: https://crash-stats.mozilla.com/signature/?_sort=-date&signature=android.database.sqlite.SQLiteConstraintException%3A%20foreign%20key%20constraint%20failed%20%28code%2019%29%20at%20android.database.sqlite.SQLiteConnection.nativeExecuteForChangedRowCount%28Native%20Method%29&date=%3E%3D2017-08-08T01%3A42%3A00.000Z&date=%3C2017-08-15T01%3A42%3A00.000Z

Comment 17

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #16)
> I think I reproduced this (without crash) in Activity stream

There are lots of different causes for this same error. Expiring history seems to be the most common in crash stats:

android.database.sqlite.SQLiteConstraintException: foreign key constraint failed (code 19)
	at android.database.sqlite.SQLiteConnection.nativeExecuteForChangedRowCount(Native Method)
	at android.database.sqlite.SQLiteConnection.executeForChangedRowCount(SQLiteConnection.java:734)
	at android.database.sqlite.SQLiteSession.executeForChangedRowCount(SQLiteSession.java:754)
	at android.database.sqlite.SQLiteStatement.executeUpdateDelete(SQLiteStatement.java:64)
	at android.database.sqlite.SQLiteDatabase.executeSql(SQLiteDatabase.java:1674)
	at android.database.sqlite.SQLiteDatabase.execSQL(SQLiteDatabase.java:1603)
	at org.mozilla.gecko.db.BrowserProvider.expireHistory(BrowserProvider.java:464)
	at org.mozilla.gecko.db.BrowserProvider.deleteInTransaction(BrowserProvider.java:602)

These will be two bugs with the same crash signature, so you probably should file a second bug.

This expiration crash above might be due to a device with an old version of SQLite without foreign key support, so our `ON DELETE CASCADE` doesn't work. That requires SQLite 3.6.19 or newer, which shipped with 2.2, but who knows.

> > values.put(Bookmarks.PARENT, Bookmarks.FIXED_PINNED_LIST_ID);
> 
> Where Bookmarks.FIXED_PINNED_LIST_ID = -3. I'm guessing this bookmark._id
> doesn't exist for some reason.

PINNED isn't in the 'specials' list:

    private boolean isSpecialFolder(ContentValues values) {
        String guid = values.getAsString(Bookmarks.GUID);
        if (guid == null) {
            return false;
        }

        return guid.equals(Bookmarks.MOBILE_FOLDER_GUID) ||
               guid.equals(Bookmarks.MENU_FOLDER_GUID) ||
               guid.equals(Bookmarks.TOOLBAR_FOLDER_GUID) ||
               guid.equals(Bookmarks.UNFILED_FOLDER_GUID) ||
               guid.equals(Bookmarks.TAGS_FOLDER_GUID);
    }

so if you ever went through a call to migrateBookmarkFolder, it might have been discarded in some way.

 
> ---
> 
> Richard, do you have an opinion about how we would move forward here if we
> chose to?

Split the bug, figure out how the pinned folder got deleted, then fix expiration.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Richard Newman [:rnewman] from comment #17)
> (In reply to Michael Comella (:mcomella) from comment #16)
> > I think I reproduced this (without crash) in Activity stream
> 
> There are lots of different causes for this same error. Expiring history
> seems to be the most common in crash stats:

Sorry, I mislead you - I didn't understand how to use crash-stats and copied the wrong crash signature. The one I've attached (I believe) should be relevant to this bug.

The reason I felt my stack trace was related was comment 14, where I had took a similar process to get into the state where pinning a site will fail and log the "foreign key failed" stack trace.

> > > values.put(Bookmarks.PARENT, Bookmarks.FIXED_PINNED_LIST_ID);
> > 
> > Where Bookmarks.FIXED_PINNED_LIST_ID = -3. I'm guessing this bookmark._id
> > doesn't exist for some reason.
> 
> PINNED isn't in the 'specials' list:
> 
>     private boolean isSpecialFolder(ContentValues values) {
> 
> so if you ever went through a call to migrateBookmarkFolder, it might have
> been discarded in some way.

I don't know if this was relevant to the expireHistory section or my issue, but I think this code is only to get called when upgrading the database from 3 to 4, or 6 to 7 but my DB was only created from upstream in the last few weeks.

> figure out how the pinned folder got deleted

I think that's the direction we need to go here. Given comment 14 and my experience, I think the folder might get deleted as part of importing desktop bookmarks and syncing although there may be alternative STRs.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Also: 45 crashes in 7 days on mostly modern versions (most on 55, release, and all > 50).

Comment 20

[Richard Newman [:rnewman]]

Those signatures still include all kinds of things. For example,

https://crash-stats.mozilla.com/report/index/a0dabab4-e781-4b77-9a48-0ac200170813

is adding a visit while browsing:

Java Stack Trace 	

android.database.sqlite.SQLiteConstraintException: foreign key constraint failed (code 19)
	at android.database.sqlite.SQLiteConnection.nativeExecuteForLastInsertedRowId(Native Method)
	at android.database.sqlite.SQLiteConnection.executeForLastInsertedRowId(SQLiteConnection.java:905)
	at android.database.sqlite.SQLiteSession.executeForLastInsertedRowId(SQLiteSession.java:788)
	at android.database.sqlite.SQLiteStatement.executeInsert(SQLiteStatement.java:86)
	at android.database.sqlite.SQLiteDatabase.insertWithOnConflict(SQLiteDatabase.java:1469)
	at org.mozilla.gecko.db.BrowserProvider.insertVisit(BrowserProvider.java:1821)
	at org.mozilla.gecko.db.BrowserProvider.insertVisitForHistory(BrowserProvider.java:1803)
	at org.mozilla.gecko.db.BrowserProvider.updateInTransaction(BrowserProvider.java:807)
	at org.mozilla.gecko.db.AbstractTransactionalProvider.update(AbstractTransactionalProvider.java:227)
	at android.content.ContentProvider$Transport.update(ContentProvider.java:235)
	at android.content.ContentResolver.update(ContentResolver.java:990)
	at org.mozilla.gecko.db.LocalBrowserDB.updateVisitedHistory(LocalBrowserDB.java:756)
	at org.mozilla.gecko.GlobalHistory.add(GlobalHistory.java:129)
	at org.mozilla.gecko.GeckoApp$33.run(GeckoApp.java:2833)

"forChangedRowCount" will catch errors during UPDATE. "forLastInsertedRowId" will catch errors during INSERT. There are lots of causes for both.

Some of those causes will be related, of course.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Richard Newman [:rnewman] from comment #20)
> Those signatures still include all kinds of things. For example,

Thanks for the info. From my understanding, one effect of this is...

(In reply to Michael Comella (:mcomella) from comment #19)
> Also: 45 crashes in 7 days on mostly modern versions (most on 55, release,
> and all > 50).

The number of crashes attributed to pinning sites may be less than 45 in 7 days.

---

Unassigning Nick because I assume he's not working on it.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This is an existing bug with a low rate of crashing - we don't need to address this right now.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

All open Activity Stream bugs are moving from the whiteboard tag, "[mobileAS]", to the Firefox for Android component, "Activity Stream", so that I can keep better track of these bugs as the new triage owner; I will send out an email shortly with additional details, caveats, etc.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

P5: this has been around a while, has a low rate of crashing, and we haven't felt the need to fix it, so I don't think it's urgent.

Comment 25

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.