Detect favicons.sqlite corruption in Maintenance, and clone/replace the db in case

RESOLVED FIXED in Firefox 62

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: trevan_github, Assigned: mak)

Tracking

(Blocks 1 bug, {dataloss})

58 Branch
mozilla62
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

Reporter

Description

a year ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20180103231032

Steps to reproduce:

I was running Firefox 57 and it auto upgraded to Firefox 58.  Once the upgrade was finished, all of my history was gone.  I checked the profile directory and noticed that there was now a places.sqlite.corrupt.  I tried closing Firefox and renaming places.sqlite.corrupt as places.sqlite but once Firefox opened, it moved it back to corrupt.  I reinstalled Firefox 57, renamed the places.sqlite, and now my history is back.



Expected results:

I would expect FF 58 to act like FF 57 in regards to the places.sqlite.

Updated

a year ago
Component: Untriaged → Places
Keywords: dataloss
Product: Firefox → Toolkit
Assignee

Comment 1

a year ago
Would you be comfortable with sharing your places.sqlite and favicons.sqlite files through private email? Without being able to look at the database it's hard to guess what may be the reason the v58 migration fails.
It's likely you have some asymptomatic corruption that the 58 migration hits. In such a case we couldn't do much.
You could try with this https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/places.sqlite_Database_Troubleshooting#How_to_(try_to)_recover_from_a_corrupt_places.sqlite
Assignee

Updated

a year ago
Flags: needinfo?(trevan_github)
Reporter

Comment 2

