Slack prompts for permission to use notifications at every browser startup

RESOLVED FIXED in Firefox 68

Status

()

defect
P1
normal
RESOLVED FIXED
Last month
Last month

People

(Reporter: heycam, Assigned: baku)

Tracking

(Regression, {regression})

unspecified
mozilla68
Points:
---

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 unaffected, firefox67 unaffected, firefox68+ fixed)

Details

Attachments

(2 attachments)

Reporter

Description

Last month

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

Flags: needinfo?(amarchesini)
Reporter

Comment 1

Last month

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

Reporter

Comment 2

Last month

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

Updated

Last month
See Also: → 1550004
Duplicate of this bug: 1550004
Component: Security: CAPS → Permission Manager
Priority: -- → P1

Updated

Last month
Assignee: nobody → amarchesini

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

Assignee

Comment 5

Last month

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.

Flags: needinfo?(amarchesini) → needinfo?(edilee)

Comment 6

Last month

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
Flags: needinfo?(edilee)

Comment 7

Last month

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

Assignee

Comment 8

Last month

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

baku, can you please explain the patch? thanks.

Flags: needinfo?(amarchesini)
Assignee

Comment 11

Last month

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.

Flags: needinfo?(amarchesini)

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

Comment 13

Last month

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

Comment 14

Last month

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
Flags: needinfo?(cam)

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

Reporter

Comment 16

Last month

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
Flags: needinfo?(cam)

Comment 17

Last month

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.

Assignee

Comment 18

Last month

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.

Flags: needinfo?(amarchesini)

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?

Assignee

Comment 21

Last month

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.

Flags: needinfo?(amarchesini)

Thanks baku, now I understand!

Comment 23

Last month
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8dfee6dc6e5
Support downgrade versioning of permissions database, r=mayhemer

Comment 24

Last month
Backout by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/34287cc2fba8
Backed out changeset f8dfee6dc6e5 for xpcshell failure on test_permmanager_migrate_9-10.js CLOSE TREE.
Assignee

Updated

Last month
Flags: needinfo?(amarchesini)

Comment 26

Last month
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f0b0cb94a58
Support downgrade versioning of permissions database, r=mayhemer
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.