1235205 - Selected spellcheck language setting lost under some circumstances

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

I'm using Earlybird 45.0a2 (2015-12-24).
I finally managed to track down a bug which has been annoying me for a long time.

STR:
- Use a US version of TB, install a en-GB dictionary.
- In Tools > Options, Composition, Spelling, set language to English/UK.
- Press <CTRL>N to start a new message. Observe that the language
  indicator shows English (GB). (*)
- Press <CTRL>N again to start another new message. Observe that the language
  indicator shows English (GB). (*)
- Close both messages.
- Check Tools > Options, Composition, Spelling again, language is
  still at English/UK.
- (Optional: <CTRL>N again to convince yourself that it's *really* English/UK.)
- Close TB.
- grep through prefs.js for spellchecker.dictionary.
  Result: Not found, the preference was cleared.
- Start TB again and check the language: Result: English/US
  No surprise, since the preference was cleared, so it comes back with
  the default for the US version.

(*) You can even type English/UK words like "colour" or "flavour" into the message to see that the language indicator is not lying. The language *is* British English.

BTW, it doesn't need to be English/UK, you can nominate any other dictionary, I tried German and Spanish and they both are lost after closing TB if you follow these steps.

Comment 1

[Jorg K (CEST = GMT+2)]

There are very few cases in the system where spellchecker.dictionary is set, in fact, there are three:
https://dxr.mozilla.org/comm-central/search?q=set+spellchecker.dictionary&redirect=false&case=false

One here:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/composer/nsEditorSpellCheck.cpp#628
which is runs when the language is set via the right-click menu, but it doesn't execute for TB since it only runs
if (!(flags & nsIPlaintextEditor::eEditorMailMask)) {

The other two are here:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2070
in getValidSpellcheckerDictionary() and here
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2101
in dictionaryRemovalObserver.

If spellchecker.dictionary were set, it would reflect in the user interface, that is, new messages would show it in the language indicator. Also, it would show in Tools > Options, Composition, Spelling.

What is puzzling here is that none of this occurs, you can create new messages all you want or check the option and it always shows the language that was chosen. However, once you close the session, it looks like the preference is not saved.

Comment 2

[Jorg K (CEST = GMT+2)]

Damn, happens in dictionaryRemovalObserver at session shutdown time. Dictionary goes away, we set another one.

Comment 3

[Jorg K (CEST = GMT+2)]

OK, here is the problem. For each composition window we register an observer.

When opening a composition window and closing it again, the dictionaryRemovalObserver gets removed in gComposeRecyclingListener.

When opening two composition windows concurrently, gComposeRecyclingListener only gets called for the one that is closed first, that is, the one that will be recycled, but not the other one. One observer hangs around and causes damage when closing the session.

So basically, the way the observer is added/removed is flawed. This is a regression from bug 1203957.

Comment 4

[Jorg K (CEST = GMT+2)]

Who has a good idea how to fix this? I can see four possible solutions:

First:
Not set spellchecker.dictionary here. I only do it out of courtesy.
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2101

Second:
Somehow detect that this is called at session close time.

Third:
Remove the observer properly. The observer is a system wide thing which doesn't have anything to do with composition window. Every time we initialise a composition window, we create and observer for this window. If my humble JS knowledge is right, we use "closure" to carry knowledge of the document into the callback. When the windows is recycled, we remove the observer from the system which knows about this window. If we close the windows without running through the recycling code, as it happens for the first composition, the observer hangs around. So how can we remove it?

Forth:
Another option would be to register only one system wide observer that manages all composition windows. That brings up the question of finding all the composition windows (maybe using getWindowEnumerator). Also we would have to register the observer at startup and remove it at session close *before* dictionaries are removed and the notification fires.

You might want to take a look at the original patch which caused this: attachment 8668314 [details] [diff] [review].

I'm open to suggestions. I'll send some NI SPAM to a few people who might inspire me.

Comment 5

[[e-mail]]

onClose is used when recycling a compose window. It resets things that need to be temporarily disabled until the window gets recycled, so for instance it resets the recipients and attachments lists.

ComposeUnload resets anything that doesn't automatically go away when the window is closed without being recycled, including things such as removing the quit observer, however it doesn't need to reset the recipients and attachments lists as those things are going away anyway.

In this case you're adding something that only applies when the window is active, so you correctly remove it when you recycle the window, but as it happens, it doesn't go away anyway, so you also need to remove it when you unload the window.

Comment 6

[Jorg K (CEST = GMT+2)]

Great, thanks so much for the answer! It was well worth the short wait ;-)
(Maybe a new year's resolution for myself: be more patient.)
It saves me so much time poking around. I'll do as you said.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1)

OK, this is a little uglier than I had hoped.

Moving the removal of the observer from to gComposeRecyclingListener() to ComposeUnload() didn't work, since at shutdown time, the dictionaries are unloaded before the window. The debug I'm seeing is:

dictionaryRemovalObserver en-GB Write: (no subject)
dictionaryRemovalObserver en-GB Write: (no subject)
====================== Boing =================es-ES
dictionaryRemovalObserver es-ES Write: (no subject)
dictionaryRemovalObserver es-ES Write: (no subject)
====================== Boing =================en-US
================= Remove dictionaryRemovalObserver

So now I remove the observer in gComposeRecyclingListener() and ComposeUnload(). That runs into a problem when the recycled window is finally unloaded, since its observer was already removed. Hence the proposed solution.

If you agree, please r+, I promise to remove the dump statements before checkin ;-)

And a happy New Year 2016!

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8703157 [details] [diff] [review]
Proposed solution (v1)

Of course I'll remove the dump().

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 8703157 [details] [diff] [review]
Proposed solution (v1)

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

LGTM, r=mkmelin with the dumps remove

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a) - dump() removed

