Closed Bug 1411538 Opened 4 years ago Closed 3 years ago

Add a corrupt database recovery mode similar to the one in Places Maintenance

Categories

(Toolkit :: Places, enhancement, P1)

Product:
Toolkit
Component:
Places
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1411538 - Try to recover tables from a corrupt places.sqlite.
59 bytes, text/x-review-board-request
standard8
: review+
Details 
the idea is pretty much to implement automatically what is explained in https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/places.sqlite_Database_Troubleshooting#How_to_(try_to)_recover_from_a_corrupt_places.sqlite

If the db is found corrupt, we can try to copy its contents to a new clean db. then, on the next startup, replace the existing places.sqlite with the clean one.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Priority: P2 → P1
Depends on: 1422058
Comment on attachment 8936979 [details]
Bug 1411538 - Try to recover tables from a corrupt places.sqlite.

https://reviewboard.mozilla.org/r/207720/#review213596


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: toolkit/components/places/Database.cpp:960
(Diff revision 1)
> +  hasResult = false;
> +  while (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) {
> +    nsAutoCString query;
> +    rv = stmt->GetUTF8String(0, query);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = conn->ExecuteSimpleSQL(query);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Comment on attachment 8936979 [details]
Bug 1411538 - Try to recover tables from a corrupt places.sqlite.

Failures on try, not ready for review :(
Attachment #8936979 - Flags: review?(standard8)
Comment on attachment 8936979 [details]
Bug 1411538 - Try to recover tables from a corrupt places.sqlite.

https://reviewboard.mozilla.org/r/207720/#review213744


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: toolkit/components/places/Database.cpp:964
(Diff revision 2)
> +  hasResult = false;
> +  while (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) {
> +    nsAutoCString query;
> +    rv = stmt->GetUTF8String(0, query);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = conn->ExecuteSimpleSQL(query);

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Comment on attachment 8936979 [details]
Bug 1411538 - Try to recover tables from a corrupt places.sqlite.

https://reviewboard.mozilla.org/r/207720/#review214226

Generally I think the idea here is fine, though if there's any specific concerns you want to talk through let me know.

There's still the static analysis issue to fix as well.

::: toolkit/components/places/tests/unit/test_database_replaceOnStartup.js:8
(Diff revision 2)
>  
>  // Tests that history initialization correctly handles a request to forcibly
>  // replace the current database.
>  
> -function run_test() {
> -  // Ensure that our database doesn't already exist.
> +add_task(async function() {
> +  let clone = Services.prefs.getBoolPref("places.database.cloneOnCorruption", true);

It might be nice to have an additional test for the no-clone case. Then if we have to set the pref to false for whatever reason, we'll still know up front that the old way of doing things does what it is meant to.
Attachment #8936979 - Flags: review?(standard8) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/ec94d6e3e253
Try to recover tables from a corrupt places.sqlite. r=standard8
https://hg.mozilla.org/mozilla-central/rev/ec94d6e3e253
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.