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

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[trevan]

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.

Comment 1

[Marco Bonardo [:mak]]

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

Comment 2

[trevan]

(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.

Comment 3

[Marco Bonardo [:mak]]

please share those files with me, you can reach me at [e-mail], any kind of password protected archive through a link should do.

Comment 4

[trevan]

(In reply to Marco Bonardo [::mak] from comment #3)
> please share those files with me, you can reach me at [e-mail], 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?

Comment 5

[Marco Bonardo [:mak]]

I'll look into those files today and try to find the reason.

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[trevan]

(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.

Comment 9

[Marco Bonardo [:mak]]

Sync is not a backup system, so the result may not be what you expect.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 12

[Marco Bonardo [:mak]]

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.

Comment 15

[Marco Bonardo [:mak]]

Attachment: Bug 1432583 - Better corruption handling for favicons.sqlite.

Review commit: https://reviewboard.mozilla.org/r/244006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244006/

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8975809 [details]
Bug 1432583 - Better corruption handling for favicons.sqlite.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244006/diff/1-2/

Comment 17

[Mark Banner (:standard8)]

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/

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8975809 [details]
Bug 1432583 - Better corruption handling for favicons.sqlite.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244006/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2204626cd4f8
Better corruption handling for favicons.sqlite. r=standard8

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2204626cd4f8

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 22

[Marco Bonardo [:mak]]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Sounds perfectly sane to me, thanks for confirming :)