887010 - InlineSpellChecker.addDictionaryListToMenu fails if called straight after InlineSpellChecker.enabled is set to true (and no async work around)

Status: RESOLVED WORKSFORME

(Core :: Spelling checker, defect)

Description

[Mark Banner (:standard8)]

In the Thunderbird compose window, we have been doing this:

  gSpellChecker = new InlineSpellChecker();
...
  gSpellChecker.enabled = aEnableInlineSpellCheck;
  gSpellChecker.addDictionaryListToMenu(...);

http://hg.mozilla.org/comm-central/annotate/bfe4e360ee05/mail/components/compose/content/MsgComposeCommands.js#l4805 (and lines there around).

This now fails with:

JavaScript error: resource://gre/modules/InlineSpellChecker.jsm, line 159: spellchecker is null

The relevant call flow of InlineSpellChecker.enabled is:

-> this.mEditor.setSpellcheckUserOverride
--> SyncRealTimeSpell
---> mInlineSpellChecker->SetEnableRealTimeSpell
----> mPendingSpellCheck->InitSpellChecker

This final method is an async call, but this is not reflected back up the stack.

Due to this not being reflected, the addDictionaryListToMenu call tries to access mInlineSpellChecker.spellChecker which is undefined.

Hence I can't see a way to get the spell checking working for us in a sane manner without fixing the APIs or a timer that we don't really want, and we need this for Gecko 24.

Comment 1

[Drew Willcoxon :adw]

I said I updated all the mozilla-central consumers, but apparently I missed this InlineSpellChecker.jsm thing.  Let me think about how to fix this.

Comment 2

[Drew Willcoxon :adw]

There's no choice but to make some InlineSpellChecker.jsm functions async, I think.

I wish it were as easy as ensuring that mozInlineSpellChecker's mSpellCheck is never null so consumers like InlineSpellChecker could use it straight away.  As it is, mSpellCheck (the nsEditorSpellCheck) is null while it's initializing, and during this time mPendingSpellCheck is nonnull; when that init finishes, mPendingSpellCheck is assigned to mSpellCheck, and mSpellCheck becomes nonnull.  So basically I'd like to extend the fix to bug 882879 so that mPendingSpellCheck always stands in for mSpellCheck.

But the problem with that is that during init, the nsEditorSpellCheck won't have a dictionary, which has a couple of consequences:

(1) The spell check object will be useless, so while consumers may safely synchronously use it during this time, they'll likely need to wait until it becomes useable anyway, and

(2) actually the object is not so safe during this time, because the spell check code in certain places internally assumes that spell check objects have dictionaries and returns errors if that's not true, which can bubble up to methods called by consumers (e.g., nsEditorSpellCheck::CheckCurrentWord -> mozSpellChecker::CheckWord -> return NS_ERROR_NULL_POINTER if !mSpellCheckingEngine).

Comment 3

[Drew Willcoxon :adw]

Attachment: asyncify InlineSpellChecker

This makes InlineSpellChecker() and init() async and take optional callbacks.  It also replaces the enabled getter and setter with enable() and disable().  enable() takes an optional callback.

Mark, I think you'll want to pass in a callback when you init() gSpellChecker and do the remainder of InitEditor in that callback.  Until the callback is called, canSpellCheck may be false (I'm not sure if it will be in your case, maybe not!), so the UI should reflect that during that time.  Can you see if this works for you?

Comment 4

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #3)
> It also replaces the enabled getter and setter

Setter only.

Comment 5

[Mark Banner (:standard8)]