a year ago
(In reply to Marco Bonardo [::mak] from comment #1)
> Would you be comfortable with sharing your places.sqlite and favicons.sqlite
> files through private email?

Yes, I could share them through a private email.

> You could try with this
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/places.
> sqlite_Database_Troubleshooting#How_to_(try_to)_recover_from_a_corrupt_places
> .sqlite

I tried those steps.  The "PRAGMA integrity_check;" command returns "OK".

Weirdly enough, FF 57 upgraded again to FF 58 even though I told it to not upgrade anymore.  The places.sqlite went to places.sqlite.corrupted again so I had to revert back to FF 57 again.
Flags: needinfo?(trevan_github)
Assignee

Comment 3

a year ago
please share those files with me, you can reach me at mak@mozilla.com, any kind of password protected archive through a link should do.
Flags: needinfo?(trevan_github)
Reporter

Comment 4

a year ago
(In reply to Marco Bonardo [::mak] from comment #3)
> please share those files with me, you can reach me at mak@mozilla.com, any
> kind of password protected archive through a link should do.

I sent you an email containing a link.

One thing I just noticed is that I can't delete entries in my history.  I can browse and search, but I can't delete.  Maybe that is related in some way?
Flags: needinfo?(trevan_github)
Assignee

Updated

a year ago
Blocks: 1410877
Assignee

Comment 5

a year ago
I'll look into those files today and try to find the reason.
Flags: needinfo?(mak77)
Assignee

Comment 6

a year ago
So, looks like your favicons.sqlite database is badly corrupted. This may be caused by various reasons, among which memory corruption (either due to bad sticks or unstable overclock) or disk corruption. For those I'd suggest do run some checks on your hardware (there are various forums that can help with that).

The reason the upgrade to 58 fails while the 57 doesn't, is that one of the migrations in 58 touches favicons, and hits the corrupt database.

For the contingent problem, you can try to delete favicons.sqlite, and then let Firefox generate a new one. You'll lose icons, but you can recover those by revisiting the pages/bookmarks. This should allow you to upgrade to Firefox 58.
Flags: needinfo?(mak77)
Summary: places.sqlite was corrupted with FF 58 but not FF 57 → Better handle favicons.sqlite corruptions
Assignee

Comment 7

a year ago
We don't currently handle properly a favicons database corruption in maintenance, we should detect it and replace the db on startup like we do with places.sqlite.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Summary: Better handle favicons.sqlite corruptions → Detect favicons.sqlite corruption in Maintenance, and clone/replace the db in case
Whiteboard: [fxsearch]
Reporter

Comment 8

a year ago
(In reply to Marco Bonardo [::mak] from comment #6)
> For the contingent problem, you can try to delete favicons.sqlite, and then
> let Firefox generate a new one. You'll lose icons, but you can recover those
> by revisiting the pages/bookmarks. This should allow you to upgrade to
> Firefox 58.

That worked.  I'm on FF 58 and my history is all there.  Thanks.

I had also tried to work around this issue by using Firefox Sync.  I synced my history, disconnected sync, upgraded, reconnected sync, and hoped my history would re-appear.  But it didn't do that.  I wonder if Sync could be used to rebuild corrupted places.
Assignee

Comment 9

a year ago
Sync is not a backup system, so the result may not be what you expect.
(In reply to trevan from comment #8)
> I had also tried to work around this issue by using Firefox Sync.  I synced
> my history, disconnected sync, upgraded, reconnected sync, and hoped my
> history would re-appear.  But it didn't do that.  I wonder if Sync could be
> used to rebuild corrupted places.

As Mak said, Sync isn't a backup. That process should have restored your bookmarks, but Sync only ever considers a subset of history given the volume of entries most users have.
Duplicate of this bug: 1443111
Assignee

Comment 12

a year ago
We should also try to detect if startup fails due to favicons.sqlite, since in case we can't startup then PlacesDBUtils may be unable to act on the db. Indeed, it may want to check coherence on a separate connection.
Assignee

Updated

a year ago
Priority: P2 → P1
Assignee

Updated

a year ago
Duplicate of this bug: 1441280
Assignee

Updated

a year ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee

Updated

a year ago
Duplicate of this bug: 1460000
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

a year ago
mozreview-review
Comment on attachment 8975809 [details]
Bug 1432583 - Better corruption handling for favicons.sqlite.

https://reviewboard.mozilla.org/r/244006/#review250630

Looks good. The logic seems to make sense. Just a few small nits to address.

::: toolkit/components/places/Database.h:275
(Diff revision 2)
>     *
>     * @param aStorage
>     *        mozStorage service instance.
>     */
> -  nsresult TryToCloneTablesFromCorruptDatabase(nsCOMPtr<mozIStorageService>& aStorage);
> +  nsresult TryToCloneTablesFromCorruptDatabase(const nsCOMPtr<mozIStorageService>& aStorage,
> +                                               const nsCOMPtr<nsIFile>& aDatabaseFile);

nit: please add aDatabaseFile to jsdoc.

::: toolkit/components/places/Database.cpp:154
(Diff revision 2)
>  /**
>   * Checks whether exists a database backup created not longer than
>   * RECENT_BACKUP_TIME_MICROSEC ago.
>   */
>  bool
> -hasRecentCorruptDB()
> +hasRecentCorruptFile(const nsCOMPtr<nsIFile>& aCorruptFile)

This feels a bit more like `isRecentCorruptFile` or `recentCorruptFileExists` rather than a "has".

::: toolkit/components/places/PlacesDBUtils.jsm:1084
(Diff revision 2)
> +  }
> +
> +  // Create a new connection for this check, so we can operate independently
> +  // from a broken Places service.
> +  // openConnection returns an exception with .status == Cr.NS_ERROR_FILE_CORRUPTED,
> +  // we should do the same everywhere we want maintance to try replacing the

nit: s/maintance/maintenance/

::: toolkit/components/places/tests/maintenance/head.js:29
(Diff revision 2)
> +  let dir = await OS.File.getCurrentDirectory();
> +  let src = OS.Path.join(dir, "corruptDB.sqlite");
> +  await OS.File.copy(src, path);
> +}
> +
> +async function test_database_replacement(src, filename, shouldClone, expectedStatus) {

Please add jsdoc for this, for example, it isn't immediately obvious what `src` is.

::: toolkit/components/places/tests/maintenance/head.js:52
(Diff revision 2)
> +  await OS.File.copy(src, dest);
> +
> +  // Create some unique stuff to check later.
> +  let db = await Sqlite.openConnection({ path: dest });
> +  await db.execute(`CREATE TABLE moz_cloned(id INTEGER PRIMARY KEY)`);
> +  await db.execute(`CREATE TABLE not_cloned (id INTEGER PRIMARY KEY)`);

nit: (this was in the original) there's a space before the open bracket here, but there isn't on the line above.

::: toolkit/components/places/tests/maintenance/head.js:70
(Diff revision 2)
> +    await db.execute(`DELETE FROM moz_cloned`); // Shouldn't throw.
> +  }
> +
> +  // Check the new database is really a new one.
> +  await Assert.rejects(db.execute(`DELETE FROM not_cloned`),
> +                       "The database should have been replaced");

Please insert the second parameter here to check the actual exception thrown is what we expect (bug 
1452706 is rolling this out as a requirement).

::: toolkit/components/places/tests/maintenance/test_corrupt_favicons.js:5
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests that history initialization correctly handles a corrupt favicons file
> +// that can't be open.

nit: s/open/opened/
Attachment #8975809 - Flags: review?(standard8) → review+
Comment hidden (mozreview-request)

Comment 19

a year ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/2204626cd4f8
Better corruption handling for favicons.sqlite. r=standard8

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2204626cd4f8
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
I'm going to assume that this is something we want to ride the trains, but I'd be open to considering it for 61 depending on the risk involved.
Assignee

Comment 22

a year ago
I'd love to have this on a previous version, but it's just too risky since it touches places startup, imo. I'd prefer having full testing.
Sounds perfectly sane to me, thanks for confirming :)
You need to log in before you can comment on or make changes to this bug.