Closed Bug 1357366 Opened 3 years ago Closed 3 years ago

Avoid a possible crash loop in Places Database corruption handling

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 54+ fixed
firefox53 --- unaffected
firefox54 + fixed
firefox55 + fixed

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file)

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.
I'm settin esr52 on the assumption bug 1355414 will be approved for it.
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.
Attachment #8859139 - Flags: review?(past) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/08cfcd3c28c0
Avoid a possible crash loop in Places Database corruption handling. r=past
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
Attachment #8859139 - Flags: approval-mozilla-esr52?
Attachment #8859139 - Flags: approval-mozilla-aurora?
Comment on attachment 8859139 [details]
Bug 1357366 - Avoid a possible crash loop in Places Database corruption handling.

54 was merged to Beta today.
Attachment #8859139 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/08cfcd3c28c0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Attachment #8859139 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Tracking 54/55+ to avoid the possible crash loop in Places.
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.
Flags: qe-verify?
Flags: needinfo?(mak77)
(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.
Flags: needinfo?(mak77)
(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.
Flags: qe-verify? → qe-verify-
Attachment #8859139 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.