Closed Bug 1185343 Opened 5 years ago Closed 5 years ago

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


(Core :: Permission Manager, defect, critical)

42 Branch
Not set



Tracking Status
firefox41 --- unaffected
firefox42 + verified


(Reporter: alice0775, Assigned: Nika)



(Keywords: dataloss, regression)


(2 files, 2 obsolete files)

[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:
Keywords: dataloss
Severity: normal → major
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)
Flags: needinfo?(michael)
Assignee: nobody → michael
Duplicate of this bug: 1185613
I added a test for the migration
Attachment #8636598 - Flags: review?(ehsan)
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 {
} finally {
Attachment #8636598 - Flags: review?(ehsan) → review+
(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.
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 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+
I wouldn't mind looking at another version of part 1 that addresses my review comments (mostly want to double check the code comments.)
(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 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.
Addresses the comment changes requested in the review
Attachment #8636279 - Attachment is obsolete: true
Attachment #8636659 - Flags: review?(ehsan)
Attachment #8636659 - Flags: review?(ehsan) → review+
Closed: 5 years ago
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!
(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.
Flags: needinfo?(alice0775)
Depends on: 1193200
Depends on: 1260945
No longer depends on: 1260945
Depends on: 1279285
See Also: → 1557153
You need to log in before you can comment on or make changes to this bug.