Permissions are lost when Nightly42.0a1-> Aurora41.0a2 and Nightly42.0a1-> Aurora41.0a2 ->Nightly42.0a1

VERIFIED FIXED in Firefox 42

Status

()

Core
Permission Manager
--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Alice0775 White, Assigned: Nika)

Tracking

({dataloss, regression})

42 Branch
mozilla42
dataloss, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 unaffected, firefox42+ verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
[Tracking Requested - why for this release]:

There is a severe problem for users who care about the security


Steps To Reproduce:
1. Set Permissions on Nightly42.0a1
2. Start Aurora41.0a2 with the same profile
   ---- Permissions are lost  --- Bug
3. Start Nightly42.0a1 with the same profile
   ---- Permissions are lost  --- Bug


Actual Results:
when Downgrade or when Downgrade then Upgrade
Permissions are lost without any user feedback

Expected Results:
Permissions should be persisted.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=443582420f2c&tochange=fb346b9b9f98
(Reporter)

Updated

3 years ago
Keywords: dataloss
(Reporter)

Updated

3 years ago
Severity: normal → major
(Assignee)

Comment 1

3 years ago
Created attachment 8636279 [details] [diff] [review]
Use new table moz_perms for permissions, and leave unmigrated permission data in moz_hosts for back-compat

This new patch moves the new table schema into a new table (moz_perms) and restores the previous permissions data into the moz_hosts table. 

This means that permissions when you upgrade to 42 will be migrated into moz_perms, and from that point forth permissions in 42 and permissions in older versions of firefox will be treated seperately, permissions changes made in 41 will not be added to the 42 permissions database and vice-versa.

Once this patch is applied, users who move from the new nightly to the current nightly (after the migration, but before this patch) will experience data loss, but other movements between versions will not experience data loss.
Attachment #8636279 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Flags: needinfo?(michael)
(Assignee)

Updated

3 years ago
Assignee: nobody → michael
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1185613
(Assignee)

Comment 4

3 years ago
Created attachment 8636598 [details] [diff] [review]
Part 2: Test for migration from schema 5->6 for permission manager

I added a test for the migration
Attachment #8636598 - Flags: review?(ehsan)

Comment 5

3 years ago
Comment on attachment 8636598 [details] [diff] [review]
Part 2: Test for migration from schema 5->6 for permission manager

Review of attachment 8636598 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/cookie/test/unit/test_permmanager_migrate_5-6.js
@@ +61,5 @@
> +    ")");
> +
> +  let id = 0;
> +
> +  // Insert an origin into the database, and return it's principal, type, and permission values

Nit: its

@@ +79,5 @@
> +      stmtInsert.reset();
> +    } catch (e) {
> +      stmtInsert.reset();
> +      throw e;
> +    }

Nit: instead of this, do:

try {
  stmtInsert.executeStep();
} finally {
  stmtInsert.reset();
}
Attachment #8636598 - Flags: review?(ehsan) → review+
(Assignee)

Comment 6

3 years ago
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #5)
> try {
>   stmtInsert.executeStep();
> } finally {
>   stmtInsert.reset();
> }

I have no idea why I wrote that code, there's a method called `execute` which does exactly this without the need for the exception handling >.>. I'm switching to using that.
(Assignee)

Comment 7

3 years ago
Created attachment 8636621 [details] [diff] [review]
Part 2: Test for migration from schema 5->6 for permission manager
Attachment #8636598 - Attachment is obsolete: true

Comment 8

3 years ago
Comment on attachment 8636279 [details] [diff] [review]
Use new table moz_perms for permissions, and leave unmigrated permission data in moz_hosts for back-compat

Review of attachment 8636279 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/cookie/nsPermissionManager.cpp
@@ +943,5 @@
>        }
>  
> +      // fall through to the next upgrade
> +
> +    // Version 5->6 is the renaming of moz_hosts to moz_perms, and moz_hosts_v4 to moz_hosts

I think this comment needs to change in order to explain how we're dealing with downgrading to v3, and also downgrading to v3, upgrading to v5, downgrading back to v3 and then back to v5.  It would also be good if you explained the expected dataloss scenarios, and also the quirk about upgrading to v4 in the few days that that version was the latest on Nightly.

@@ +955,5 @@
> +          NS_ENSURE_SUCCESS(rv, rv);
> +        } else {
> +          NS_WARNING("moz_hosts was not renamed to moz_perms, as a moz_perms table already exists");
> +
> +          rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("DROP TABLE moz_hosts"));

Please explain why you are dropping the old table (could be condensed in the above comment.)

@@ +959,5 @@
> +          rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("DROP TABLE moz_hosts"));
> +          NS_ENSURE_SUCCESS(rv, rv);
> +        }
> +
> +        // Rename moz_hosts_v4 back to it's original location, if it exists

Please add an #ifdef DEBUG chunk here that would MOZ_ASSERT that moz_hosts does not exist (mostly for clarifying what goes on here.)

@@ -997,5 @@
>    if (NS_FAILED(rv)) return rv;
>  
>    // create the table
> -  // SQL also lives in automation.py.in. If you change this SQL change that
> -  // one too.

Why are you removing this comment?  Please put it back :-)
Attachment #8636279 - Flags: review?(ehsan) → review+

Comment 9

3 years ago
I wouldn't mind looking at another version of part 1 that addresses my review comments (mostly want to double check the code comments.)
(Assignee)

Comment 10

3 years ago
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #8)
> Why are you removing this comment?  Please put it back :-)

I removed it in passing because automation.py.in doesn't actually contain this SQL anymore. At some point in the past month or so, it was removed. The other location the SQL exists now is in mozprofile (which I should probably mention instead), but I agree that modifying this comment is probably outside the scope of this patch.
(Assignee)

Comment 11

3 years ago
Created attachment 8636659 [details] [diff] [review]
Part 1: Use new table moz_perms for permissions, and leave unmigrated permission data in moz_hosts for back-compat

Addresses the comment changes requested in the review
Attachment #8636279 - Attachment is obsolete: true
Attachment #8636659 - Flags: review?(ehsan)

Updated

3 years ago
Attachment #8636659 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/d4cc20fc3973
https://hg.mozilla.org/mozilla-central/rev/973a648bd7fe
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
No need to track this as it has already been fixed in FF42. Please re-nominate for tracking if this bug is reopened.

Alice, can you please verify that the fix works on 2015-07-22 Nightly build or later? Thanks!
tracking-firefox42: ? → ---

Updated

3 years ago
Flags: needinfo?(alice0775)

Comment 15

3 years ago
(In reply to Ritu Kothari (:ritu) from comment #14)
> No need to track this as it has already been fixed in FF42. Please
> re-nominate for tracking if this bug is reopened.

Regressions do need to be tracked even if they are fixed, in case they get backed out in the future to ensure that they don't fall into the cracks.
tracking-firefox42: --- → ?
Tracked.
tracking-firefox42: ? → +
(Reporter)

Updated

3 years ago
Status: RESOLVED → VERIFIED
Flags: needinfo?(alice0775)

Updated

3 years ago
status-firefox42: fixed → verified
(Reporter)

Updated

3 years ago
Depends on: 1193200

Updated

3 years ago
Depends on: 1211591
(Reporter)

Updated

2 years ago
Depends on: 1260945
(Reporter)

Updated

2 years ago
No longer depends on: 1260945

Updated

2 years ago
Depends on: 1279285
You need to log in before you can comment on or make changes to this bug.