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

RESOLVED FIXED in Thunderbird 53.0

Status

Thunderbird
General
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 53.0

Thunderbird Tracking Flags

(thunderbird52 fixed, thunderbird53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 months ago
+++ 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/?"

Magnus, can I assign this to you?
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 1

4 months ago
Created attachment 8827112 [details] [diff] [review]
WIP: 1330640-migrate-chrome-URIs.patch

Something like this should do it, but strangely the mail migrator doesn't get triggered at all, hmm.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)

Comment 2

4 months ago
Would it make sense to put the actual migration function into mailnews so that it can be called by SeaMonkey too?
(Assignee)

Comment 3

4 months ago
Turns out that a |mozilla/mach build| was required and not just a |mozilla/mach build mail/base|. Debug then shows:
=== migration 13
=== migration 1
=== migration [xpconnect wrapped nsILocalFile @ 0xc567140 (native @ 0xc53dd00)]
=== migration 4
=== success

However, the problem is that at the time the migration is run, the permission manager has already read the database. So the table gets updated, but the exceptions still show the old faulty content. Next time TB is started, it looks good.

I'll need to see whether I can get the permission manager to drop its data and read it again from the table.
(Assignee)

Comment 4

4 months ago
(In reply to Frank-Rainer Grahl from comment #2)
> Would it make sense to put the actual migration function into mailnews so
> that it can be called by SeaMonkey too?
I don't know. Does SM have a migrator like mail? Anyway, I need to get it to work first.

Comment 5

4 months ago
> Does SM have a migrator like mail? 

Need to check. Would probably call the function during startup.

> Anyway, I need to get it to work first.

> I'll need to see whether I can get the permission manager to drop its data and read it again from the table.

Maybe you can read them using sql and use the permisssions api for each row to delete the old and insert the new ones? Only one time so hopefully it won't take too long.
(Assignee)

Comment 6

4 months ago
Sadly the permissions API doesn't work since the origins are truncated at the "?". Yes, I can insert new ones, but I can't delete the old ones. A simple wait out would be:
Thunderbird has updated an internal database and needs to restart now. [OK] ;-)

Comment 7

4 months ago
> Sadly the permissions API doesn't work since the origins are truncated at the "?".

Did you try it with a delete? I worked on the SeaMonkey Data Manager last year and was able to put some real garbage into the DB using the official apis. They only did a limited amount of error checking...

Comment 8

4 months ago
Did a static test which worked for me:

>  var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>                     .getService(Components.interfaces.nsIIOService);
>  var nURI = ioService.newURI("chrome://messenger/content/?email=testl@gmx.net");
>  Services.perms.remove(nURI, "image");
(Assignee)

Comment 9

4 months ago
Yes, that seems to work, so I'll do it your way. Stand by ;-)
(Assignee)

Comment 10

4 months ago
Created attachment 8827210 [details] [diff] [review]
1330640-migrate-chrome-URIs.patch (v2)

OK, let's do things the FRG way. I liked my update statement, but it didn't fit into the scheme of things.

This works. I left a dump() in the code in case the reviewer wants to try it.
Attachment #8827112 - Attachment is obsolete: true
Attachment #8827210 - Flags: review?(mkmelin+mozilla)

Comment 11

4 months ago
> // Sadly we still need to clear the database manually. Experiments
> // showed that the permissions manager deleted only one record.

For the record: I thought it might be some sort of sharing problem with the db and tried to delete /add the permissions in a separate loop from an array and it still didn't work. Definitly looks like a bug in the permissions manager api maybe because of the unsupported uri.
(Assignee)

Comment 12

4 months ago
(In reply to Frank-Rainer Grahl from comment #11)
> For the record: I thought it might be some sort of sharing problem with the
> db and tried to delete /add the permissions in a separate loop from an array
> and it still didn't work.
Me too. I didn't publish the patch, but here is the code:

        let permissions = [];
        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/?';");
        try {
          while (statement.executeStep()) {
            permissions.push([statement.getUTF8String(0),
                             statement.getInt32(1)]);
          }
        } catch (ex) {
          throw ex;
        } finally {
          statement.finalize();
          db.close();
        }

        for (let p of permissions) {
          let origin = p[0];
          dump("=== processing "+origin+"\n");
          Services.perms.remove(
            Services.io.newURI(origin), "image");
          origin = origin.replace("chrome://messenger/content/?",
                                  "chrome://messenger/content/");
          Services.perms.add(
            Services.io.newURI(origin), "image", p[1]);
        }
      }

Comment 13

4 months ago
>> Me too. I didn't publish the patch, but here is the code:

Almost exactly what I did. I also removed a dummy permission after the first one and put remove / add in different loops. I think the permission manager caches the permissions and thinks it has been removed already. Probably only uses 1 copy internally for all the ? entries.

Comment 14

4 months ago
Comment on attachment 8827210 [details] [diff] [review]
1330640-migrate-chrome-URIs.patch (v2)

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

::: mail/base/modules/mailMigrator.js
@@ +337,5 @@
> +            let origin = statement.getUTF8String(0);
> +            let permission = statement.getInt32(1);
> +            dump("=== retrieved "+origin+"\n");
> +            Services.perms.remove(
> +              Services.io.newURI(origin), "image");

I think this is wrong, origins are not really urls (but look similar) so I'm not confident you will continue to get the right thing.

But shouldn't the only sql statement just be something like
UPDATE moz_perms SET origin = replace(origin, 'chrome://messenger/content/?', 'chrome://messenger/content/' );

You should be able to get the permissions refreshed using
Services.perms.refreshPermission() 

 ... after you're done.
Attachment #8827210 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 15

4 months ago
Comment on attachment 8827210 [details] [diff] [review]
1330640-migrate-chrome-URIs.patch (v2)

Sorry, Magnus, I spent one entire day wrestling with this, and this is the only solution that works, unless you want to quit the session.

You can see the "update solution" in attachment 8827112 [details] [diff] [review], but there is no way to convince the permissions manager to refresh its data.

Services.perms.refreshPermission() does not do what the name suggests. I tried it and it returned an error here:
https://dxr.mozilla.org/comm-central/rev/96cb95af530477edb66ae48d98c18533476e57bb/mozilla/extensions/cookie/nsPermissionManager.cpp#823

FRG has also extensively experimented with this and he agrees with my findings. Anyway, if you have the time, you can play with it. I'll shout you a beer if you find a better solution.

As for the:
> I think this is wrong, origins are not really urls (but look similar)
> so I'm not confident you will continue to get the right thing.
Please check how you store them in the first place, exactly like this:
https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#3202
Attachment #8827210 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 16

4 months ago
BTW, Ian Neal just approved the same thing in bug 1331477.

Comment 17

4 months ago
> https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#3202
Permalink:
https://dxr.mozilla.org/comm-central/rev/9cb95ca4c3f7cbd473e78addb8e3607af6af8386/mail/base/content/mailWindowOverlay.js#3202

Comment 18

4 months ago
This worries me a bit:
https://dxr.mozilla.org/comm-central/rev/96cb95af530477edb66ae48d98c18533476e57bb/mozilla/extensions/cookie/nsPermissionManager.cpp#1926

// Remove from memory and notify immediately. Since the in-memory
// database is authoritative, we do not need confirmation from the
// on-disk database to notify observers.
RemoveAllFromMemory();

Comment 19

4 months ago
Comment on attachment 8827210 [details] [diff] [review]
1330640-migrate-chrome-URIs.patch (v2)

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

I guess we can do this, though I'm torn. Just going with the first version of the patch is cleaner.

::: mail/base/modules/mailMigrator.js
@@ +344,5 @@
> +            Services.perms.add(
> +              Services.io.newURI(origin), "image", permission);
> +          }
> +        } catch (ex) {
> +          throw ex;

you can just ommit these two lines (no need for a catch that will throw it on)
Attachment #8827210 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 20

4 months ago
Created attachment 8829278 [details] [diff] [review]
1330640-migrate-chrome-URIs.patch (v2b).

Removed the dump() the superfluous catch(). Also fixed the error handling, since the database didn't get closed in all cases.

(In reply to Magnus Melin from comment #19)
> I guess we can do this, though I'm torn. Just going with the first version
> of the patch is cleaner.
Another cryptic Magnus comment. "Torn" meaning what? Yes, the first solution is nicer, one update statement, but sadly it doesn't work since the permissions manager can't be convinced to drop its internal data and read the database again.
Attachment #8827210 - Attachment is obsolete: true
Attachment #8829278 - Flags: review+
(Assignee)

Comment 21

4 months ago
Oops, forgot to refresh the patch, so the dump() is still there. I won't land it though.
(Assignee)

Comment 22

4 months ago
https://hg.mozilla.org/comm-central/rev/543431e0ab6980233762d197542fee80b031ee01
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0

Comment 23

4 months ago
Well "doesn't work" is relative. Not working on the first session is not the end of the world, it's not a critical feature after all. So I'm fine with v2, but there's a chance a hack like this will come back to haunt us.
(Assignee)

Comment 24

4 months ago
Oh, speaking of hacks ... ;-) Who invented that chrome URI hack in the first place, oh sorry, according to bug 1329494 comment #21 it's a carefully constructed chrome URI.

When we ship TB 59, the next ESR after TB 52, which will upgrade people, we need to check that the hack still works.
(Assignee)

Comment 25

3 months ago
Beta (TB 52):
https://hg.mozilla.org/releases/comm-beta/rev/1f512d34bee25b6975d9207b834631a18a0494c5
This needed to be uplifted due to a merge conflict with bug 1337071.
status-thunderbird52: --- → fixed
status-thunderbird53: --- → fixed
You need to log in before you can comment on or make changes to this bug.