1357366 - Avoid a possible crash loop in Places Database corruption handling

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

The current code in bug 1355414 sets a pref and crashes. That may not be enough to ensure the pref is written to disk, so I'd prefer to add a call to savePrefFile() to be sure.

Comment 1

[Marco Bonardo [:mak]]

I'm settin esr52 on the assumption bug 1355414 will be approved for it.

Comment 2

[Marco Bonardo [:mak]]

Attachment: Bug 1357366 - Avoid a possible crash loop in Places Database corruption handling.

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

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8859139 [details]
Bug 1357366 - Avoid a possible crash loop in Places Database corruption handling.

https://reviewboard.mozilla.org/r/131186/#review133732

LGTM.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08cfcd3c28c0
Avoid a possible crash loop in Places Database corruption handling. r=past

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8859139 [details]
Bug 1357366 - Avoid a possible crash loop in Places Database corruption handling.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a follow-up to bug 1355414.
User impact if declined: possible crash loop in case of corruption
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): no risk
String or UUID changes made by this patch: none

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8859139 [details]
Bug 1357366 - Avoid a possible crash loop in Places Database corruption handling.

54 was merged to Beta today.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/08cfcd3c28c0

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8859139 [details]
Bug 1357366 - Avoid a possible crash loop in Places Database corruption handling.

Fix a potential crash. Beta54+. Should be in 54 beta 1.

Comment 9

[Marcia Knous [:marcia]]

Tracking 54/55+ to avoid the possible crash loop in Places.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1a74650efe11

Comment 11

[Andrei Vaida [:avaida]]

Marco, is this something that would benefit from manual testing? Bug 1355414 doesn't seem to provide anything actionable for Release QA, but we'd like to be sure that we're not overlooking something -- especially since this is now in beta.

Comment 12

[Marco Bonardo [:mak]]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #11)
> Marco, is this something that would benefit from manual testing? Bug 1355414
> doesn't seem to provide anything actionable for Release QA, but we'd like to
> be sure that we're not overlooking something -- especially since this is now
> in beta.

to test and reproduce this you'd need a corrupt database that can be opened but fails one of the migration steps that follows an asynchronous migration step. It's not trivial to build one, the thunderbird case was one of those, but it's fixed in bug 1355414.
Off-hand I don't think this would benefit much from manual testing, and we'll get crash reports so we can check those out.

Comment 13

[Andrei Vaida [:avaida]]

(In reply to Marco Bonardo [::mak] from comment #12)
> to test and reproduce this you'd need a corrupt database that can be opened
> but fails one of the migration steps that follows an asynchronous migration
> step. It's not trivial to build one, the thunderbird case was one of those,
> but it's fixed in bug 1355414.
> Off-hand I don't think this would benefit much from manual testing, and
> we'll get crash reports so we can check those out.

Thank you for following up on this so quickly! We can check crash stats instead.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/0b855945ce34