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

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

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

Magnus, can I assign this to you?

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: WIP: 1330640-migrate-chrome-URIs.patch

Something like this should do it, but strangely the mail migrator doesn't get triggered at all, hmm.

Comment 2

[Frank-Rainer Grahl (:frg)]

Would it make sense to put the actual migration function into mailnews so that it can be called by SeaMonkey too?

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[Jorg K (CEST = GMT+2)]

(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

[Frank-Rainer Grahl (:frg)]

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

Comment 6

[Jorg K (CEST = GMT+2)]

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

[Frank-Rainer Grahl (:frg)]

> 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

[Frank-Rainer Grahl (:frg)]

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=[e-mail]");
>  Services.perms.remove(nURI, "image");

Comment 9

[Jorg K (CEST = GMT+2)]

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

Comment 10

[Jorg K (CEST = GMT+2)]

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

Comment 11

[Frank-Rainer Grahl (:frg)]

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

Comment 12

[Jorg K (CEST = GMT+2)]

(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

[Frank-Rainer Grahl (:frg)]

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

[Magnus Melin [:mkmelin]]

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.

Comment 15

[Jorg K (CEST = GMT+2)]

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

Comment 16

[Jorg K (CEST = GMT+2)]

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

Comment 17

[Philip Chee]

> 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

[Philip Chee]

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

[Magnus Melin [:mkmelin]]

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)

Comment 20

[Jorg K (CEST = GMT+2)]

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

Comment 21

[Jorg K (CEST = GMT+2)]

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

Comment 22

[Jorg K (CEST = GMT+2)]

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

Comment 23

[Magnus Melin [:mkmelin]]

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.

Comment 24

[Jorg K (CEST = GMT+2)]

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.

Comment 25

[Jorg K (CEST = GMT+2)]

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.