Despite my allowing notifications, Slack now prompts for permission to send notifications at every startup. mozregression tells me this was caused by bug 1320404.

13:58.56 INFO: Last good revision: 70b26c05406e45df7299bdba32e85a4678e3bf5c
13:58.56 INFO: First bad revision: e720637a07b85685422677f0d5dc7946c0887673
13:58.56 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=70b26c05406e45df7299bdba32e85a4678e3bf5c&tochange=e720637a07b85685422677f0d5dc7946c0887673

Unfortunately I can reproduce in my normal profile but not in a new profile.

And if it makes any difference, I have Slack in a pinned container tab.

I started seeing this with the update to the 2019-05-10 21:49:31 nightly, whereas I hadn't seen it with any of the previous nightlies (and I update twice a day pretty reliably).

Ed, in bug 1550004 you said that you are able to reproduce it consistently. Can you share your permission.sqlite (directly to me by email) and a STR? Thanks.

I was looking into my permissions.sqlite, and I think I found the cause.

sqlite> pragma user_version;
9

A new profile results in 10.

STR:
1) create a new profile
2) go to about:preferences#privacy-sitedata -> Manage Permissions
3) add "https://www.mozilla.org/" then Allow then Save Changes
4) quit firefox
5) run `sqlite permissions.sqlite "pragma user_version=9"
6) start firefox go back to about:preferences#privacy-sitedata -> Manage Permissions
7) see no permissions

For my permissions.sqlite from my profile that's been around for years.. even after clearing out the data, I see the regression, so it seems to be independent of table contents/data:

$ sqlite3 permissions.sqlite .dump
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE IF NOT EXISTS "moz_perms_v6" ( id INTEGER PRIMARY KEY,origin TEXT,type TEXT,permission INTEGER,expireType INTEGER,expireTime INTEGER,modificationTime INTEGER);
CREATE TABLE IF NOT EXISTS "moz_perms" ( id INTEGER PRIMARY KEY,origin TEXT,type TEXT,permission INTEGER,expireType INTEGER,expireTime INTEGER,modificationTime INTEGER);
COMMIT;

$ sqlite3 permissions.sqlite "PRAGMA user_version"
9

Just guessing, https://hg.mozilla.org/mozilla-central/rev/b0f584907e03#l1.179

+            "SELECT id, host, type, permission, expireType, expireTime, "
+            "modificationTime, isInBrowserElement FROM moz_hosts WHERE appId = "

My sqlite doesn't have a moz_hosts to select from

Great! Thanks for your debugging. I have a patch to submit.

baku, can you please explain the patch? thanks.

The problem I would like to fix is the downgrade from version 10 to version 9. When we go from 9 to 10, we have to remove the appId column from moz_hosts. SQLite doesn't support the deletion of columns, so, the current approach is to create a temporary table (moz_hosts_v9) without appId, and copy records from the existing table into the new one:

"INSERT INTO moz_hosts_v9 (id, host, type, permission, expireType, expireTime, modificationTime, isInBrowserElement) SELECT id, host, type, permission, expireType, expireTime, modificationTime, isInBrowserElement FROM moz_hosts WHERE appId = 0"

After that, moz_hosts table is removed and moz_hosts_v9 is renamed to moz_hosts. This works fine.

Now, in the STR, Ed forced version 9 to simulate a version downgrade. At the next opening, we are not able to execute that SQL query because appId column would not exist anymore. In the patch, I want to propose a different SQL query witch doesn't involve appId:

"INSERT INTO moz_hosts_v9 (host, id, type, permission, expireType, expireTime, modificationTime, isInBrowserElement) SELECT DISTINCT host, id, type, permission, expireType, expireTime, modificationTime, isInBrowserElement FROM moz_hosts".

Note that SQLite doesn't support multiple DISTINCT columns and the 'distinct' column must be the first one in the SELECT query.

A follow up is to have a better downgrade approach where we always check that the current tables have all the correct columns.

What I'm concerned about is that when we are upgrading from an older (9-) version, we should throw away everything that has appid!=0, right? looking at [1], it seems we were ignoring appid values even before that patch (upgrade from 4 and 6) and happily added rows with appid!=0; as a result, aren't there now rows that originally were for apps only? Or am I missing something?

Changing the schema version externally is data tempering and not a valid use case. I still don't understand how the original problem happened under normal use.

[1] https://phabricator.services.mozilla.com/D29355#change-f3ZPA5AfEDJP

The STR was more to get a permissions.sqlite that seemed to match up with the current state of my permissions.sqlite. I'm assuming my permissions.sqlite through various nightly updates ended up at user_version=9 naturally as well as not having a moz_hosts table. As from comment 6, there does seem to be a moz_perms_v6 table that normally shouldn't be there, so I'm wondering if there was some failed schema upgrade that resulted in moz_perms_v6 and no moz_hosts ??

For anyone else running into this problem, can you check the tables structure and version? E.g.,

On a clean profile created from 66:

$ sqlite3 permissions.sqlite .schema
CREATE TABLE moz_perms ( id INTEGER PRIMARY KEY,origin TEXT,type TEXT,permission INTEGER,expireType INTEGER,expireTime INTEGER,modificationTime INTEGER);
CREATE TABLE moz_hosts ( id INTEGER PRIMARY KEY,host TEXT,type TEXT,permission INTEGER,expireType INTEGER,expireTime INTEGER,modificationTime INTEGER,appId INTEGER,isInBrowserElement INTEGER);

$ sqlite3 permissions.sqlite "pragma user_version"
9

On my regular nightly profile:

$ sqlite3 permissions.sqlite .schema
CREATE TABLE IF NOT EXISTS "moz_perms_v6" ( id INTEGER PRIMARY KEY,origin TEXT,type TEXT,permission INTEGER,expireType INTEGER,expireTime INTEGER,modificationTime INTEGER);
CREATE TABLE IF NOT EXISTS "moz_perms" ( id INTEGER PRIMARY KEY,origin TEXT,type TEXT,permission INTEGER,expireType INTEGER,expireTime INTEGER,modificationTime INTEGER);

$ sqlite3 permissions.sqlite "pragma user_version"
9

On a clean nightly profile:

$ sqlite3 permissions.sqlite .schema
CREATE TABLE moz_perms ( id INTEGER PRIMARY KEY,origin TEXT,type TEXT,permission INTEGER,expireType INTEGER,expireTime INTEGER,modificationTime INTEGER);
CREATE TABLE moz_hosts ( id INTEGER PRIMARY KEY,host TEXT,type TEXT,permission INTEGER,expireType INTEGER,expireTime INTEGER,modificationTime INTEGER,isInBrowserElement INTEGER);

$ sqlite3 permissions.sqlite "pragma user_version"
10

(In reply to Ed Lee :Mardak from comment #13)

The STR was more to get a permissions.sqlite that seemed to match up with the current state of my permissions.sqlite. I'm assuming my permissions.sqlite through various nightly updates ended up at user_version=9 naturally as well as not having a moz_hosts table. As from comment 6, there does seem to be a moz_perms_v6 table that normally shouldn't be there,

I have that one too, with 180 rows.

I think it's expected. can't see any code that would delete that. this test kinda surprises me then.

Then the expected moz_perms has 1000+ rows.
moz_hosts does exist in my db, but is empty.
version is 10. and I'm on Nightly not going back for many months.

so I'm wondering if there was some failed schema upgrade that resulted in moz_perms_v6 and no moz_hosts ??

could be!

still, I'm not persuaded (maybe just for lack of understanding) that the patch is the right thing to do.

From my regular Nightly profile:

$ sqlite3 permissions.sqlite .schema
CREATE TABLE moz_perms ( id INTEGER PRIMARY KEY,origin TEXT,type TEXT,permission INTEGER,expireType INTEGER,expireTime INTEGER,modificationTime INTEGER);
$ sqlite3 permissions.sqlite "pragma user_version"
9

From a newly created Nightly profile:

$ sqlite3 permissions.sqlite .schema
CREATE TABLE moz_perms ( id INTEGER PRIMARY KEY,origin TEXT,type TEXT,permission INTEGER,expireType INTEGER,expireTime INTEGER,modificationTime INTEGER);
CREATE TABLE moz_hosts ( id INTEGER PRIMARY KEY,host TEXT,type TEXT,permission INTEGER,expireType INTEGER,expireTime INTEGER,modificationTime INTEGER,isInBrowserElement INTEGER);
$ sqlite3 permissions.sqlite "pragma user_version"
10

Sorry for the bad STR earlier. Looks like the common thing is our existing permissions.sqlite version=9 is missing moz_hosts for some reason. I don't really know how we ended up there, but you can simulate that by dropping the table as in the attached permissions.sqlite

Using that permissions.sqlite in 65 shows an Allow for https://www.mozilla.org from about:preferences#privacy-sitedata -> Manage Permissions

Whereas using that file on nightly 68 shows no entries when opening Manage Permissions.

Ok.. so.. "good news". moz_hosts is actually not used and we can remove it completely. It was kept around to support downgrade from version 4. All the migration I have implemented can be removed completely. But we don't want to create inconsistency for nightly users, so I would suggest to keep version 10 around and remove migration 9->10.

Then, as follow up, we should simplify the permission db migrations. A good approach would be: if the current schema version is lower than 8 (4 years ago), we can drop all the entries and create the tables from scratch.

And I still don't understand how it fixes the bug and how actually the bug (that Slack prompts for permission on every browser start) happened. Here is an example of missing a dedicated field in bugzilla to describe the cause and fix of a defect.

Or was it just that a downgrade and then upgrade again crippled the database? And the fix just fixes the scenario that lead to the broken state, but doesn't fix this for users that already have the database in this bad shape?

And I still don't understand how it fixes the bug and how actually the bug (that Slack prompts for permission on every browser start) happened. Here is an example of missing a dedicated field in bugzilla to describe the cause and fix of a defect.

Here is what happens:

1. At startup, the permission manager checks the scheme version. It finds version 9 and it does the migration.

2. Because we already migrated from 9 to 10 (see the STR - comment 6), moz_hosts doesn't have appId column. The migration query fails and the initialization of the permission manager terminates here:
   https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/extensions/permissions/nsPermissionManager.cpp#1613

3. Because of that, we don't read the permissions:
   https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/extensions/permissions/nsPermissionManager.cpp#1688

4. At this point, we continue without preexisting permissions. Slack wants to notify the user and we show the notification prompt.

My patch removed migration 9->10 because moz_hosts table is there just for historical reasons (legacy from migration 4). It's not used, and it can be fully removed.

Or was it just that a downgrade and then upgrade again crippled the database? And the fix just fixes the scenario that lead to the broken state, but doesn't fix this for users that already have the database in this bad shape?

There are 3 types of users, I guess:

• with moz_hosts with appId column. All good. Migration 9-10 without this patch will remove appId column; with this patch it will keep appId column as it is.

• without moz_hosts. This can happen on profiles who migrated from version 5 to 6: https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/extensions/permissions/nsPermissionManager.cpp#1227-1231 - The database is already in a inconsistent state... migration 9-10, as it is, will fail. With this patch, the migration will succeeds.

• with moz_hosts without appId. For users who migrated from 9 to 10, then 10 to 9, then back 9 to 10. Even here, my patch would migrate correctly, because it ignore moz_hosts completely.

Thanks baku, now I understand!

Backed out for xpcshell failure on test_permmanager_migrate_9-10.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=superseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&selectedJob=246855511&revision=f8dfee6dc6e57a2a27978313a0409db058a08255

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=246859790&repo=autoland&lineNumber=2605

Backout: https://hg.mozilla.org/integration/autoland/rev/34287cc2fba86afb3147fb744e3e29095db249bf

https://hg.mozilla.org/mozilla-central/rev/3f0b0cb94a58