+++ 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=no@test.invalid New: chrome://messenger/content/email=no@test.invalid We need to write a migration from old to new format, à la: update moz_perms set origin= ... where origin like "chrome://messenger/content/?"

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

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

> 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 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 on attachment 8827283 [details] [diff] [review] 1331477-migrate-perm.patch LGTM r/a=me

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

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

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.

You never addressed comment #17.

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

And thanks :)

Comment on attachment 8827283 [details] [diff] [review] 1331477-migrate-perm.patch r/a=me

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

https://hg.mozilla.org/releases/comm-beta/rev/9a6f4dd62f09f2fdda71bf99435836990bb7a148 Stay tuned for patch #2

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 on attachment 8838931 [details] [diff] [review] 1331477-part2-dbclose.patch r/a=me

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

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

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

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