Closed Bug 1575512 Opened 5 years ago Closed 5 years ago

Default IMAP folder names not localised when language pack is used [involving permissions.sqlite (Fix: delete from moz_perms where type = 'storageAccessAPI' and origin like 'imap%')]

Categories

(Thunderbird :: Folder and Message Lists, defect)

Product:
Thunderbird
Component:
Folder and Message Lists
Platform:
Unspecified
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr6870+ fixed, thunderbird71 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 71.0
Tracking Flags:
Tracking Status
thunderbird_esr68 70+ fixed
thunderbird71 --- fixed

People

(Reporter: de.berberich, Assigned: jorgk-bmo)

References

Details

(Keywords: intl, regression, regressionwindow-wanted)

Attachments

(5 files, 29 obsolete files)

permissions.sqlite
96.00 KB, application/octet-stream
Details
folder-debug-mark2.patch
1.91 KB, patch
Details | Diff | Splinter Review
1575512-glue-mark2.patch (v1c).
16.47 KB, patch
jorgk-bmo
: review+
jorgk-bmo
: approval-comm-esr68+
Details | Diff | Splinter Review
1575512-follow-up.patch
2.57 KB, patch
jorgk-bmo
: approval-comm-beta+
jorgk-bmo
: approval-comm-esr68+
Details | Diff | Splinter Review
1575512-follow-up-trunk.patch
2.64 KB, patch
aceman
: review+
Details | Diff | Splinter Review

Steps to reproduce:

Download and install French locale of Thunderbird 68.0-candidates/build4.
Install German de.xpi (and/or American-English en-US.xpi or Italian, ...etc) language packs.
Open the config editor, create a new string preference, name it intl.locale.requested and enter the value "de" (or "en-US" or "it", ...etc) without quotation marks.
Restart Thunderbird.

Actual results :

The Thunderbird GUI now is in German (or English or Italian) excepting the names of the default folders in Folder Pane.

Expected results:

Folder names should be translated, too.

Blocks: tb68found

But the localised versions work, right? Just not the changes with the language pack?

Everything in the localized versions works correctly, tool bars and menus in Main window, address book and compose windows.
It's just the folder names in the folder pane which resist to translation and keep the original French names of my (French) Thunderbird version when I switch from French to German or English or Italian.....

What happens with en-US TB plus language pack? Do the folders remain in English? (I can try that myself when I find a moment.)

I did further tests with the en-US and German versions of TB 68.0 in new separate profiles, installing two or three other language packs.
I could not reproduce the issue. Each time I switched to another language and restarted TB the folder names would be translated correctly.
Seems to be a problem of the French TB 68.0 version.
I'll continue testing the French version in a new profile without installing any add-ons.

Thanks for spending time with this issue. So localised to German + langpack also works. That's good to know.

I continued testing and trying to reproduce this issue. First I recreated a new identical profile for the French version with the de.xpi and en-US.xpi language packs. After hours of testing I suddenly had the Folder Pane in French after switching to German (de) in intl.locale.requested but I still couldn't identify what triggered the error.
Then I re-tested the en-US version of Thunderbird with the fr.xpi and de.xpi language packs in a new profile. Eventually with this version I succeeded to provoke the same error : the Folder Pane staying in US-English after switching to German (de) which I documented in an new screen shot. But as in the other profiles I was unable to identify the factor which triggers this error.

I've been working for years with language packs but I've never before encountered this issue.

Attached file TB 69.0b4 fr + de.xpi.png (obsolete) (deleted) —
The content of attachment 9087471 [details] has been deleted for the following reason: Requested by attachment author Eckard Berberich
Attached image Test TB 69.0b4 fr + de.xpi.png (obsolete) —

Oddly enough today again while testing Thunderbird 69.0b4-candidates/build1/ French version with German language pack de.xpi I could reproduce the error (Folder Pane being fixed in French) but I still can't find a common trigger for this issue (see screen shot).

And here is one more example of the same issue, again in a newly created profile for Thunderbird 68.0 fr in which I have installed

• the de.xpi and en-US.xpi language packs;
• the English United States Dictionary 60.1webext by Jorg K and German Dictionary 2.0.6.1webext by KaiRo (no French dictionary since it is included in French TB versions);
• The add-ons Lightning 68.0b6, Phoenity Buttons 3.1, Provider for CalDAV & CardDAV 1.2, TbSync [Beta Release Channel] 2.3.

This time I could observe that the "freeze" of the folder names in French happened after I had changed the language in Preferences (Options) > Composition > Spelling > Language. I now remember that in my second case, too, the selection of the spelling language (at that moment no language had been selected) had preceded the "freeze" of the folder names in French. But this cannot be the only condition, there must be other factors that favor the freeze. Since once the folder names are frozen there is no way to display them in English or German by fiddling the intl.locale.requested value in the config editor or changing the spelling language.

I still have another TB 68.0 fr test profile with the same configuration and the same add-ons in which I can't provoke the error.

Attached image TB 68.0 fr + de.xpi.png (obsolete) —

Eventually I could solve this issue after looking up the TB 68 release notes https://www.thunderbird.net/en-US/thunderbird/68.0/releasenotes/
Under "What's New" I found
"Language packs can now be selected in the Advanced Options. Preference intl.multilingual.enabled needs to be set (and possily also extensions.langpacks.signatures.required needs to be set to false)"

Just like in Firefox 68 the UI language can now be changed in TB 68 via Preferences (Options) > Advanced > General > Language ....
To display the "Language" item in "General" you first have to toggle the value of the preferences (options) "intl.multilingual.enabled" and "intl.multilingual.downloadEnabled" from "false" to "true" and restart TB.

Language packs must then be downloaded from http://ftp.mozilla.org/pub/thunderbird/releases/68.0/ and installed manually since the feature "Select a language to add..." in "Thunderbird Language Settings" actually doesn't display any languages.

Following these instructions in a new pristine profile I could not reproduce the issue described in comments #1 to #13.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
Attached image TB 68 - 5th profile.png (obsolete) —

Unfortunately my joy with the new profile created three days ago as described in comment #14 didn't last.
Today after changing the UI language in Preferences (Options) > Advanced > Language and restarting TB the folders in the Folder Pane again stayed in French and wouldn't be translated to the new UI language. I have no idea how this issue is triggered.

Status: RESOLVED → REOPENED
Resolution: INVALID → ---

Update:
I think that I've found a possible culprit for this issue of the non-localized folder names when using language packs (German de.xpi and US-English en-us.xpi in the French Thunderbird version in my case).
Yesterday again in my actual TB 68 test profile, after switching the GUI language to German or US-English and restarting TB, the folder names would stay in French instead of being localized in German or English.

Since I backup all my TB profiles every day, I systematically replaced one file after the other in my actual test profile with the equivalent file from the latest backup of this profile and restarted TB. I began with the prefs.js file, my usual suspect, but no joy. Finally I came to replace the permissions.sqlite file and ...... bingo! On restart the folder names would be localized correctly again in the GUI language I had chosen before!!

I'm no programmer and don't understand which possible role a file such as permissions.sqlite would play as a trigger for the non-localization of the folder names. But for the meantime I have at least a workaround for this issue: replace the permissions.sqlite file or simply delete it.
If someone is interested and would like to take an insight: I am keeping two of those "corrupt" permissions.sqlite files at your disposal. And I'm sure there will be more of these corrupt files in the future.

See Also: → 1579747

I'm having the same issue while for me I never saw the folder names correctly translated. So I also cannot tell if restoring some permissions.sqlite is helping my case.
All language is totally fine besides the default folder names. Talking here about en-US builds with installed langpacks. In my case tested with DE. Also checked that the langpack contains
./chrome/de/locale/de/messenger/messenger.properties:inboxFolderName=Posteingang

No matter what I set as
intl.multilingual.enabled
and
intl.locale.requested

I can confirm that renaming/removing of permissions.sqlite solves the issue, i.e., the folder names are localized correctly.

