1186034 - Permission dataloss on nightlies between 1165263 and 1185340

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Description

[Ed Morley [:emorley]]

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?

Comment 1

[Ed Morley [:emorley]]

(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/

Comment 2

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 3

[Ed Morley [:emorley]]

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?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

[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 --

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Re-migrate users with permissions-database versions 5/6 to prevent dataloss

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Tests for new permission manager migration pathways

Comment 7

[Nika Layzell [:nika] (ni? for response)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f73f26f40193

Comment 8

[Nika Layzell [:nika] (ni? for response)]

I believe we're also going to need someone to look over this because of the new telemetry.

Comment 9

[(no longer active)]

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!

Comment 10

[Vladan Djeric (:vladan)]

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": ["[e-mail]"],
> +    "expires_in_version": "43",

you only want this in v42?

Comment 11

[(no longer active)]

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.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Re-migrate users with permissions-database versions 5/6 to prevent dataloss

Fixing nits, and extending the telemetry to cover 43 as well, just to be safe (as discussed with :vladan).

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89fbd9f70cf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/45f326aa8cd6

Comment 14

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=12037399&repo=mozilla-inbound

Comment 15

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fee0ec63f0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e089efed433c

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Sorry, I'm an idiot, I forgot that the patch I wrote for bug 1186909 had to land first.

Comment 17

[(no longer active)]

I can help land the stuff for you.  I think using checkin-needed is impractical for this stuff.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/783c6c78c458
https://hg.mozilla.org/integration/mozilla-inbound/rev/8daa0ce9c054

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/783c6c78c458
https://hg.mozilla.org/mozilla-central/rev/8daa0ce9c054

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Ed can you verify that the problem isn't happening anymore? Thanks!

Comment 21

[Ed Morley [:emorley]]

(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.