This definitely looks good for the gSpellChecker.init case, and providing a callback. It seems to work fine there (I've patched it locally for TB). I've a little bit more experimentation to do tomorrow, but I don't expect any further issues with this patch.


Is this something we'd be able to get onto Aurora, or would we need a work-around there?

Comment 6

[[e-mail]]

Comment on attachment 769264 [details] [diff] [review]
asyncify InlineSpellChecker

>+  // Initializes the spell checker for the given editor.  The callback, if
>+  // given, is called when the spell checker's initialization completes.
getInlineSpellChecker appears to be synchronous; it attempts to create the object, but doesn't actually turn on spellchecking.

>+    // Wait for initialization.
>+    let editor = this.mEditor;
>+    let onSpellCheckEnded = function(subj) {
>+      if (subj == editor) {
>+        Services.obs.removeObserver(onSpellCheckEnded, SPELL_CHECK_ENDED_TOPIC);
>+        callback();
>+      }
>+    }.bind(this);
You don't need to use both let editor = this.mEditor and bind(this).

>+    return !this.mInlineSpellChecker ? false :
We write !x ? false : y as x && y
>+           this.mInlineSpellChecker.spellChecker ? true :
And x ? true : y as x || y
>+           !this.mInlineSpellChecker.spellCheckPending;
(Don't forget that && has higher precedence than ||)

Comment 7

[Drew Willcoxon :adw]

Attachment: asyncify InlineSpellChecker 2

Neil, your comments prompted me to think about my patch again, and I think it's wrong in a couple of places.

First, getInlineSpellChecker doesn't actually trigger any async init within mozInlineSpellChecker, so InlineSpellChecker() and init() don't need to take callbacks.  Async init is triggered by enabling the inline spell checker (via InlineSpellChecker.enable(), focusing a spell-checked html:textarea, etc.), so enable() is the only method that needs a callback.

Second, I'm changing the definition of canSpellCheck, but I think it's enabled's I should change.  canSpellCheck is used to determine whether there are any dictionaries available.  That determination is still synchronously made when init() attempts to create mInlineSpellChecker by calling getInlineSpellChecker(true).

Comment 8

[Mark Banner (:standard8)]

Attachment: TB not fully working fix

I've been trying this patch with the core patch. Almost everything seems to be working fine for fresh compose windows.

However, if I use a recycled compose window (one which effectively gets hidden rather than re-created) then I can't get spell check to work at all.

The enable callback is still called, and toggling the check spelling seems to work, but the actual spell-check doesn't happen.

Comment 9

[[e-mail]]

(In reply to Mark Banner)
> In the Thunderbird compose window, we have been doing this:
> 
>   gSpellChecker = new InlineSpellChecker();
> ...
>   gSpellChecker.enabled = aEnableInlineSpellCheck;
>   gSpellChecker.addDictionaryListToMenu(...);

I just noticed that SeaMonkey doesn't bother adding its dictionary list to the menu synchronously. Instead:
* the toolbarbutton popupshowing calls OnShowDictionaryMenu which calls InitLanguageMenu
* the context menu popupshowing calls a function which calls addDictionaryListToMenu

So would it work for you to just remove your other two calls from your code? (Note that Thunderbird ported SeaMonkey's additional call of InitLanguageMenu in bug 355064 but SeaMonkey then removed again it in order to fix bug 370891; Thunderbird must have found a different fix.)

Comment 10

[Mark Banner (:standard8)]

(In reply to [e-mail] from comment #9)
> So would it work for you to just remove your other two calls from your code?
> (Note that Thunderbird ported SeaMonkey's additional call of
> InitLanguageMenu in bug 355064 but SeaMonkey then removed again it in order
> to fix bug 370891; Thunderbird must have found a different fix.)

I've tried that (see attachment 772543 [details] [diff] [review]) and whilst it still works for all the menu options, in the cached composed window we're still not getting the red line underscores that we should do. This is strange, as SM trunk seems to be working fine. My guess is there's something else that TB is doing wrong, but I can't find it at the moment.

Comment 11

[[e-mail]]

(In reply to Mark Banner from comment #10)
> it still works for all the menu options, in the cached composed window we're
> still not getting the red line underscores that we should do. This is
> strange, as SM trunk seems to be working fine. My guess is there's something
> else that TB is doing wrong, but I can't find it at the moment.

Actually in a build from last week I'm not getting the spellcheck wavy lines either. I started doing some debugging and found that it was stuck waiting for a full spellcheck to complete. I'm not sure what triggered the full spellcheck or why it didn't complete though.

Comment 12

[[e-mail]]

OK, so for the wavy lines:

Editor likes to synchronise the spell check state a lot. I'm not 100% sure why, but you can get it to turn spell checking on and off in quick succession.

Turning spell checking on when it is already on triggers a full spell check. I don't know why that is either.

Unfortunately when spell checking is turned off when a full spell check is pending, the flag never gets cleared.

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 770552 [details] [diff] [review]
asyncify InlineSpellChecker 2

Neil, could you please review this patch?

Comment 14

[[e-mail]]

Comment on attachment 770552 [details] [diff] [review]
asyncify InlineSpellChecker 2

I want to cancel this review because it's no longer helpful for any of the outstanding spellchecking issues.
The original issue of being unable to get the dictionary list immediately after turning on spell checking was handled in bug 880595 by not getting the dictionary list until the dictionary UI was accessed.
There's an issue with being unable to turn inline spellchecking off and on again which is covered by bug 891904 and also a workaround patch in bug 880595.

Comment 15

[[e-mail]]

And then there's also an issue with being unable to ignore words immediately after turning on spell checking. The problem here is that when we turn on spell checking we don't know if we are going to want to ignore words, and when we want to ignore words we don't know if we have immediately turned on spell checking. Is there an API that says "invoke X(Y) if spell checking is enabled, but if it's pending then invoke it when it's finished enabling, but if it's disabled then don't bother calling it at all", or something we can leverage to achieve this?

Comment 16

[Drew Willcoxon :adw]

No, but with this patch, consumers can keep track of that themselves by passing a callback to enable().

Comment 17

[[e-mail]]

(In reply to Drew Willcoxon from comment #16)
> No, but with this patch, consumers can keep track of that themselves by
> passing a callback to enable().

So we're supposed to keep independent track of whether spell checking is pending?

Comment 18

[Drew Willcoxon :adw]

There's also nsIInlineSpellChecker.spellCheckPending.  Does that help you?

Comment 19

[[e-mail]]

(In reply to Drew Willcoxon from comment #18)
> There's also nsIInlineSpellChecker.spellCheckPending.  Does that help you?

Yes, I think that helped us fix our last problem, thanks.

Comment 20

[Drew Willcoxon :adw]

Great, glad to hear it.  What's the status of this bug now?  I think this patch is the right thing to do in light of async spell check, even if Firefox and Thunderbird are using InlineSpellChecker.jsm in such a way that they don't currently run into any problems.

If you still want to decline the patch, then let's close the bug.

Comment 21

[[e-mail]]

I don't really want to go with the patch as is because of having to fix up all the consumers at the same time, none of which actually needed to know when spellchecking was enabled at the point that they enabled it.

What might be useful is one of two APIs.

The first idea is to expose some of the inner spell check APIs, but with a check to defer them if the spell check is pending. So we could write

gSpellCheck.ignoreWords(tokenizedNames, tokenizedNames.length);

and if spellchecking was pending then the inline spell checker module would ensure that the words get ignored once spell check was ready.

The other option is to make the pending check more explicit using a callback, like this:

function addRecipientsToIgnoreList(aAddressesToAdd)
{
  gSpellChecker.whenEnabled(function() {
    /* ... */
    gSpellChecker.mInlineSpellChecker.ignoreWords(tokenizedNames, tokenizedNames.length);
  });
}

whenEnabled would do nothing when the spellchecker was disabled, and would wait if it was pending, and would presumably execute soon if it was enabled (although you then run into the potential issue of the spellchecker getting disabled for some reason between the call and the dispatch).