Avoid a possible crash loop in Places Database corruption handling

RESOLVED FIXED in Firefox -esr52

Status

()

Toolkit
Places
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52? fixed, firefox53 unaffected, firefox54+ fixed, firefox55+ fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
tracking-firefox-esr52: --- → ?
Comment hidden (mozreview-request)
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+

Comment 4

4 months ago
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?

Comment 7

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/08cfcd3c28c0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 8

4 months ago
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.
tracking-firefox54: ? → +
tracking-firefox55: ? → +

Comment 10

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/1a74650efe11
status-firefox54: affected → fixed
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-

Updated

3 months ago
Attachment #8859139 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 14

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/0b855945ce34
status-firefox-esr52: affected → fixed
You need to log in before you can comment on or make changes to this bug.