1410457 - Schema migration may mark the database as corrupt due to a SQLITE_BUSY error.

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I just found this case while working on bug 1356531.

If a schema migration executes something asynchronously that requires a transaction, the next migration step may fail due to statements failing with SQLITE_BUSY.
We just introduced such a case in Firefox 55 to migrate icons :(
That means upgrading to 56 or later is at risk of marking the database as corrupt, even if it's not.

In the future we should avoid or delay any async executions in InitSchema.
I'm looking into adding some code to prevent the same issue from marking things as corrupt in the future, but it's tricky because if the db is not corrupt and not upgraded, it can only be marked as locked, and it may not be able to exit from that state.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

This is pretty bad and more than worth an uplift. I'm not sure whether we should consider a chemspill to 56, but for sure it's something we should handle in 57.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Bug 1410457 - Places.sqlite may be marked as corrupt if schema migration mixes up sync and async execution.

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

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Unfortunately it's pretty much impossible to test this. I could test it as working using my work in bug 1356531, but clearly because I could reproduce the failure that this is fixing.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the actual error we get is not SQLITE_BUSY, but SQLITE_LOCKED, not that it matters much.

Comment 5

[:Paolo Amadini]

Comment on attachment 8920626 [details]
Bug 1410457 - Places.sqlite may be marked as corrupt if schema migration mixes up sync and async execution.

https://reviewboard.mozilla.org/r/191620/#review196882

Makes sense to me, but I have a question.

::: toolkit/components/places/Database.cpp:1124
(Diff revision 1)
>          NS_ENSURE_SUCCESS(rv, rv);
>        }
>  
>        if (currentSchemaVersion < 37) {
>          rv = MigrateV37Up();
>          NS_ENSURE_SUCCESS(rv, rv);

If the migration fails after this point, we'll leave mShouldConvertIconPayloads to true but never perform the conversion. If this procedure is only executed once in the main process and never repeated with different databases, I don't think it can have any adverse effects, but it's worth noting.

Something to think about, though, is whether the conversion should be executed anyways if any of the later migration steps fail.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to :Paolo Amadini from comment #5)
> If the migration fails after this point, we'll leave
> mShouldConvertIconPayloads to true but never perform the conversion. If this
> procedure is only executed once in the main process and never repeated with
> different databases, I don't think it can have any adverse effects, but it's
> worth noting.

You're right, I'll move the work into a ScopeExit guard created before any migration steps, so it will start regardless.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8920626 [details]
Bug 1410457 - Places.sqlite may be marked as corrupt if schema migration mixes up sync and async execution.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7cab42203df
Places.sqlite may be marked as corrupt if schema migration mixes up sync and async execution. r=Paolo

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c7cab42203df

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8920626 [details]
Bug 1410457 - Places.sqlite may be marked as corrupt if schema migration mixes up sync and async execution.

Approval Request Comment
[Feature/Bug causing the regression]: New favicons migration in 55
[User impact if declined]: Upgrading to 56 or 57 may fail and mark places.sqlite as corrupt even if it's not
[Is this code covered by automated tests?]: no, it's non-trivial since it depends on thread scheduling order
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no, it's not easily reproducible manually
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not particularly
[Why is the change risky/not risky?]: it's just moving a function call after a critical code area and nit-picking some error values
[String changes made/needed]: none

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8920626 [details]
Bug 1410457 - Places.sqlite may be marked as corrupt if schema migration mixes up sync and async execution.

This fix makes the update to 57 more robust, taking it, beta57+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4d583d584b5c