880595 - Recent changes making spell check more async (Bug 856270) broke spell check in Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Drew Willcoxon :adw]

Bug 856270 recently landed and made spell checking more async.  nsEditorSpellCheck used to rely on nsIContentPrefService to store dictionary preferences, but nsIContentPrefService uses I/O on the main thread, which we want to stop.  Bug 856270 switched to nsIContentPrefService2, which does I/O off the main thread and therefore is async.

As a result, nsIEditorSpellCheck.InitSpellChecker and UpdateCurrentDictionary now take callbacks.  The spell checker isn't initialized or updated until they're called.  Bug 856270 updated all the nsEditorSpellCheck consumers in mozilla-central, including mozInlineSpellChecker, so if Thunderbird also relies on those consumers being sync, it'll need to be updated for them, too.

A particular PITA consequence is that when you turn on spell checking in an editor, e.g., by focusing an editor element, spell check may not happen on the next turn of the event loop anymore.  nsEditorSpellCheck may need to look up a dictionary in nsIContentPrefService2, which takes an indefinite amount of time.  To help in those situations, you can use onSpellCheck in AsyncSpellCheckTestHelper.jsm (resource://gre/modules/AsyncSpellCheckTestHelper.jsm):

http://mxr.mozilla.org/mozilla-central/source/editor/AsyncSpellCheckTestHelper.jsm

I'm probably omitting things so please don't hesitate to ask questions.  And I'm sorry for not bringing this to your attention earlier.

Comment 1

[Hartmut Figge]

i have just noticed the failing of spell check in my SM-Trunk on Linux x86_64 and determined the regression range to

Last good: 2013-06-05 15:24:00 PDT
First bad: 2013-06-06 15:14:00 PDT

Backing out some checkins showed that either 8f48c49425ab or eac7a8c8d5d4 of Bug 856270 is the culprit.

Comment 2

[Scoobidiver (away)]

(In reply to Hartmut Figge from comment #1)
> Backing out some checkins showed that either 8f48c49425ab or eac7a8c8d5d4 of
> Bug 856270 is the culprit.
See the Blocks field.

Comment 4

[Mark Banner (:standard8)]

Attachment: Fix spell check dialog

This patches fixes the spell check dialog in the compose window by taking account of the new async init.

It does not fix the inline spell check or any other issues within the actual compose window which will need the fix for bug 887010.

Comment 5

[John L. Galt]

Using Daily (Win x64 24.0a1 (2013-06-21)) build, and having spell checker set to auto check when sending email, I've found that there is no spell check performed (resulting in a no spelling error notification) until I press re-check page.  Is this symptom that I'm seeing related to this particular bug filing?

Comment 6

[[e-mail]]

Comment on attachment 770691 [details] [diff] [review]
Fix spell check dialog

>+}
> 
>+function spellCheckStarted() {
This gets the dialog working, but unfortunately the dialog now opens at the wrong size because its elements haven't been populated in time.

>+var gRecheckLanguage;
>+
>+function finishRecheck() {
>+  gSpellChecker.SetCurrentDictionary(gRecheckLanguage);
>+  gMisspelledWord = gSpellChecker.GetNextMisspelledWord();
>+  SetWidgetsForMisspelledWord();
>+}
>+
> function Recheck()
> {
>   //TODO: Should we bother to add a "Recheck" method to interface?
>   try {
>-    var curLang = gSpellChecker.GetCurrentDictionary();
>+    gRecheckLanguage = gSpellChecker.GetCurrentDictionary();
>     gSpellChecker.UninitSpellChecker();
>-    gSpellChecker.InitSpellChecker(GetCurrentEditor(), false);
>-    gSpellChecker.SetCurrentDictionary(curLang);
>-    gMisspelledWord = gSpellChecker.GetNextMisspelledWord();
>-    SetWidgetsForMisspelledWord();
>+    gSpellChecker.InitSpellChecker(GetCurrentEditor(), false, finishRecheck);
Slightly less ugly than a global variable would be a local anonymous function that finishes the recheck.

Comment 7

[[e-mail]]

(In reply to comment #6)
> This gets the dialog working, but unfortunately the dialog now opens at the
> wrong size because its elements haven't been populated in time.
[Extra brownie points if you can fix the warning in the error console!]

Comment 8

[Mark Banner (:standard8)]

Attachment: [checked in] Fix spell check dialog v2

This might fix it. I can't see anything else to move that would help the dialog be formatted correctly sooner, I can't actually reproduce that either here, so maybe this is platform specific somehow (and I don't have a different platform to try on either).

The console warning, I'm assuming is the strict one in EdDialogCommon.js, if so, that's fixed by a different bug (patch waiting to land).

Comment 9

[[e-mail]]

(In reply to Mark Banner from comment #8)
> This might fix it. I can't see anything else to move that would help the
> dialog be formatted correctly sooner, I can't actually reproduce that either
> here, so maybe this is platform specific somehow (and I don't have a
> different platform to try on either).
Could be; the problem was obvious on Windows with SeaMonkey Modern theme, and slightly less obvious in the default theme.

> The console warning, I'm assuming is the strict one in EdDialogCommon.js, if
> so, that's fixed by a different bug (patch waiting to land).
(No, it's the getElementById warning that I was seeing.)

Comment 10

[[e-mail]]

Comment on attachment 771362 [details] [diff] [review]
[checked in] Fix spell check dialog v2

No, that doesn't help, so stick with the previous version (except for the Recheck function change of course).

>   } catch(ex) {
>-    dump(ex);
>+    console.log(ex);
I'd prefer Components.reportException(ex); (does console.log actually go anywhere useful in Thunderbird?)

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 771362 [details] [diff] [review]
[checked in] Fix spell check dialog v2

https://hg.mozilla.org/comm-central/rev/1a4332221534

Comment 12

[Mark Banner (:standard8)]

(In reply to [e-mail] from comment #9)
> > The console warning, I'm assuming is the strict one in EdDialogCommon.js, if
> > so, that's fixed by a different bug (patch waiting to land).
> (No, it's the getElementById warning that I was seeing.)

I don't see that on Thunderbird.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 771362 [details] [diff] [review]
[checked in] Fix spell check dialog v2

[Triage Comment]
We want this to fix bustage on aurora as well

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/34152b9627ba

Comment 15

[Mark Banner (:standard8)]

Attachment: [checked in] Fix first-time compose window and broken mozmill tests

This removes unnecessary calls which are attempting to initialise menu options when we don't need to - there's a function to initialise the menu options when we open the menu itself. This is as Neil suggested in bug 887010 comment 9.

This gets us enough such that:

- inline spell check works in non-cached compose windows
- mozmill bustages about attachment reminders are fixed
- a number of other issues seen with cached compose windows are fixed.

It doesn't fix inline spell check in cached compose windows yet. There seems to be something else going on that prevents that from working, but I haven't worked out what yet, in the meantime this would be a big step forward, and I think removing those calls is fine.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 772543 [details] [diff] [review]
[checked in] Fix first-time compose window and broken mozmill tests

As far as I can tell, it seems good to me.  I'm trusting there's no effect on the /suite/ code from this change…

Comment 17

[Mark Banner (:standard8)]

(In reply to Blake Winton (:bwinton) from comment #16)
> As far as I can tell, it seems good to me.  I'm trusting there's no effect
> on the /suite/ code from this change…

Yep, no effect, this is a /mail/ only file.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 772543 [details] [diff] [review]
[checked in] Fix first-time compose window and broken mozmill tests

https://hg.mozilla.org/comm-central/rev/b2a4c19e5f04

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 772543 [details] [diff] [review]
[checked in] Fix first-time compose window and broken mozmill tests

[Triage Comment]
Taking onto aurora as well

Comment 20

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/7f804e500286

Comment 21

[[e-mail]]

Attachment: Workaround for recycling windows

The editor and inline spellchecker conspire to wind up in a confused state if you disable the editor before turning off the inline spellchecker.

Comment 22

[Joe Sabash [:JoeS1]]

Hmm, tried your fix locally and it doesn't seem to completely solve the problem
with mail.compose.max_recycled_windows=1

The first compose window; inline spellcheck seems to work fine
The next compose window shows no squigglies at all

If I change compose modes to plaintext then compose next message in html, it works. 
But subsequent html compositions show no inline spelling errors

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 773324 [details] [diff] [review]
Workaround for recycling windows

[Triage Comment]
Looks reasonable, a=me for landing on the CLOSED TREE and on aurora as I think we definitely want this under testing asap.

Comment 24

[[e-mail]]

Comment on attachment 773324 [details] [diff] [review]
Workaround for recycling windows

http://hg.mozilla.org/comm-central/rev/351c48846abe
http://hg.mozilla.org/releases/comm-aurora/rev/4748f12ac8a1

Comment 25

[Mark Banner (:standard8)]

AFAIK we fixed all the issues late in the 24 Earlybird cycle. If there's any more outstanding please file new bugs and request tracking-thunderbird24.

Comment 26

[Rubin110]

Running Daily, 27.0a1 (2013-09-18) on Debian Sid. Occasionally reply composition windows will not show the red underline for misspelled words. Haven't figured out a good way to reproduce it yet as it feels a bit random. Anyone else seeing this?

Comment 27

[Joe Sabash [:JoeS1]]

(In reply to Rubin110 from comment #26)
> Running Daily, 27.0a1 (2013-09-18) on Debian Sid. Occasionally reply
> composition windows will not show the red underline for misspelled words.
> Haven't figured out a good way to reproduce it yet as it feels a bit random.
> Anyone else seeing this?

See bug 917027
There is a simple work-around, just set the pref "mail.compose.max_recycled_windows" to 0