Closed Bug 715268 Opened 8 years ago Closed 8 years ago

ASSERTION: Someone added an entry without adding a GUID! due to a missing GUID in favicons

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla12
Tracking Status
firefox10 --- unaffected
firefox11 + verified

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [qa!])

Attachments

(1 file)

Both khuey and Ms2ger hit this assertion, and we figured it's due to a missing guid. This is bad, especially because the assertion is weak so nobody noticed it.

I'll check again all the code paths adding guids to favicons, we may need a further migration to fix profiles, but first of all need to figure out where these entries may come from.
A issue I found is that downgrading the user can create entries in moz_favicons that lack a guid, when upgrading again we don't fill those up, due to a too strict check in the migrator. and we end up with a messed up profile.
I overlooked this in the review :(
Drat. Sorry for inadequately considering downgrades!
Attached patch patch v1.0Splinter Review
we'll want this on FF11, since I don't want incomplete profile around, we had funny bugs related to those in the past.
Attachment #585915 - Flags: review?(dietrich)
Flags: in-testsuite+
Attachment #585915 - Flags: review?(dietrich) → review+
https://hg.mozilla.org/mozilla-central/rev/3c970a5c173c

Thanks for proving that complaints in #developers are the best way to fix assertions :)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 585915 [details] [diff] [review]
patch v1.0

[Approval Request Comment]
Regression caused by (bug #): bug 675996
User impact if declined: messed up profile missing favicons GUIDs
Testing completed (on m-c, etc.): m-i, m-c, xpcshell, breaking assert
Risk to taking this patch (and alternatives if risky): limited, will just fill up missing GUIDs on profile update.
Attachment #585915 - Flags: approval-mozilla-aurora?
Comment on attachment 585915 [details] [diff] [review]
patch v1.0

[Triage Comment]
Approving for Aurora given the possibility of unexpected behavior upon an incomplete profile.
Attachment #585915 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Is there something QA can do to verify this fix?
Whiteboard: [qa?]
upgrade from 10 to 11, downgrade to 10, visit some page with a favicon and check that a debug build on shutdown doesn't assert, otherwise check all moz_favicons table rows have a value in the guid column.
ehr, sorry, I meant downgrade from 11 to 10, visit a page with a favicon, then upgrade to 11 again and check... I inverted versions :/
Thanks Marco, qa+ to be verified using comment 11.
Whiteboard: [qa?] → [qa+]
Works for me using Firefox 11.0b5 -> 10.0.2 -> 11.0b5 as per comment 11.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.