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

RESOLVED FIXED in Thunderbird 25.0

Status

Thunderbird
Message Compose Window
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: adw, Unassigned)

Tracking

({dogfood, regression})

24 Branch
Thunderbird 25.0
dogfood, regression
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird24+ fixed)

Details

(Whiteboard: [workaround: disable spell checking in preferences if the exception in compose window causes problems])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
Severity: normal → major
tracking-thunderbird24: --- → ?

Updated

4 years ago
Blocks: 856270
Keywords: regression
Version: unspecified → 24

Comment 1

4 years ago
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

4 years ago
(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.
tracking-thunderbird24: ? → +

Updated

4 years ago
Summary: Recent changes making spell check more async may have broken spell check in Thunderbird → Recent changes making spell check more async (Bug 856270) broke spell check in Thunderbird
Blocks: 881222

Updated

4 years ago
Keywords: dogfood
Blocks: 881588

Updated

4 years ago
Duplicate of this bug: 881349

Updated

4 years ago
Whiteboard: [workaround: disable spell checking in preferences if the exception in compose window causes problems]
Created attachment 770691 [details] [diff] [review]
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.
Attachment #770691 - Flags: review?(neil)

Comment 5

4 years ago
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

4 years ago
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.
Attachment #770691 - Flags: review?(neil) → feedback+

Comment 7

4 years ago
(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!]
Created attachment 771362 [details] [diff] [review]
[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).
Attachment #770691 - Attachment is obsolete: true
Attachment #771362 - Flags: review?(neil)

Comment 9

4 years ago
(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 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?)
Attachment #771362 - Flags: review?(neil) → review+
Comment on attachment 771362 [details] [diff] [review]
[checked in] Fix spell check dialog v2

https://hg.mozilla.org/comm-central/rev/1a4332221534
(In reply to neil@parkwaycc.co.uk 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 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
Attachment #771362 - Flags: approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/34152b9627ba
Created attachment 772543 [details] [diff] [review]
[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.
Attachment #772543 - Flags: review?(mkmelin+mozilla)
Attachment #772543 - Flags: review?(mconley)
Attachment #772543 - Flags: review?(bwinton)
Attachment #772543 - Attachment is patch: true
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…
Attachment #772543 - Flags: review?(bwinton) → review+
Attachment #772543 - Flags: review?(mkmelin+mozilla)
Attachment #772543 - Flags: review?(mconley)
(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.
Attachment #771362 - Attachment description: Fix spell check dialog v2 → [checked in] Fix spell check dialog v2
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
Attachment #772543 - Attachment description: Fix first-time compose window and broken mozmill tests → [checked in] Fix first-time compose window and broken mozmill tests
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
Attachment #772543 - Flags: approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/7f804e500286
Created attachment 773324 [details] [diff] [review]
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.
Attachment #773324 - Flags: review?(mbanner)

Comment 22

4 years ago
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 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.
Attachment #773324 - Flags: review?(mbanner)
Attachment #773324 - Flags: review+
Attachment #773324 - Flags: approval-comm-aurora+
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
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-thunderbird24: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0

Comment 26

4 years ago
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

4 years ago
(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
You need to log in before you can comment on or make changes to this bug.