Carrying forward Magnus r+.

Thanks for the review, a very annoying bug this is.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8708035 [details] [diff] [review]
Proposed solution (v1a) - dump() removed

[Approval Request Comment]
Regression caused by (bug #): bug 1203957
User impact if declined: Very annoying losing the language setting.
Risk to taking this patch (and alternatives if risky):
Not really risky, was an oversight when implementing bug 1203957.

Comment 12

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/0e939ca875f17bc2e7dfd102b769d46b95f86bfc
Bug 1235205 - Add unregistering the dictionary removal observer to ComposeUnload(). r=mkmelin

Comment 13

[:aceman]

In some of my tests with smtp failures I noticed this code throws an exception when removing the observer. I'll file a bug when I find how to reproduce it.

Comment 14

[Jorg K (CEST = GMT+2)]

Which removal fails, the one in gComposeRecyclingListener or the one in ComposeUnload()?

Comment 15

[Jorg K (CEST = GMT+2)]

Aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/f7158a9ad377

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #13)
> In some of my tests with smtp failures I noticed this code throws an
> exception when removing the observer. I'll file a bug when I find how to
> reproduce it.
I'd be interested. I've tried various scenarios with "mail.compose.max_recycled_windows" set to 0, 1 or 2. Seems to work OK. On shutdown I only see this which is something else:
[976] WARNING: NS_ENSURE_TRUE(context) failed: file c:/mozilla-source/comm-central/mozilla/xpcom/threads/nsThread.cpp, line 794
[976] WARNING: 'NS_FAILED(RemovePermissionChangeObserver())', file c:/mozilla-source/comm-central/mozilla/dom/notification/Notification.cpp, line 676
[976] WARNING: some msg dbs left open: '!m_dbCache.Length()', file c:/mozilla-source/comm-central/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 84
db left open C:\Users\jorgk\AppData\Roaming\Thunderbird\Profiles\testing.testing\Mail\Local Folders\Unsent Messages.msf

What do you mean by "throws an exception"? A crash? A dump on the console? When does that happen? When you send and the composition window closes? Or when sending (artificially) fails? Or at shutdown?

I landed this on Aurora and I will start using Earlybird now to see whether I experience any problems.

Comment 17

[:aceman]

Yes, I artifically injected SMTP send failures to debug the other bug. Once in a while I noticed the JS error in the Error console (on multiple occurrences). It clearly pointed to code line that looked like the code in this patch. If there are 2 blocks similar to the one in this patch, then I am not yet sure which one it was. That is why I need to reproduce it again.

Comment 18

[:aceman]

I can see the error when opening the compose window and then quickly closing it. It does not happen always, but maybe half of the times.

Comment 19

[Jorg K (CEST = GMT+2)]

I've spotted the error browsing some logs. Let's see what I can do. To be continued in bug 1246517.