Closed
Bug 1186034
Opened 9 years ago
Closed 9 years ago
Permission dataloss on nightlies between 1165263 and 1185340
Categories
(Core :: Permission Manager, defect)
Core
Permission Manager
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: emorley, Assigned: nika)
References
Details
(Keywords: dataloss, regression)
Attachments
(2 files, 1 obsolete file)
51.13 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
24.72 KB,
patch
|
Details | Diff | Splinter Review |
In the privacy prefs I use the cookie "Keep until I close Nightly" pref, and then explicitly list sites in the "Exceptions" section who I wish to be able to set cookies. In the last few days these site's cookies are no longer kept after restarting Nightly, causing me to have to sign into sites, which is pretty frustrating. I'm guessing bug 1173523 could be at fault here?
Reporter | ||
Comment 1•9 years ago
|
||
(In reply to Ed Morley [:emorley] from comment #0) > then explicitly list sites in the "Exceptions" section who I wish to be able > to set cookies. s/to set cookies/to set cookies that last beyond the Nightly session/
Assignee | ||
Comment 2•9 years ago
|
||
This is probably actually the fault of 1165263 as the exceptions you are setting are likely being set for http://HOST, and you are probably visiting on https://HOST. As cookies are based on host, this means that the cookie permission system's interaction with the permission manager is now wrong, as the permission manager now cares about scheme and port. We should probably change the cookie permission system to instead normalize the scheme and port of URIs it uses to test the permissions manager to "http://" and "80", so that only the host is considered when testing cookie permissions.
Assignee: nobody → michael
Reporter | ||
Comment 3•9 years ago
|
||
I've not modified the list at all recently, these sites were added some time ago. However looking at the exceptions list, several sites are now missing? (eg facebook.com, github.com) The remaining entries are now doubled up, one for http and one for https. Is there a bug in the migration?
Assignee | ||
Comment 4•9 years ago
|
||
[Tracking Requested - why for this release]: Permissions migrated during the nightlies over the weekend (after 1165263 landed but before 1185340) suffered from a dataloss problem. The permission migration code incorrectly only allowed one permission to be inserted per origin, meaning that any origin with multiple permissions would only have one permission inserted for it. This problem is fixed for new migrations as of bug 1185340. This bug is for a fix to re-migrate users who were migrated using the old migration code from the backup table, to avoid the severe dataloss. Steps to reproduce: 1. Set multiple permissions for a single host on a version of firefox from before bug 1165263 landed (e.g. Aurora41.0a2). 2. Start firefox nightly from after bug 1165263 landed, but before bug 1185340 landed. -- All but one of the permissions for that host are lost --
Severity: normal → critical
tracking-firefox42:
--- → ?
Component: Preferences → Permission Manager
Keywords: dataloss
Product: Firefox → Core
Summary: Previously whitelisted sites no longer retain cookies when cookie "Keep until I close Nightly" set → Permission dataloss on nightlies between 1165263 and 1185340
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(michael)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8637401 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8637402 -
Flags: review?(ehsan)
Assignee | ||
Comment 7•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f73f26f40193
Flags: needinfo?(michael)
Assignee | ||
Comment 8•9 years ago
|
||
I believe we're also going to need someone to look over this because of the new telemetry.
Flags: needinfo?(vdjeric)
Comment 9•9 years ago
|
||
Comment on attachment 8637401 [details] [diff] [review] Part 1: Re-migrate users with permissions-database versions 5/6 to prevent dataloss Review of attachment 8637401 [details] [diff] [review]: ----------------------------------------------------------------- Vladan, can you please review the telemetry changes? Thanks!
Attachment #8637401 -
Flags: review?(vdjeric)
Comment 10•9 years ago
|
||
Comment on attachment 8637401 [details] [diff] [review] Part 1: Re-migrate users with permissions-database versions 5/6 to prevent dataloss Review of attachment 8637401 [details] [diff] [review]: ----------------------------------------------------------------- r+ on the data collection review ::: toolkit/components/telemetry/Histograms.json @@ +8492,5 @@ > "description": "Reports why a graphics sanity test was run. 0=First Run, 1=App Updated, 2=Device Change, 3=Driver Change." > + }, > + "PERMISSIONS_REMIGRATION_COMPARISON": { > + "alert_emails": ["michael@thelayzells.com"], > + "expires_in_version": "43", you only want this in v42?
Attachment #8637401 -
Flags: review?(vdjeric) → review+
Updated•9 years ago
|
Flags: needinfo?(vdjeric)
Comment 11•9 years ago
|
||
Comment on attachment 8637401 [details] [diff] [review] Part 1: Re-migrate users with permissions-database versions 5/6 to prevent dataloss Review of attachment 8637401 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/cookie/nsPermissionManager.cpp @@ +1055,5 @@ > + // re-migration. We count the rows in the old table for telemetry, > + // and then back up their old database as moz_perms_v6 > + > + nsCOMPtr<mozIStorageStatement> countStmt; > + mDBConn->CreateStatement(NS_LITERAL_CSTRING("SELECT count(*) FROM moz_perms"), Nit: COUNT please, as that's a built-in SQL function.
Attachment #8637401 -
Flags: review?(vdjeric)
Attachment #8637401 -
Flags: review?(ehsan)
Attachment #8637401 -
Flags: review+
Assignee | ||
Comment 12•9 years ago
|
||
Fixing nits, and extending the telemetry to cover 43 as well, just to be safe (as discussed with :vladan).
Attachment #8637401 -
Attachment is obsolete: true
Attachment #8637401 -
Flags: review?(vdjeric)
Updated•9 years ago
|
Attachment #8637402 -
Flags: review?(ehsan) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/89fbd9f70cf8 https://hg.mozilla.org/integration/mozilla-inbound/rev/45f326aa8cd6
Keywords: checkin-needed
Comment 14•9 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=12037399&repo=mozilla-inbound
Flags: needinfo?(michael)
Comment 15•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/2fee0ec63f0e https://hg.mozilla.org/integration/mozilla-inbound/rev/e089efed433c
Assignee | ||
Comment 16•9 years ago
|
||
Sorry, I'm an idiot, I forgot that the patch I wrote for bug 1186909 had to land first.
Depends on: 1186909
Flags: needinfo?(michael)
Comment 17•9 years ago
|
||
I can help land the stuff for you. I think using checkin-needed is impractical for this stuff.
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/783c6c78c458 https://hg.mozilla.org/integration/mozilla-inbound/rev/8daa0ce9c054
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/783c6c78c458 https://hg.mozilla.org/mozilla-central/rev/8daa0ce9c054
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 20•9 years ago
|
||
Ed can you verify that the problem isn't happening anymore? Thanks!
Flags: needinfo?(emorley)
Reporter | ||
Comment 21•9 years ago
|
||
(In reply to Liz Henry (:lizzard) from comment #20) > Ed can you verify that the problem isn't happening anymore? Thanks! I had to fix a bunch of my permissions by hand even after this landed, but most people won't have to be re-migrated, so don't know if it would affect others. Michael/Ehsan have my places and perms DB, so could say for sure.
Flags: needinfo?(emorley)
Updated•9 years ago
|
Keywords: regressionwindow-wanted
You need to log in
before you can comment on or make changes to this bug.
Description
•