1331477 - Migrate remote content exceptions in SeaMonkey after bug 1329494: Remove ? from database in chrome URIs

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Frank-Rainer Grahl (:frg)]

+++ This bug was initially created as a clone of Bug #1330640 +++

+++ This bug was initially created as a clone of Bug #1329494 +++

In bug 1329494 we removed the "?" from the encoding of e-mail addresses as chrome URI for storage in the permission manager.

Old: chrome://messenger/content/?email=[e-mail]
New: chrome://messenger/content/email=[e-mail]

We need to write a migration from old to new format, à la:
  update moz_perms set origin= ... where origin like "chrome://messenger/content/?"

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1331477-migrate-perm.patch

More or less a 1:1 copy of Jorgs patch. Not setting review yet in case something else pops up.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8827279 [details] [diff] [review]
1331477-migrate-perm.patch

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

::: suite/common/src/nsSuiteGlue.js
@@ +374,5 @@
>                            .createInstance(Components.interfaces.nsITimer);
>      timer.init(this, 3000, timer.TYPE_ONE_SHOT);
>    },
>  
> +  _migrateUI: function() 

Trailing space.

@@ +400,5 @@
> +
> +      let statement = db.createStatement(
> +        "select origin, permission from moz_perms where " +
> +        // Avoid 'like' here which needs to be escaped.
> +        "  substr(origin, 1, 28) = 'chrome://messenger/content/?';");

Why the space before the 'substr'?

Comment 3

[Frank-Rainer Grahl (:frg)]

Attachment: 1331477-migrate-perm.patch

> Trailing space.

Thanks fixed.

> Why the space before the 'substr'?

Just personal preference. And if you forget the blank at the end of the previous line the statement will still work. I usally omit blanks at the end.

I my previous static sql host programmer life I lined them up like:

> delete 
>   from moz_perms 
>  where substr(origin, 1, 28) = 'chrome://messenger/content/?';")
>    and blabla bla  ;

Was/is easier to read for me.

Comment 4

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8827283 [details] [diff] [review]
1331477-migrate-perm.patch

Jorg and I fiddled around with this a lot and I doubt a better solution will fly in any time soon. So lets just set review.

Comment 5

[Ian Neal]

Comment on attachment 8827283 [details] [diff] [review]
1331477-migrate-perm.patch

LGTM r/a=me

Comment 6

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/c789fe71e02c0b1396da5e8c5c1107a1598dfad4

Comment 7

[Jorg K (CEST = GMT+2)]

You've landed this too soon. What you landed has two issues, one was picked up by Magnus, another one by me:

https://hg.mozilla.org/comm-central/rev/c789fe71e02c0b1396da5e8c5c1107a1598dfad4#l1.70
You don't need the catch (ex) { throw ex; }.

If that throws, the database won't get closed since the db.close() sits in the finally {} of the other try.

Sorry 'bout that ;-(

Comment 8

[Jorg K (CEST = GMT+2)]

Check this:
https://hg.mozilla.org/comm-central/rev/543431e0ab6980233762d197542fee80b031ee01

Comment 9

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8827283 [details] [diff] [review]
1331477-migrate-perm.patch

[Approval Request Comment]
Regression caused by (bug #): Bug 1330640, Bug 1337071
User impact if declined:  managing older whitelisted mail adresses is not possible.
Testing completed (on m-c, etc.): c-c c-a
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: none.

Comment 10

[Jorg K (CEST = GMT+2)]

You never addressed comment #17.

Comment 11

[Frank-Rainer Grahl (:frg)]

Sorry missed your comments. Will fix it in a small followup patch for all branches.

Comment 12

[Frank-Rainer Grahl (:frg)]

And thanks :)

Comment 13

[Ian Neal]

Comment on attachment 8827283 [details] [diff] [review]
1331477-migrate-perm.patch

r/a=me

Comment 14

[Frank-Rainer Grahl (:frg)]

Reopened to add an additional patch to take care of the comments.

Comment 15

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-beta/rev/9a6f4dd62f09f2fdda71bf99435836990bb7a148

Stay tuned for patch #2

Comment 16

[Frank-Rainer Grahl (:frg)]

Attachment: 1331477-part2-dbclose.patch

Part 2. Fixes the issues Jorg mentioned.

[Approval Request Comment]
User impact if declined: After a migration error the permissions db might not be closed.
Testing completed (on m-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): One time only migration and simple patch. No risk.
String changes made by this patch: none.

Comment 17

[Ian Neal]

Comment on attachment 8838931 [details] [diff] [review]
1331477-part2-dbclose.patch

r/a=me

Comment 18

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/836c4adc56091868c7d52b34f772960e120e47b9
https://hg.mozilla.org/releases/comm-aurora/rev/8d99754a516111bfedf34a54ce66624ef1083494
https://hg.mozilla.org/releases/comm-beta/rev/1c210d14fb7c105502e19f81d972999bfe6387bc

Comment 19

[rsx11m]

This needs to land on comm-release as well for 2.48, also to make bug 1366496 attachment 8869825 [details] [diff] [review] work.

Comment 20

[rsx11m]

Fails with an existing 2.48b1 profile on 2.48b1 with both patches applied, some missed exception?

Comment 21

[rsx11m]

Ok, so bug 1329494 and bug 1330640 landed on 52.0+ only, so this shouldn't apply for 51.0 = SM 2.48. Sorry for the noise...