The default bug view has changed. See this FAQ.

Selected spellcheck language setting lost under some circumstances

RESOLVED FIXED in Thunderbird 46.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
a year ago
a year ago

People

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

Tracking

({regression})

Trunk
Thunderbird 46.0
regression

Thunderbird Tracking Flags

(thunderbird45 fixed, thunderbird46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
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.
(Assignee)

Comment 2

a year ago
Damn, happens in dictionaryRemovalObserver at session shutdown time. Dictionary goes away, we set another one.
(Assignee)

Comment 3

a year ago
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.
Keywords: regression
(Assignee)

Comment 4

a year ago
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.
Flags: needinfo?(rkent)
Flags: needinfo?(neil)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
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.
Flags: needinfo?(neil)
(Assignee)

Comment 6

a year ago
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.
Flags: needinfo?(rkent)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
(Assignee)

Comment 7

a year ago
Created attachment 8703157 [details] [diff] [review]
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!
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8703157 - Flags: feedback?(neil)
(Assignee)

Comment 8

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

Of course I'll remove the dump().
Attachment #8703157 - Flags: review?(mkmelin+mozilla)

Comment 9

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

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

LGTM, r=mkmelin with the dumps remove
Attachment #8703157 - Flags: review?(mkmelin+mozilla)
Attachment #8703157 - Flags: review+
Attachment #8703157 - Flags: feedback?(neil)
(Assignee)

Comment 10

a year ago
Created attachment 8708035 [details] [diff] [review]
Proposed solution (v1a) - dump() removed

Carrying forward Magnus r+.

Thanks for the review, a very annoying bug this is.
Attachment #8703157 - Attachment is obsolete: true
Attachment #8708035 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 11

a year ago
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.
Attachment #8708035 - Flags: approval-comm-aurora?

Comment 12

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

Updated

a year ago
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 46.0

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Attachment #8708035 - Flags: approval-comm-aurora? → approval-comm-aurora+

Comment 13

a year ago
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.
(Assignee)

Comment 14

a year ago
Which removal fails, the one in gComposeRecyclingListener or the one in ComposeUnload()?
(Assignee)

Comment 15

a year ago
Aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/f7158a9ad377
status-thunderbird45: --- → fixed
status-thunderbird46: --- → fixed
(Assignee)

Comment 16

a year ago
(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

a year ago
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

a year ago
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.
(Assignee)

Comment 19

a year ago
I've spotted the error browsing some logs. Let's see what I can do. To be continued in bug 1246517.
(Assignee)

Updated

a year ago
Depends on: 1246517
You need to log in before you can comment on or make changes to this bug.