(In reply to FK from comment #19)

I can confirm that renaming/removing of permissions.sqlite solves the issue, i.e., the folder names are localized correctly.

Updating to Thunderbird 68.1.0 the issue is unfortunately back.

(In reply to Frank K from comment #20)

(In reply to FK from comment #19)

I can confirm that renaming/removing of permissions.sqlite solves the issue, i.e., the folder names are localized correctly.
Updating to Thunderbird 68.1.0 the issue is unfortunately back.

FYI: permissions.sqlite is not corrupted for me, i.e. "pragma integrity_check" does not return any error.

It just happened again (still French TB 68.1.0 version with en-US.xpi and de.xpi langpacks in same profile) :
I added a new "allow remote content from ....." exception for an A.I. newsletter, went to Preferences (Options) > Advanced > General > Language, switched from English (United States) to German and restarted TB. All folder names were in French instead of German.
This time I edited the permissions.sqlite file in a text editor, removed the two lines which had been added by the A.I. newsletter and started TB again: my manual repair of the file must have been effective since now the folders were again correctly localized in German!
Unfortunately I'm no sqlite file specialist and I still don't understand in which way permissions.sqlite would interfere in localization of the folder names.

In a further experiment I switched language to French and restarted TB in French, set again the "allow remote content from ....." exception for the same A.I. newsletter, then switched to German before restarting TB. This time the folders names were correctly localized in German!
When I looked up the contents of the permissions.sqlite file I remarked that this time the new exception for this newsletter had been added at the end of the file whereas the first time it had been added at the top of the file. Does that ring any bells?

From bug 1579747 comment 32 "It seems that giving read-only permissions to permissions.sqlite is a temporary workaround solution:
chmod -w permissions.sqlite"

Keywords: intl, regression, regressionwindow-wanted
Summary: Names of default folders are not translated when changing locale via intl.locale.requested → Names of default folders are not translated when changing locale via intl.locale.requested, involving permissions.sqlite

Apart from the above-mentioned dirty workaround, is there any news regarding a fix? The issue still exists for TB 68.1.1.

Sadly none of this makes any sense. permissions.sqlite is a binary database file, it makes no sense to open it with a text editor. If you do, you see "SQLite format 3" followed by non-printable characters.

Which platforms are affected? Linux and Mac? Any case on Windows?

(In reply to Jorg K (GMT+2) from comment #25)

Sadly none of this makes any sense. permissions.sqlite is a binary database file, it makes no sense to open it with a text editor. If you do, you see "SQLite format 3" followed by non-printable characters.

I agree. As I have already mentioned in comment #21, permissions.sqlite is not corrupted for me, i.e. "pragma integrity_check" does not return any error.

Which platforms are affected? Linux and Mac? Any case on Windows?

I am on Linux (openSUSE TW20191007), Manjaro also seems to be affected (cf. https://forum.manjaro.org/t/thunderbird-68-in-frenglish/102907).

Wow, this is incredible. On Windows with my official 68.1.2 64bit installation I downloaded a Spanish language pack from here
http://ftp.mozilla.org/pub/thunderbird/releases/68.1.2/win64/xpi/,
installed it and enabled Spanish in the advances settings. I restarted.

Everything was Spanish apart from the folder names. Then I made permissions.sqlite read-only in Windows, restarted, now the folder names were in Spanish. I made the file writeable again, restarted and now the folder names are back to English. That's totally crazy.

OK, so let's find the regression for this, if we can:
Alice, this is going to be difficult. You to know that you can switch languages in the Advanced Settings under Languages if you have pref intl.multilingual.enabled set to true and extensions.langpacks.signatures set to false.

For Dailies, you can find the language packs here, for example:
http://ftp.mozilla.org/pub/thunderbird/nightly/2019/06/2019-06-29-08-44-29-comm-central-l10n/win64/xpi/

Zibi and Francesco: Why would the localisation of folder names, retrieved from a string bundle here:
https://searchfox.org/comm-central/rev/29706036071c4629c2b44512d1acecb64008cc46/mailnews/base/util/nsMsgDBFolder.cpp#2737
have anything to do with permissions.sqlite. Any idea? Oops, Zibi has requests blocked :-(

Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(alice0775)

(In reply to Jorg K (GMT+2) from comment #27)

Wow, this is incredible. On Windows with my official 68.1.2 64bit installation I downloaded a Spanish language pack from here
http://ftp.mozilla.org/pub/thunderbird/releases/68.1.2/win64/xpi/,
installed it and enabled Spanish in the advances settings. I restarted.

Everything was Spanish apart from the folder names. Then I made permissions.sqlite read-only in Windows, restarted, now the folder names were in Spanish. I made the file writeable again, restarted and now the folder names are back to English. That's totally crazy.

Incidentally, renaming/removing permissions.sqlite gives the same result. That is very weird indeed.

Or maybe we should ask Axel.

Flags: needinfo?(l10n)

(In reply to Jorg K (GMT+2) from comment #27)

Alice, this is going to be difficult. You to know that you can switch languages in the Advanced Settings under Languages if you have pref intl.multilingual.enabled set to true and extensions.langpacks.signatures set to false.

For Dailies, you can find the language packs here, for example:
http://ftp.mozilla.org/pub/thunderbird/nightly/2019/06/2019-06-29-08-44-29-comm-central-l10n/win64/xpi/

As far as I know, folder names of imap(gmail) and local are not changed between en-US and ja build.
I do not know about lang pack. I have never used lang pack, because the lang pack is version sensitive.

Flags: needinfo?(alice0775)

Well, a language pack is like an add-on, you need to download and install it. Then you can select the newly installed version in the Advanced Settings under Languages. Yes, they are version dependent, that's why finding the regression here is tricky.

OK, here's more weirdness.

I've opened permissions.sqlite with my trusty "DB Browser for SQLite" on windows and looked at the two tables it has. moz_hosts aren't interesting, but I deleted all records anyway. No change. Then I ran this SQL on the other table:
delete from moz_perms where type = 'storageAccessAPI'
I have no idea what those storage permissions are, so I blew them all away. ... And, dada, now the folders are in Spanish.

So now we need to understand what that's about.

intl.multilingual.enabled set to true, install lang pack, options>Advanced>General>Language > select a lang & restart

TB 68.1.2(en-US build) + Japanese Language Pack68.0buildid20190909201201 : works for me(except imap/local folder).

Right. The folders should also be translated like in the localised Japanese version. So the question it: When did that break? So when did en-US + Japanese language pack not give any Japanese folder names any more.

Check my comment #32: It has something to do with the rows of type storageAccessAPI in table moz_perms of that database.

Summary: Names of default folders are not translated when changing locale via intl.locale.requested, involving permissions.sqlite → Names of default folders are not translated when changing locale via intl.locale.requested, involving permissions.sqlite (Fix: delete from moz_perms where type = 'storageAccessAPI')

Ehsan, looks like you worked on most of the code in the vicinity of storageAccessAPI. We're seeing records like this
"7758" "mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/Local%20Folders/Mozilla?number=141800" "storageAccessAPI" "1" "2" "1570788430711" "1568196430711"
in permissions.sqlite.

Blowing all the records with type storageAccessAPI away fixes the bug of Thunderbird folder names not being localised under some circumstances. That's about the weirdest bug I've ever seen. You can also remove permissions.sqlite or make it read-only to achieve the same effect.

Can you shine some light onto this?

Flags: needinfo?(ehsan)

(In reply to Jorg K (GMT+2) from comment #34)

So the question it: When did that break?

I do not understand what is problem.
Do you mean about folder names of imap?

IMAP and local folders. Check attachment 9092662 [details], everything French, but the folders are English. If you download a Japanese version of TB, you get the folders in Japanese, right? When you install en-US + Japanese language pack, you get the folders in English under some circumstances. That's incorrect.

Neither flod nor I can help here a lot.

Language packs are interesting, and their behavior is highly sensitive to timing and caching of things. All that timing changes occasionally in the backend, and then you need to figure out your caching and loading order and stuff again.

Eventually, we hope to have things in Fluent, and in a way that things update if locale selections change. Things that are not doing that need manual care.

Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)

(In reply to Jorg K (GMT+2) from comment #38)

Ehsan, looks like you worked on most of the code in the vicinity of storageAccessAPI. We're seeing records like this
"7758" "mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/Local%20Folders/Mozilla?number=141800" "storageAccessAPI" "1" "2" "1570788430711" "1568196430711"
in permissions.sqlite.

Blowing all the records with type storageAccessAPI away fixes the bug of Thunderbird folder names not being localised under some circumstances. That's about the weirdest bug I've ever seen. You can also remove permissions.sqlite or make it read-only to achieve the same effect.

Can you shine some light onto this?

These permissions are used to store information about which sites the user interacts with. They're saved through AntiTrackingCommon::StoreUserInteractionFor() and later on can be checked using AntiTrackingCommon::HasUserInteraction(). Of course none of this is for the usage of Thunderbird and it looks like some code in Thunderbird is coming across Gecko's anti-tracking code in an unexpected way and things are going haywire.

I have no theories that explain the behaviour you're seeing unfortunately and cannot think of why the existence of these permissions should matter... The only code that we have that uses HasUserInteraction() shouldn't really ever be called on Thunderbird, so whether or not these permissions exist shouldn't change the behaviour of the code that uses them. What else it changes elsewhere, I'm not sure. The only thing that occurs to me is perhaps some code is enumerating permissions, looking at their types and failing to handle storageAccessAPI or something like that. You can check for those types of possibilities by going through the nsIPermissionManager APIs and for those who could possibly give out information about these permissions add printf debugging code maybe?

Flags: needinfo?(ehsan)

Thanks for the comment, Ehsan. We don't do a whole lot with permissions in C-C, we mostly add and test permissions of type "image" for embedded images:
https://searchfox.org/comm-central/search?q=nsIPermissionManager&case=false&regexp=false&path=mail
https://searchfox.org/comm-central/search?q=addPermission&case=false&regexp=false&path=mail
https://searchfox.org/comm-central/search?q=testPermission&case=false&regexp=false&path=mail

We don't use the enumerator-style functions getAllForPrincipal, getAllWithTypePrefix or "readonly attribute nsISimpleEnumerator enumerator".

I'll add some prints to see why AntiTrackingCommon::StoreUserInteractionFor() is called for those mailbox: URLs you can see in comment #42. I'll also see where AntiTrackingCommon::HasUserInteraction() is called in TB. The good thing is that the problem is reproducible in a local trunk build using the "faulty" permissions.sqlite from my production profile.

OK, I added some debug. Just starting TB, I see no calls to HasUserInteraction and three calls to StoreUserInteractionFor with a null URI:
=== StoreUserInteractionFor called with null URI
from this debug:

void AntiTrackingCommon::StoreUserInteractionFor(nsIPrincipal* aPrincipal) {
  if (XRE_IsParentProcess()) {
    nsCOMPtr<nsIURI> uri;
    Unused << aPrincipal->GetURI(getter_AddRefs(uri));
    LOG_SPEC(("Saving the userInteraction for %s", _spec), uri);
    if (uri) {
      nsAutoCString s;
      uri->GetSpec(s);
      printf("=== StoreUserInteractionFor |%s|\n", s.get());
    } else {
      printf("=== StoreUserInteractionFor called with null URI\n");
    }

I wasn't able to attach the debugger early enough during startup, but when I click onto a folder in the TB folder tree, I also get a "StoreUserInteractionFor called with null URI" coming from this stack:

xul.dll!mozilla::AntiTrackingCommon::StoreUserInteractionFor(nsIPrincipal * aPrincipal) Line 2135	C++
xul.dll!mozilla::dom::Document::SetUserHasInteracted() Line 14958	C++
xul.dll!mozilla::EventStateManager::PreHandleEvent(nsPresContext * aPresContext, mozilla::WidgetEvent * aEvent, nsIFrame * aTargetFrame, nsIContent * aTargetContent, nsEventStatus * aStatus, nsIContent * aOverrideClickTarget) Line 511	C++
xul.dll!mozilla::PresShell::EventHandler::DispatchEvent(mozilla::EventStateManager * aEventStateManager, mozilla::WidgetEvent * aEvent, bool aEventStatus, nsEventStatus * aOverrideClickTarget, nsIContent *) Line 7798	C++
xul.dll!mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo(mozilla::WidgetEvent * aEvent, nsEventStatus * aEventStatus, bool aOverrideClickTarget, nsIContent *) Line 7770	C++
xul.dll!mozilla::PresShell::EventHandler::HandleEventWithTarget(mozilla::WidgetEvent * aEvent, nsIFrame * aNewEventFrame, nsIContent * aNewEventContent, nsEventStatus * aEventStatus, bool aIsHandlingNativeEvent, nsIContent * * aTargetContent, nsIContent * aOverrideClickTarget) Line 7654	C++
xul.dll!mozilla::PointerEventHandler::DispatchPointerFromMouseOrTouch(mozilla::PresShell * aShell, nsIFrame * aFrame, nsIContent * aContent, mozilla::WidgetGUIEvent * aEvent, bool aStatus, nsEventStatus * aTargetContent, nsIContent * *) Line 536	C++
xul.dll!mozilla::PresShell::EventHandler::DispatchPrecedingPointerEvent(nsIFrame * aFrameForPresShell, mozilla::WidgetGUIEvent * aGUIEvent, nsIContent * aPointerCapturingContent, bool aEventTargetData, mozilla::PresShell::EventHandler::EventTargetData * aEventStatus, nsEventStatus *) Line 6880	C++
xul.dll!mozilla::PresShell::EventHandler::HandleEventUsingCoordinates(nsIFrame * aFrameForPresShell, mozilla::WidgetGUIEvent * aGUIEvent, nsEventStatus * aEventStatus, bool) Line 6705	C++
xul.dll!mozilla::PresShell::EventHandler::HandleEvent(nsIFrame * aFrameForPresShell, mozilla::WidgetGUIEvent * aGUIEvent, bool aEventStatus, nsEventStatus *) Line 6531	C++
xul.dll!mozilla::PresShell::HandleEvent(nsIFrame * aFrameForPresShell, mozilla::WidgetGUIEvent * aGUIEvent, bool aEventStatus, nsEventStatus *) Line 6457	C++

Something wrong with the interaction of the clang-cl compiler and the VS debugger on Windows since it doesn't show MaybeStoreUserInteractionAsPermission() on the stack at that point, but I guess the call comes from here:
https://searchfox.org/mozilla-central/rev/5e830ac8f56fe191cb58a264e01cdbf6b6e847bd/dom/base/Document.cpp#15264

It's all a mystery to me since all this runs whether or not permissions.sqlite is in place. Observed behaviour: With the faulty permissions.sqlite in place the folder names are not localised, deleting the faulty permissions.sqlite makes them localised.

I tried

void Document::MaybeStoreUserInteractionAsPermission() {
  return;

but that doesn't fix the issue either.

So where from here?

Flags: needinfo?(ehsan)

We could be barking up the wrong tree here. Maybe those 2400+ storageAccessAPI records are triggering a storage bug?
No, see next comment.

OK, only removing IMAP entries also fixes the issue:
delete from moz_perms where type = 'storageAccessAPI' and origin like 'imap%'

Here's an IMAP entry:
"8574" "8574" "imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.Trash%3E7520" "storageAccessAPI" "1" "2" "1571733595886" "1569141595886"

In fact, I deleted all records from moz_perms except one IMAP record, and that reproduces the problem. So it looks like that IMAP origin causes a big issue. Changing imap to bimap in that record makes the issue go away.

So somehow IMAP must reach into that DB, and get confused.

Maybe we do use "readonly attribute nsISimpleEnumerator enumerator" for nsIPermissionManager in IMAP, hard to tell, there is some messing with permissions:
https://searchfox.org/comm-central/search?q=permission&case=false&regexp=false&path=imap
... and I have to leave right now.

Summary: Names of default folders are not translated when changing locale via intl.locale.requested, involving permissions.sqlite (Fix: delete from moz_perms where type = 'storageAccessAPI') → Names of default folders are not translated when changing locale via intl.locale.requested, involving permissions.sqlite (Fix: delete from moz_perms where type = 'storageAccessAPI' and origin like 'imap%')

OK, current working assumption: Something goes wrong when messing around with permissions during start-up, so we error out before reaching the code that gets the localised folder names. I'd have to add some debug/breakpoint to nsPermissionManager::GetEnumerator().

I'm leaving the NI for Ehsan so he can look at comment #44 to see whether it makes sense to call StoreUserInteractionFor() with a principal that has no URI.

Alice, thanks for looking into the issue. I guess we'll solve it with debugging and a regression range is not necessary. Most likely it broke when storageAccessAPI was introduced as a type into the permissions manager.

(In reply to Jorg K (GMT+2) from comment #47)

I'm leaving the NI for Ehsan so he can look at comment #44 to see whether it makes sense to call StoreUserInteractionFor() with a principal that has no URI.

Sure. The permission manager doesn't care about the URI for the principal, it uses the origin of the principal to store the permission, and as you can see in the dump of the sqlite file, the permissions are stored successfully.

As to your higher level question on whether this makes sense, in my opinion, no it doesn't make sense for principals representing imap/mailbox/or any other non-web URIs to have an origin, since origin is a Web concept defined in HTML. Generally when Gecko encounters non-Web things pretending to be origins, I personally wouldn't expect the resulting behaviour to make sense. I think I brought up this point recently in another bug where we were setting cookies on either imap or mailbox URIs...

But anyway, I don't think going down the path of figuring out this philosophical question would be particularly helpful to answer the question of why things fail with the presence of this permission. You still need to find the code in Thunderbird which is accessing the permission manager and finding something it doesn't expect to see how to fix this, I think. If you found it super hard to find this code, perhaps it would help to add debugging code to nsPermissionManager::GetPermissionHashKey() and also GetPrincipalFromOrigin() which are the core functions that are called when we're about to look up a permission or enumerate them.

Flags: needinfo?(ehsan)

... and as you can see in the dump of the sqlite file, the permissions are stored successfully.

Well, getting the origin from the principal I see this:
=== StoreUserInteractionFor |[System Principal]|

But yes, also see:
=== StoreUserInteractionFor |mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Jan%2018?number=9|

You still need to find the code in Thunderbird which is accessing the permission manager and finding something it doesn't expect to see how to fix this, I think.

Indeed. I'll tackle this in the next few days, it's getting late in Europe on a Friday night.

OK, I've dug some more. With a Spanish language pack installed, if permissions.sqlite contains an imap record, then GetStringFromName("inboxFolderName", kLocalizedInboxName); returns "Inbox", with no permissions.sqlite (or no imap record) it returns "Bandeja de entrada". So this seems to a Mozilla platform bug, somehow there must be a connection between L10N and the content of the permissions database. The calls were looking for won't be found in C-C code but in M-C code.

So this code
https://searchfox.org/comm-central/rev/29706036071c4629c2b44512d1acecb64008cc46/mailnews/base/util/nsMsgDBFolder.cpp#2737
always runs, but the result differs depending on content of permissions.sqlite.

(In reply to Jorg K (GMT+2) from comment #50)

OK, I've dug some more. With a Spanish language pack installed, if permissions.sqlite contains an imap record, then GetStringFromName("inboxFolderName", kLocalizedInboxName); returns "Inbox", with no permissions.sqlite (or no imap record) it returns "Bandeja de entrada".

So one could assume that GetStringFromName doesn't work, but that's not the case. The string accountDisconnected is only retrieved via GetStringFromName
https://searchfox.org/comm-central/search?q=accountDisconnected&case=false&regexp=false&path=
and that is translated properly.

Of course it sits in a different bundle, conversations.properties, whereas the untranslated folder names are in messenger.properties. So I tried a string from that bundle, LoadingMailMsgForPrint, and that is also translated. So its just the folder names. So weird.

Did some more digging. In the bad case, the folder name strings are about the first strings to be retrieved:

Line 139: === GetStringFromName(inboxFolderName) = |Inbox| (0)
Line 141: === GetStringFromName(trashFolderName) = |Trash| (0)
Line 142: === GetStringFromName(sentFolderName) = |Sent| (0)
Line 143: === GetStringFromName(draftsFolderName) = |Drafts| (0)
Line 144: === GetStringFromName(templatesFolderName) = |Templates| (0)
Line 145: === GetStringFromName(junkFolderName) = |Junk| (0)
Line 146: === GetStringFromName(outboxFolderName) = |Outbox| (0)
Line 147: === GetStringFromName(archivesFolderName) = |Archives| (0)
Line 148: === GetStringFromName(brandShortName) = |Daily| (0)
Line 149: === GetStringFromName(mailnews.view_default_charset) = |ISO-8859-1| (0)
Line 159: === GetStringFromName(openH264_name) = |OpenH264 Video Codec proporcionado por C
Line 161: === GetStringFromName(openH264_description2) = |Este plugin se ha instalado auto

and after that, it those folder name strings, strings come out in Spanish. In the good case, the plugin related strings are retrieved first:

Line 106: === GetStringFromName(brandShortName) = |Daily| (0)
Line 129: === GetStringFromName(openH264_name) = |OpenH264 Video Codec proporcionado por C
Line 131: === GetStringFromName(openH264_description2) = |Este plugin se ha instalado auto
Line 136: === GetStringFromName(widevine_description) = |MA3dulo de descifrado de contenid
Line 138: === GetStringFromName(cdm_description2) = |Este complemento permite la reproducc
Line 162: === GetStringFromName(inboxFolderName) = |Bandeja de entrada| (0)
Line 164: === GetStringFromName(trashFolderName) = |Papelera| (0)
Line 165: === GetStringFromName(sentFolderName) = |Enviados| (0)
Line 166: === GetStringFromName(draftsFolderName) = |Borradores| (0)
Line 167: === GetStringFromName(templatesFolderName) = |Plantillas| (0)
Line 168: === GetStringFromName(junkFolderName) = |Correo no deseado| (0)
Line 169: === GetStringFromName(outboxFolderName) = |Bandeja de salida| (0)
Line 170: === GetStringFromName(archivesFolderName) = |Archivos| (0)

So there is some timing issue in loading the language pack. Quoting Axel from comment #41:

Language packs are interesting, and their behavior is highly sensitive to timing and caching of things. All that timing changes occasionally in the backend, and then you need to figure out your caching and loading order and stuff again.

So I think that's what happens. It's still curious that it depends on the content of the permissions.sqlite database.

OK, I think I've tracked it down.

When permissions.sqlite is imported, it runs GetPrincipalFromOrigin() on the imap origin/URL that is stored in the database:
https://searchfox.org/mozilla-central/rev/e54b007827800410a616ce0584ccaae308a4afd9/extensions/permissions/nsPermissionManager.cpp#2832

That in the calls NS_NewURI() for an imap origin and that runs a lot of IMAP code (we know that imap: URL creation runs an awful lot of code), including retrieving the localised folder names, before the localised string bundle is even loaded.

That's it. No imap: URL in the database, no problem.

I'm going to propose a patch to skip MailNews URLs on import. Let's see what Ehsan thinks.

Attached file permissions.sqlite —

Here is the permissions.sqlite database the reviewer can use for testing. He can also install a German language pack from here:
http://ftp.mozilla.org/pub/thunderbird/nightly/2019/10/2019-10-13-07-58-41-comm-central-l10n/linux-x86_64/xpi/

Attachment #9086978 - Attachment is obsolete: true
Attachment #9087406 - Attachment is obsolete: true
Attachment #9087541 - Attachment is obsolete: true
Attachment #9087852 - Attachment is obsolete: true
Attachment #9089748 - Attachment is obsolete: true

I tried to do a C-C-only solution by initialising the strings only when needed, but it turns out that that also doesn't work. This patch here gets the strings all the time before they are used and that guarantees localised results.

The only other solution would be to teach M-C about MailNews URLs and then skip those upon import, see comment #53. Oh, I just had another idea. The permissions manager only worries about http(s) and ftp, so let me do an M-C patch for that.

Assignee: nobody → jorgk
Status: REOPENED → ASSIGNED

OK, let's the summarise the issue again:

When the permissions are read from permissions.sqlite at start-up, URIs are created from the stored origins. Creating an imap: URI runs a whole lot of MailNews code, which also reads localised names of the standard folder names into static variables. Sadly that happens at the time where l10n is not initialised yet, so users see the untranslated names later.

Possible solutions:

  1. When reading permissions.sqlite, only import http(s), ftp (and sadly chrome) origins (attachment in Phab)
  2. Always translate strings when needed, don't pre-cache them during start-up (attachment 9100751 [details] [diff] [review])
  3. Hacky and nonsense: Only lookup the strings 50 times. That's an empirical value. After 20 lookups there were still not translated, after 50 lookups they were (attachment 9100764 [details] [diff] [review]).

Thanks for your comments, Ehsan.

If you want to deliberate about what the real/actual problem is, here's my view:

The root issue is that storageAccessAPI permissions are stored for non-web origins, that is MailNews schemes like imap: or mailbox:. Do I read comment #48 correctly and we agree on that? Quote: "no it doesn't make sense for principals representing imap/mailbox/or any other non-web URIs to have an origin".

I in fact know that TB needs image permission for http(s) and chrome schemes and nothing else. I don't know what other permissions are needed under the hood for the underlying Mozilla platform code, so in that sense the patch could have been quite fatal ;-)

So would you accept a patch for TB (with #ifdef) to suppress storageAccessAPI types for non-http(s): and chrome: schemes when reading the permissions back from the database and potentially also a part that would prevent those permissions from being stored in the first place? That's my preferred solution.

Continuing the deliberation on the real problem:

The next issue is that the permissions manager instantiates nsIURI objects when restoring the permissions from the database.

The next issue is that creating a new imap: URI runs a lot of MailNews code,
https://searchfox.org/comm-central/rev/364e5ba7492c8a13e104662a7e43769819e339f7/mailnews/imap/src/nsImapService.cpp#2375
including handling/initialising folders and hence trying to get localised strings and store them in static memory before those strings are actually available.
https://searchfox.org/comm-central/rev/29706036071c4629c2b44512d1acecb64008cc46/mailnews/base/util/nsMsgDBFolder.cpp#2727

This part could be rephrased to saying: The problem is that the permissions system is initialised before the language pack loads.

I don't know how you envisage to achieve (quote) "making the code that is reading this string wait on the loading of the thing that it is depending on". Permissions code, MailNews folder initialisation code and loading the strings from the language pack are completely independent. All I can imagine is to listen to some "mail-startup-done" event and re-reading the strings/folder names then.

So where from here?

Flags: needinfo?(ehsan)

(In reply to Jorg K (GMT+2) from comment #59)

Thanks for your comments, Ehsan.

If you want to deliberate about what the real/actual problem is, here's my view:

The root issue is that storageAccessAPI permissions are stored for non-web origins, that is MailNews schemes like imap: or mailbox:. Do I read comment #48 correctly and we agree on that? Quote: "no it doesn't make sense for principals representing imap/mailbox/or any other non-web URIs to have an origin".

That is one manifestation of the problem, but not the root issue. The root issue is that the permission manager allows storing permissions for any URI scheme for any permission type. The storageAccessAPI permission type just happens to be the type of permission that trickled the bug in this case, but if some other code today or tomorrow saves other permission types for some other unexpected URI scheme in the permission manager database you'd more or less have the exact same effect as a result.

I in fact know that TB needs image permission for http(s) and chrome schemes and nothing else. I don't know what other permissions are needed under the hood for the underlying Mozilla platform code, so in that sense the patch could have been quite fatal ;-)

Your patch modifies SeaMonkey behaviour as well...

If you're going to rely on this knowledge, you should probably formalize the expectations in the form of some MOZ_RELEASE_ASSERTs that crash Thunderbird and SeaMonkey if any code ever violate your expectations accidentally, and write some code to migrate the permission manager database for existing installations since those may include the types of permissions which newer builds may crash upon loading. Given that SeaMonkey should probably have a superset of Firefox's behaviour, and no such expectations exist for Firefox, I have a very hard time that this is a productive path to coming up with a good solution for this problem, but you're welcome to pursue it.

So would you accept a patch for TB (with #ifdef) to suppress storageAccessAPI types for non-http(s): and chrome: schemes when reading the permissions back from the database and potentially also a part that would prevent those permissions from being stored in the first place? That's my preferred solution.

No, that's a wallpaper solution that doesn't address the underlying problem. As I explained above, any patch that tries to solve this at the level of the permission manager needs to crash Thunderbird and SeaMonkey for invalid input into permission manager and upgrade the database. Again, this is not a good direction to solve the problem.

Continuing the deliberation on the real problem:

The next issue is that the permissions manager instantiates nsIURI objects when restoring the permissions from the database.

The next issue is that creating a new imap: URI runs a lot of MailNews code,
https://searchfox.org/comm-central/rev/364e5ba7492c8a13e104662a7e43769819e339f7/mailnews/imap/src/nsImapService.cpp#2375
including handling/initialising folders and hence trying to get localised strings and store them in static memory before those strings are actually available.
https://searchfox.org/comm-central/rev/29706036071c4629c2b44512d1acecb64008cc46/mailnews/base/util/nsMsgDBFolder.cpp#2727

This part could be rephrased to saying: The problem is that the permissions system is initialised before the language pack loads.

I understand all of this, and I think you need to think a bit outside of the box here. Think about what would have happened if it wasn't the permission manager, but (for example) PSM, or DOM storage, or safebrowsing, etc. etc. etc. which were trying to process a URI at startup and somehow ended up having an imap URI on their hand. It's not even possible to find all such dependencies, let alone fix every single similar bug by adding custom workarounds like the one you're suggesting here. What I'm trying to suggest to you instead is to figure out the actual requirements of the IMAP service, whatever they may be, and write the proper code to ensure that all dependencies are loaded before we need to process our first IMAP URI. That way you wouldn't need to touch the permission manager at all, and neither would you need to worry about other similar cases where this problem may be showing up.

In other words...

I don't know how you envisage to achieve (quote) "making the code that is reading this string wait on the loading of the thing that it is depending on". Permissions code, MailNews folder initialisation code and loading the strings from the language pack are completely independent. All I can imagine is to listen to some "mail-startup-done" event and re-reading the strings/folder names then.

So where from here?

... make the code reading this string (the IMAP service) wait on the thing it is depending on (the localized strings) to become available[1]. :-)

[1] Maybe my understanding is overly simplistic here and there is a bit more to it which was why I was so vague previously. I hope now I've explained the idea I'm going for better.

Flags: needinfo?(ehsan)

Hey Ehsan, thanks for your further comment. Let's face it: SeaMonkey is dead beyond the equivalent of mozilla60 (sixty, no typo), which they haven't even released yet. TB with about 10 staff has replaced all overlays, XBL bindings and are now working on the XUL to XHTML transition. SM has done none of that work, so at trunk, they can compile but not start :-( - We could also have conditional compile for TB only. But since TB also serves as a browser when used in HTML mode for RSS articles, I agree with your argument.

Anyway, your further argument that other parts of Gecko could instantiate an imap: URL and hence trigger the train of disaster that we're seeing now, and that we can't "whack every mole", has convinced me :-)

Yes, the waiting bit is going to be hard, since you're saying that I should wait somewhere in IMAP land for some strings to arrive while being called synchronously from the permission manager while creating an imap: URI. I can't see how that would work.

All I can see working, as I said, is to wait for the strings to arrive and then read them, and any call for the folder names before that would get the non-localised strings and we can only hope they haven't been propagated elsewhere into the UI, or we need to replace them there, too.

Attachment #9100752 - Attachment is obsolete: true
Attached patch 1575512-jit-strings.patch (v3) (obsolete) — — Splinter Review

This does the trick. I didn't add the observer, it seems more useful to block the undesired string lookup during IMAP's creation of the URI which also messes with folders. The visual effect is that folders first show in English and get localised a short time later. The observer solution would be the same, we'd need to re-retrieve the strings when it fires. Maybe that would fire earlier.

Attachment #9100751 - Attachment is obsolete: true
Attachment #9100764 - Attachment is obsolete: true
Attachment #9101408 - Flags: review?(acelists)
Attached patch 1575512-jit-strings.patch (v3b) (obsolete) — — Splinter Review

This is a bit better since it counts the enable/disable calls. The effect is the same, for IMAP folders, the correct names show up a little later. I'll try the observer solution tomorrow.

Attachment #9101408 - Attachment is obsolete: true
Attachment #9101408 - Flags: review?(acelists)
Attachment #9101418 - Flags: feedback?(acelists)
Attached patch 1575512-mail-startup-done.patch (obsolete) — — Splinter Review

OK, here is the observer variation. Correct folder names lag behind just like with the other patch. I think I prefer the other solution since we're not using big observer guns and we're not affecting people who don't use IMAP.
EDIT: I forgot to say: This was a 30 minute job copying https://hg.mozilla.org/comm-central/rev/f2405b6ea062

Attachment #9101508 - Flags: feedback?(acelists)

Listening to final-ui-startup works a bit better, still delays on the IMAP folder names.

Attachment #9101508 - Attachment is obsolete: true
Attachment #9101508 - Flags: feedback?(acelists)
Attachment #9101510 - Flags: feedback?(acelists)
Comment on attachment 9101510 [details] [diff] [review] 1575512-mail-startup-done.patch (v1b) I guess that would add an observer to every folder created. That's not so desirable. Perhaps we should remember whether the event already happened and after that not create any observers any more. That's a refinement I can make once we decided to use this solution and not the other one.

OK, we only add observers until our event fires.

Attachment #9101510 - Attachment is obsolete: true
Attachment #9101510 - Flags: feedback?(acelists)
Attachment #9101930 - Attachment is patch: true
Attachment #9101930 - Flags: feedback?(acelists)

Even better, we attach the observer only to the very first folder.

Attachment #9101930 - Attachment is obsolete: true
Attachment #9101930 - Flags: feedback?(acelists)

OK, with the patch we see the main window come up and the names of local folders are already localised, the ones of IMAP folders aren't. They get corrected a little later. Debugging shows that this happens during nsImapIncomingServer::PossibleImapMailbox() which calls SetPrettyName(). Looks like the name of the Inbox is corrected even later in nsImapIncomingServer::CheckSpecialFolder().

Attachment #9101418 - Attachment is obsolete: true
Attachment #9101418 - Flags: feedback?(acelists)
Comment on attachment 9101968 [details] [diff] [review] 1575512-mail-startup-done.patch (v2b) OK, I think this works well enough. DiscoveryDone() will always run and if there's a short delay, so be it. We'd have to try this on an opt build.
Attachment #9101968 - Attachment is patch: true
Attachment #9101968 - Flags: review?(acelists)

Aceman was worried that we might not run the code that fixes the folder names. So here's a different approach. We don't do the "folder stuff" in nsImapService::NewURI() which is mostly optional anyway. Surely we don't need it when URIs are created from the permissions manager. With this patch, the main window comes up and all the folders have the correct names straight away. Yay.

Attachment #9101990 - Flags: review?(acelists)
Attachment #9101990 - Attachment is patch: true
Attached patch folder-debug.patch (obsolete) — — Splinter Review

Here's a debug patch.

Aceman observed correctly, that without a faulty permissions.sqlite the observer never fires. Looking at the debug, it is is attached after the system has already fired the event. Whether it fires or not is irrelevant, it will always hang around on the first folder we encounter. It's only important that if folders are created before the UI initialisation, that we re-read the strings. That's what the patch does.

Outch, but if the event never fires, then "names ready" will never return true, and the IMAP NewURI() will always suppress the folder stuff :-( - Grrrr. Does that mean we need to go with version 2b which didn't to the IMAP hackery but instead relied on the names being fixed later.

Comment on attachment 9101990 [details] [diff] [review] 1575512-mail-startup-done.patch (v3) We can't use this, see comment #72.
Attachment #9101990 - Attachment is obsolete: true
Attachment #9101990 - Flags: review?(acelists)
Attached patch folder-debug.patch (obsolete) — — Splinter Review

This matches v2b.

Attachment #9101999 - Attachment is obsolete: true
Attached patch 1575512-jit-strings.patch (v3c) (obsolete) — — Splinter Review

Thinking about it, the JIT approach also has its merits. Aceman was concerned that the SetPrettyName() would happen while we don't have the strings, so I fixed this in this version. I'll attach a matching debug patch in a minute.

Attachment #9102073 - Flags: feedback?(acelists)
Attached patch folder-debug-jit.patch (obsolete) — — Splinter Review

OK, here's the debug patch. It shows:
=== NewURI start block
=== accessing string INBOX in SetPrettyName, blocked=1
=== accessing string Archives in SetPrettyName, blocked=1
=== accessing string Drafts in SetPrettyName, blocked=1
=== accessing string Sent in SetPrettyName, blocked=1
=== accessing string Templates in SetPrettyName, blocked=1
=== accessing string Trash in SetPrettyName, blocked=1
=== NewURI end block
=== NewURI start block
=== NewURI end block
and more and later: many like:
=== accessing string Trash in SetPrettyName, blocked=0

The initially wrong folder names of IMAP folders get fixed later on just like in the observer solution.

So whilst this JIT version is a little hackier, it avoids attaching observers which may or may not fire.

Attachment #9102073 - Attachment is patch: true

Thinking about this, I had a better idea: We should not handle the final-ui-startup in the folder code (where the observer is attached too late), but elsewhere, like in MailGlue.

When the event arrives call into the folders code to re-read the strings. Also, in IMAP's NewURI() suppress folder handling for the "throw-away" creations that happens before the event arrives (like in 1575512-mail-startup-done-v3.patch).

The hard part is to create a folder API that MailGlue can call.

The hard part is to create a folder API that MailGlue can call.

Or it could be the other way around: Folder code checks somewhere if final-ui-startup has already run.

(In reply to Jorg K (GMT+2) from comment #61)

Hey Ehsan, thanks for your further comment. Let's face it: SeaMonkey is dead beyond the equivalent of mozilla60 (sixty, no typo), which they haven't even released yet. TB with about 10 staff has replaced all overlays, XBL bindings and are now working on the XUL to XHTML transition. SM has done none of that work, so at trunk, they can compile but not start :-(

Oh, sorry to hear about that. :-/ I wasn't aware...

  • We could also have conditional compile for TB only. But since TB also serves as a browser when used in HTML mode for RSS articles, I agree with your argument.

Anyway, your further argument that other parts of Gecko could instantiate an imap: URL and hence trigger the train of disaster that we're seeing now, and that we can't "whack every mole", has convinced me :-)

Good to hear :-). Sorry to have pushed back, I know that it sucks to receive pushback from a reviewer when they don't really know what better solution to offer and they're effectively telling you "please spend more time to come up with something else!", so I really appreciate you considering my argument here.

Yes, the waiting bit is going to be hard, since you're saying that I should wait somewhere in IMAP land for some strings to arrive while being called synchronously from the permission manager while creating an imap: URI. I can't see how that would work.

Waiting for asynchronous stuff when something is called synchronously is indeed hard, but it can sometimes be achieved with some care if you know something about what it is that you're waiting for and what triggers it to happen. The way to do that is usually through setting up a nested event loop to process incoming events to your thread (in this case the main thread) until the condition you're waiting for (e.g. the strings becoming available) has been met.

We have the mozilla::SpinEventLoopUntil() helper function for dealing with these nested event loops, with many examples around the code base. It looks like you're going on a different path now (which I admit I don't fully understand!) but I thought I'd mention this in case it proves useful anyway.

Hi Ehsan, thanks for your continued interest and comments. I'll have the final patch ready in the next hour according to my idea in comment #78.

I think the right solution is not to do the "damaging operations" in the IMAP code why the system hasn't fully started up.

Attached patch 1575512-glue.patch (obsolete) — — Splinter Review

OK, here comes the (hopefully) final solution. It features:

  • Using the existing observer in mailGlue which is guaranteed to fire.
  • Suppressing throw-away IMAP NewURI folder processing while strings aren't ready
  • No starting up with wrong folder names that are corrected later.
Attachment #9101968 - Attachment is obsolete: true
Attachment #9102048 - Attachment is obsolete: true
Attachment #9102073 - Attachment is obsolete: true
Attachment #9102074 - Attachment is obsolete: true
Attachment #9101968 - Flags: review?(acelists)
Attachment #9102073 - Flags: feedback?(acelists)
Attachment #9102584 - Flags: review?(acelists)
Attached patch folder-debug-glue.patch (obsolete) — — Splinter Review

Here's the debug patch that goes with it.

Attached patch 1575512-glue.patch - Simplified (obsolete) — — Splinter Review

Here's a simplified version of the idea. The folder code doesn't check any more whether the strings are ready, it just assumes that they are ready when really needed. This works just as well as the other patch but is less secure.

Comment on attachment 9102584 [details] [diff] [review] 1575512-glue.patch Review of attachment 9102584 [details] [diff] [review]: ----------------------------------------------------------------- It's a pity we have to add these hacks for the current localization infrastructure. But it is maybe also a bit unusual we cache strings in the backend like this. I like this non-simplified version better. It could be useful to have that gInitializeStringsDone variable indicate if we think the strings are OK, instead of blindly assuming they are (in the simplified version). Thanks for pursuing this with the various approaches you have tried. ::: mail/components/nsIMailGlue.idl @@ +17,5 @@ > * > */ > void sanitize(in nsIDOMWindow aParentWindow); > > + void finalUiStartupDone(out boolean aDone); Can this be made 'attribute boolean finalUiStartupDone' ? ::: mailnews/base/util/nsMsgDBFolder.cpp @@ +2727,2 @@ > nsresult rv; > + if (!aForce) { Can you add some TODO comment that all these hacks can go if m-c changes the localization infrastructure so that the localized strings are available immediately? @@ +3182,5 @@ > (name.LowerCaseEqualsLiteral("outbox") && nonEnglishApp()); > } > > NS_IMETHODIMP nsMsgDBFolder::SetPrettyName(const nsAString &name) { > + initializeStrings(); Could we actually check the return value of initializeStrings? It can fail at many sports and reports the errors, but we never check it at the callers.
Attachment #9102584 - Flags: review?(acelists) → review+

Can this be made 'attribute boolean finalUiStartupDone' ?

You mean "readonly". I don't think so. I don't have an instance of nsIMailGlue. I just get the service and run one of its methods.

I'll worry about the other comments tomorrow.

Attached file 1575512-glue.patch (v1b) (obsolete) —

Here's the final patch with added comments and error checking as well as added #ifdef since otherwise SeaMonkey won't compile since it doesn't have mailGlue (but suiteGlue instead).

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=feccca1eb30ca259599ff91a887f68c1d942f5b7

Attachment #9102584 - Attachment is obsolete: true
Attachment #9102586 - Attachment is obsolete: true
Attachment #9102588 - Attachment is obsolete: true
Attachment #9102698 - Flags: review+

Hmm, two Xpcshell test failures:
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/db/gloda/test/unit/test_index_messages_imap_offline.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | comm/mailnews/db/gloda/test/unit/test_index_messages_imap_online_to_offline.js | Test timed out

Most likely they fail since due to the IMAP hunk. I guess final-ui-startup doesn't fire in headless Xpcshell mode and therefore that folder code is now permanently disabled. Oh joy. Well, at least we know it's good for something ;-)

Changing that event to profile-after-change doesn't help. In the IMAP code ready is always false :-( - Setting it to true makes the test pass as expected.

Attached patch fix-tests.patch (obsolete) — — Splinter Review

OK, this makes the tests pass. I'll merge it onto the main patch if you approve.

Attachment #9102703 - Flags: review?(acelists)
Attached patch 1575512-glue-mark2.patch (obsolete) — — Splinter Review

Here's the whole thing reshuffled. We don't call into the glue any more but the glue calls the folders.

This is the simpler solution although the patch is bigger due to all the boilerplate.

Attachment #9102698 - Attachment is obsolete: true
Attachment #9102703 - Attachment is obsolete: true
Attachment #9102703 - Flags: review?(acelists)
Attachment #9102751 - Flags: review?(acelists)
Attached patch 1575512-glue-mark2.patch (v1b) (obsolete) — — Splinter Review

Slightly optimised, we only check the app name once, so if there are thousands of URI in the permissions file, we don't get the app name repeatedly. Debug patch doesn't apply any more.

Attachment #9102751 - Attachment is obsolete: true
Attachment #9102751 - Flags: review?(acelists)
Attachment #9102753 - Flags: review?(acelists)
Attachment #9102753 - Attachment is patch: true
Comment on attachment 9102753 [details] [diff] [review] 1575512-glue-mark2.patch (v1b) Review of attachment 9102753 [details] [diff] [review]: ----------------------------------------------------------------- I find it a bit excessive to add a whole new class for this, but it does work so if you like it better... Please again add the TODO comment of this being a temporary hack. I assume this does not break Seamonkey now. ::: mailnews/imap/src/nsImapService.cpp @@ +2416,5 @@ > server->GetRootFolder(getter_AddRefs(rootFolder)); > + bool ready; > + if (rootFolder && !folderName.IsEmpty() && > + // Skip folder processing if folder names aren't ready yet. > + // That happens early in initalisation. typo
Attachment #9102753 - Flags: review?(acelists) → review+

Fixed typo and added more comments.

We've discussed improved error handling on IRC. initializeStrings() shouldn't really fail, unless someone removes some of the eight folder name strings or the system is so damaged that it can't find the bundle.

Passing the error our to the final-ui-startup observer is also not useful since it could only report it.

While looking at the code I saw four uses of nsString(XXX).get() which is of course nonsense.

Attachment #9102753 - Attachment is obsolete: true
Attachment #9102763 - Flags: review+
Attachment #9102763 - Attachment is patch: true

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8c7735bd0dc3
Avoid IMAP folder operations until final-ui-startup is done. r=aceman DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Attachment #9102763 - Flags: approval-comm-esr68+

Thanks a lot for your efforts! Does this mean that TB 68.x will get the fix soon?

Yes, I'm thinking about when to ship this. I think it's too risky for 68.2.0, so it will likely be in 68.2.1 in more than a week. You can always run the SQL given in the summary on your permissions.sqlite file.

Many many thanks for your relentless efforts to fix this issue, Jörg.
Being a regular locale switcher I highly appreciate your insistence!

OK, I'm running with 68.2.1 now and something terrible has happened. I have a Spanish language pack installed, and although it's not active, the folders are now all in Spanish. Eckard, can you please try this version. You're on Mac, right?
https://queue.taskcluster.net/v1/task/L17h8vIXSx6peUpjHBp2yg/runs/0/artifacts/public/build/target.dmg

So the fix that works on trunk doesn't work on 68, or worse, it's so zealous that you get the folder names from a language pack you don't want :-(

Flags: needinfo?(de.berberich)

I've played with it a bit more. It works OK as long as the offending language isn't listed in the "Alternatives". So removing Spanish from the alternatives brings the English folder names back. So I guess there's a bug in the Mozilla platform which we won't get fixed for TB 68.

I confirm the issue you describe in comment #101.
I installed TB 68.2.1 (en-US) from the above link, created a new profile and in Preferences (Options) > Advanced > General > Languages > Set Alternatives added French and German as alternatives, thus installing the French and the German language packs in Add-ons > Languages.

Switching from English to French or to German and then switching between German and French works flawlessly and the folder names or correctly localized.
But switching back to English from German or from French doesn't localize the folder names back in English, they would stay respectively in German or in French.

Removing French and/or German via "Set Alternatives" doesn't work for me. After restarting TB the French and/or German languages are still displayed in the "Thunderbird Language Settings" window and the folder names are still localized in French or German instead of English.
You'll have to remove or at least disable both language packs in Add-ons > Languages to force TB to localize the folder names back to English.

Flags: needinfo?(de.berberich)

So what are we doing here then? I've spent an extra-ordinary about of time on the niche feature which is clearly quite buggy in mozilla68. I'm not inclined to dedicate more time to it. So either I remove the patch again from TB 68 and you never get localised folder names, or we leave it "as is" with the "bad after-taste" of the previous language :-( - Could you check the new beta, TB 71. As far as I can see, it works there.

Hi Axel, maybe you can help me here:

If landed a patch https://hg.mozilla.org/comm-central/rev/8c7735bd0dc3 to re-initialise strings for folder names on final-ui-startup. We also suppress any processing creating folders with the wrong names before that point. The patch works on trunk and beta, but not on ESR. There, upon getting the strings again, we get strings from a language pack (which clearly is available at final-ui-startup) which isn't the chosen UI language. That looks like and M-C bug. Maybe I can fix the issue in TB 68 by backporting something what may have been fixed in the Mozilla platform at some stage. Do you know of any bugs in this area?

Flags: needinfo?(l10n)

(In reply to Jorg K (GMT+2) from comment #104)

Could you check the new beta, TB 71. As far as I can see, it works there.

I cannot test this in TB 71.0b1 since there are no language packs available for this version!

No idea. There's two ways to get to a locale through the chrome reg:

  • requested languages has that before en-US
  • there's no en-US chrome package registered for messenger

How either of those two could happen for just a time-window on startup ..... no idea.

I also don't think that anything changed there recently.

To say something constructive, caching the folder names could be the actual problem. I'd suggest to invalidate that caching on intl:app-locales-changed, https://firefox-source-docs.mozilla.org/intl/locale.html#events. Not sure if there's a smooth way to make sure the UI updates.

Even longer-term, if you could make the display string of those folders be Fluent and data-l10n-id, you shouldn't have these problems at all.

Flags: needinfo?(l10n)

I'd suggest to invalidate that caching on intl:app-locales-changed

Wow, that works, I've locally patched my TB 68.2.1 installation. That's an easy fix on top of what we have already, thanks, Axel!!

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch 1575512-follow-up.patch (obsolete) — — Splinter Review

This is the beta/ESR 68 version, trunk to follow.

(In reply to Jorg K (GMT+2) from comment #106)

Please get them from here: http://ftp.mozilla.org/pub/thunderbird/releases/71.0b1/linux-x86_64/xpi/

Thanks. I installed en-US English and German as supplementary UI languages in my French TB 71.0b1 version, switched a dozen times from one language to another, restarted TB, allowed remote contents for several correspondents and restarted, switched again ..... Folder names are always correctly localized. So it seems that this issue is not reproducible in TB 71.

Attached patch 1575512-follow-up-trunk.patch (obsolete) — — Splinter Review

Easy tweak to finally get this working across the board.

Attachment #9104179 - Flags: review?(acelists)
Attachment #9104178 - Flags: approval-comm-esr68+
Attachment #9104178 - Flags: approval-comm-beta+
Attached patch 1575512-follow-up-trunk.patch (obsolete) — — Splinter Review

Hold on, I assume that intl:app-locales-changed doesn't fire in a stock-standard installation with no language packs. We still need to initialisation to make sure the IMAP processing is unblocked.

Attachment #9104178 - Attachment is obsolete: true
Attachment #9104179 - Attachment is obsolete: true
Attachment #9104179 - Flags: review?(acelists)
Attachment #9104180 - Flags: review?(acelists)

OK, this makes my TB 68.2.1 work.

Attachment #9104193 - Flags: approval-comm-esr68+
Attachment #9104193 - Flags: approval-comm-beta+

Sigh, fs was declared twice.

Attachment #9104180 - Attachment is obsolete: true
Attachment #9104180 - Flags: review?(acelists)
Attachment #9104194 - Flags: review?(acelists)
Comment on attachment 9104194 [details] [diff] [review] 1575512-follow-up-trunk.patch I guess it's hard to review since it makes no difference on trunk. You have have to trust me that is fixes TB 68 or do the experiment yourself of getting a version from treeherder and patching the file.

I've done a bit more debugging here.

On trunk, at startup, the events fire in this order: intl:app-locales-changed en-US, final-ui-startup. So we get the folder names when the latter event fires, and we're cool. All working as observed on beta.

On TB 68 I see: intl:app-locales-changed es-ES, final-ui-startup and then another intl:app-locales-changed en-US. en-US is the chosen language. So frankly, mozilla68 seems buggy, it advertises the wrong language first and then the correct language after the UI event has fired :-(

So the 68 patch is correct to work around it. I'm not sure whether we want to land the trunk/beta part.

Axel, thanks again for your suggestion. As you can see, the behaviour has changed, in 68 it wasn't quite correct, now it is. Maybe it's faster to go with the workaround for TB 68 instead of trying to locate where the Mozilla platform changed and then trying to backport that.

(In reply to Jorg K (GMT+2) from comment #116)

I've done a bit more debugging here.

On trunk, at startup, the events fire in this order: intl:app-locales-changed en-US, final-ui-startup. So we get the folder names when the latter event fires, and we're cool. All working as observed on beta.

On TB 68 I see: intl:app-locales-changed es-ES, final-ui-startup and then another intl:app-locales-changed en-US. en-US is the chosen language. So frankly, mozilla68 seems buggy, it advertises the wrong language first and then the correct language after the UI event has fired :-(

So the 68 patch is correct to work around it. I'm not sure whether we want to land the trunk/beta part.

Axel, thanks again for your suggestion. As you can see, the behaviour has changed, in 68 it wasn't quite correct, now it is. Maybe it's faster to go with the workaround for TB 68 instead of trying to locate where the Mozilla platform changed and then trying to backport that.

Does this mean that your patch is obsolete for TB 71+?

Yes, what I'm about to land on 68 is not required for 71+. We can deliberate whether we still want if for consistency.

(In reply to Jorg K (GMT+2) from comment #118)

Yes, what I'm about to land on 68 is not required for 71+. We can deliberate whether we still want if for consistency.

Since TB71 is far from being published, I would give your patch a try. Thx.

I'd recommend to keep watching the intl notification. I would totally expect more changes to startup code paths as we extend more fluent and intl stuff in m-c, and that's what we expect the app to respond to.

Since TB71 is far from being published, I would give your patch a try. Thx.

Available as beta now from https://www.thunderbird.net/en-US/thunderbird/beta/all/

@Axel: OK, we'll land the C-C part then as well.

TB 68.2.1 (follow-up):
https://hg.mozilla.org/releases/comm-esr68/rev/c540ca7f085dcc455cf0250bb7dc5a4d0d02eddd

I'll land the C-C part soon after review, yes, a little unorthodox ;-)

Comment on attachment 9104194 [details] [diff] [review] 1575512-follow-up-trunk.patch Review of attachment 9104194 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, I think this will be useful on trunk too.
Attachment #9104194 - Flags: review?(acelists) → review+

Eckard, if you're not bored yet, this version will do it:
https://queue.taskcluster.net/v1/task/WQERYcM6Ty68YSs7ngJnNQ/runs/0/artifacts/public/build/target.dmg

Flags: needinfo?(de.berberich)

On the contrary, I'm eager to go on and will do some testing tonight with your patched version!

Flags: needinfo?(de.berberich)

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3834c3b93900
Follow-up: Re-initialise folder names on intl:app-locales-changed. r=aceman

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED

(In reply to Jorg K (GMT+2) from comment #124)

Eckard, if you're not bored yet, this version will do it:
https://queue.taskcluster.net/v1/task/WQERYcM6Ty68YSs7ngJnNQ/runs/0/artifacts/public/build/target.dmg

Excellent!! I just tested this version for two hours. No more problem with the folder names when switching locales!

Summary: Names of default folders are not translated when changing locale via intl.locale.requested, involving permissions.sqlite (Fix: delete from moz_perms where type = 'storageAccessAPI' and origin like 'imap%') → Default IMAP folder names not localised when language pack is used [involving permissions.sqlite (Fix: delete from moz_perms where type = 'storageAccessAPI' and origin like 'imap%')]

TB 71 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/5da939a404338d73dd43e51cb5d360cdceabc8f6

(In reply to :aceman from comment #123)

Thanks, I think this will be useful on trunk too.

I doubt it. If the language changes without a restart, we can re-initialise the folder name cache all we want, but that won't update the names in the UI.

FYI: The issue is solved for me with TB 68.2.1. Works pretty well with the new multilingual UI. Thank you!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: