Last Comment Bug 880595 - Recent changes making spell check more async (Bug 856270) broke spell check in Thunderbird
: Recent changes making spell check more async (Bug 856270) broke spell check i...
Status: RESOLVED FIXED
[workaround: disable spell checking i...
: dogfood, regression
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: 24 Branch
: All All
: -- major with 1 vote (vote)
: Thunderbird 25.0
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 881349 (view as bug list)
Depends on:
Blocks: 856270 881222 881588
  Show dependency treegraph
 
Reported: 2013-06-06 23:45 PDT by Drew Willcoxon :adw
Modified: 2013-09-18 20:08 PDT (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed


Attachments
Fix spell check dialog (2.60 KB, patch)
2013-07-03 02:21 PDT, Mark Banner (:standard8)
neil: feedback+
Details | Diff | Splinter Review
[checked in] Fix spell check dialog v2 (4.04 KB, patch)
2013-07-04 07:47 PDT, Mark Banner (:standard8)
neil: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review
[checked in] Fix first-time compose window and broken mozmill tests (1.03 KB, patch)
2013-07-09 01:03 PDT, Mark Banner (:standard8)
bwinton: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review
Workaround for recycling windows (3.95 KB, patch)
2013-07-10 08:30 PDT, neil@parkwaycc.co.uk
standard8: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Drew Willcoxon :adw 2013-06-06 23:45:02 PDT
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 2013-06-08 12:41:47 PDT
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) 2013-06-08 13:54:12 PDT
(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 3 :aceman 2013-06-17 00:41:54 PDT
*** Bug 881349 has been marked as a duplicate of this bug. ***
Comment 4 Mark Banner (:standard8) 2013-07-03 02:21:25 PDT
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.
Comment 5 John L. Galt 2013-07-03 12:48:53 PDT
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 neil@parkwaycc.co.uk 2013-07-03 14:23:46 PDT
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 neil@parkwaycc.co.uk 2013-07-03 14:32:24 PDT
(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) 2013-07-04 07:47:06 PDT
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).
Comment 9 neil@parkwaycc.co.uk 2013-07-04 08:53:24 PDT
(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 neil@parkwaycc.co.uk 2013-07-04 08:57:02 PDT
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) 2013-07-04 14:54:28 PDT
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) 2013-07-04 14:56:31 PDT
(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 13 Mark Banner (:standard8) 2013-07-08 00:15:24 PDT
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) 2013-07-08 00:20:27 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/34152b9627ba
Comment 15 Mark Banner (:standard8) 2013-07-09 01:03:30 PDT
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.
Comment 16 Blake Winton (:bwinton) (:☕️) 2013-07-09 05:24:48 PDT
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) 2013-07-09 05:47:09 PDT
(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) 2013-07-09 05:50:19 PDT
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) 2013-07-09 09:24:57 PDT
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) 2013-07-09 09:25:57 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/7f804e500286
Comment 21 neil@parkwaycc.co.uk 2013-07-10 08:30:25 PDT
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.
Comment 22 Joe Sabash [:JoeS1] 2013-07-19 16:29:30 PDT
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) 2013-07-25 08:40:55 PDT
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 25 Mark Banner (:standard8) 2013-08-12 02:56:55 PDT
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 2013-09-18 17:30:48 PDT
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] 2013-09-18 20:08:22 PDT
(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

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