Closed Bug 1410457 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Places, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

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.
Blocks: 1356531
Priority: -- → P1
Whiteboard: [fxsearch]
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.
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
the actual error we get is not SQLITE_BUSY, but SQLITE_LOCKED, not that it matters much.
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.
Attachment #8920626 - Flags: review?(paolo.mozmail) → review+
(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.
Pushed by mak77@bonardo.net:
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
https://hg.mozilla.org/mozilla-central/rev/c7cab42203df
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
Attachment #8920626 - Flags: approval-mozilla-beta?
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+
Attachment #8920626 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.