1613615 - Sqlite.jsm should forward ex.result in case of corruption or single error

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect, P1)

Description

[Marco Bonardo [:mak]]

We wrap statement execution errors in a global Error collecting all of them here:
https://searchfox.org/mozilla-central/rev/3a0a8e2762821c6afc1d235b3eb3dde63ad3b01a/toolkit/modules/Sqlite.jsm#889

The result of this is that we can't directly check ex.result against NS_ERROR_FILE_CORRUPTED

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1613615 - Forward ex.result from Sqlite.jsm when there's a single error, or for corruption. r=Standard8

Comment 2

[Marco Bonardo [:mak]]

I originally filed this in Places because we are actually hitting this case in PlacesDBUtils:
https://searchfox.org/mozilla-central/rev/3a0a8e2762821c6afc1d235b3eb3dde63ad3b01a/toolkit/components/places/PlacesDBUtils.jsm#122

It looks like this may be a common mistake to make, thus I prefer to fix it in Sqlite.jsm

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58522ee9dcbb
Forward ex.result from Sqlite.jsm when there's a single error, or for corruption. r=Standard8

Comment 4

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/58522ee9dcbb

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 9124681 [details]
Bug 1613615 - Forward ex.result from Sqlite.jsm when there's a single error, or for corruption. r=Standard8

Beta/Release Uplift Approval Request

• User impact if declined: In some cases Places won't replace a corrupt database, because we don't catch the right error type. Even after integrity check runs and finds a file corruption, Places won't try to fix it, by replacing places.sqlite at the next startup, because of this bug.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: So, this would be nice to verify on the user side, but it's very complicate to actually do so, because it requires a database with a specific kind of corruption that we don't have. The patch is very simple and safe, on the other side.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The underlying API patch has a test, and it's quite a simple and safe patch because it's just setting a property on an error object.
• String changes made/needed:

Comment 6

[Pascal Chevrel:pascalc]

Comment on attachment 9124681 [details]
Bug 1613615 - Forward ex.result from Sqlite.jsm when there's a single error, or for corruption. r=Standard8

Looks low risk and should improve Firefox reliability, uplift approved for 74.0b3, thanks.

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ecc3d52581b9