Closed Bug 1331477 Opened 3 years ago Closed 3 years ago

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

Categories

(SeaMonkey :: Passwords & Permissions, defect)

SeaMonkey 2.50 Branch
defect
Not set

Tracking

(seamonkey2.49esr fixed, seamonkey2.50 fixed, seamonkey2.51 fixed)

RESOLVED FIXED
seamonkey2.51
Tracking Status
seamonkey2.49esr --- fixed
seamonkey2.50 --- fixed
seamonkey2.51 --- fixed

People

(Reporter: frg, Assigned: frg)

References

Details

Attachments

(2 files, 1 obsolete file)

+++ 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/?"
Attached patch 1331477-migrate-perm.patch (obsolete) — Splinter Review
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.
Attachment #8827279 - Attachment is obsolete: true
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.
Attachment #8827283 - Flags: review?(iann_bugzilla)
Comment on attachment 8827283 [details] [diff] [review]
1331477-migrate-perm.patch

LGTM r/a=me
Attachment #8827283 - Flags: review?(iann_bugzilla) → review+
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
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 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.
Attachment #8827283 - Flags: approval-comm-beta?
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
Attachment #8827283 - Flags: approval-comm-beta? → approval-comm-beta+
Reopened to add an additional patch to take care of the comments.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8838931 - Flags: review?(iann_bugzilla)
Attachment #8838931 - Flags: approval-comm-beta?
Attachment #8838931 - Flags: approval-comm-aurora?
Comment on attachment 8838931 [details] [diff] [review]
1331477-part2-dbclose.patch

r/a=me
Attachment #8838931 - Flags: review?(iann_bugzilla)
Attachment #8838931 - Flags: review+
Attachment #8838931 - Flags: approval-comm-beta?
Attachment #8838931 - Flags: approval-comm-beta+
Attachment #8838931 - Flags: approval-comm-aurora?
Attachment #8838931 - Flags: approval-comm-aurora+
This needs to land on comm-release as well for 2.48, also to make bug 1366496 attachment 8869825 [details] [diff] [review] work.
Flags: needinfo?(frgrahl)
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...
Flags: needinfo?(frgrahl)
You need to log in before you can comment on or make changes to this bug.