856270 - Update nsEditorSpellCheck to use nsIContentPrefService2

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Drew Willcoxon :adw]

LastDictionary::FetchLastDictionary gets a content pref and needs to be updated.  It's a little tricky.  I think nsIEditorSpellCheck.UpdateCurrentDictionary is going to have to become async and take a callback: mozInlineSpellChecker::UpdateCurrentDictionary calls nsIEditorSpellCheck.UpdateCurrentDictionary and then immediately calls nsIEditorSpellCheck.GetCurrentDictionary, expecting the Update call to have synchronously set the current dictionary by calling SetCurrentDictionary.  But Update can no longer synchronously call Set, because before calling Set, it calls LastDictionary::FetchLastDictionary, which must now be async...

LastDictionary::StoreCurrentDictionary and ClearCurrentDictionary also use content prefs, but it doesn't look like updating those will be as hard.

Comment 1

[Drew Willcoxon :adw]

Attachment: WIP

This changes nsIEditorSpellCheck.UpdateCurrentDictionary to be async and take a callback, since its implementation calls the content prefs service, and there are callers that expect it to be sync.  I think I'm going to have to also broadcast a notification when the update is done, because it ends up getting called when you focus an nsIEditor (like an html:textarea), and at least one test focuses a textarea and then expects Update to have been called.

I haven't updated the other two methods yet.

Comment 2

[Drew Willcoxon :adw]

Attachment: patch

Ehsan, you reviewed the bug that added content prefs to nsEditorSpellCheck (bug 678842), so would you mind reviewing this, too?  It updates nsEditorSpellCheck to use the new async content prefs service.  I had to change some idl as a result.

I also had to update a couple of tests to wait for the now async nsEditorSpellCheck::UpdateCurrentDictionary to complete.  They poll for dictionary changes.  Pretty cheesy.  At first I broadcasted an observer notification when the Update completes.  The problem with that is that Update ends up getting called in a variety of non-obvious situations.  The tests must know exactly which of their calls end up triggering an Update at some point in the stack, and then they must count the Updates, waiting for the right number to have passed.  And it turns out that the number of Updates in a test depends on whether the test runs alone or after another test in the suite.  After all of that, I thought that polling is much less brittle.

Comment 3

[(no longer active)]

Comment on attachment 736174 [details] [diff] [review]
patch

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

r=me, but note that I'm just trusting you on the nsIContentPrefsService2 changes.  You might want to get somebody else's review on those bits.

::: editor/composer/src/nsEditorSpellCheck.cpp
@@ +110,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  DictionaryFetcher(nsEditorSpellCheck* aSpellCheck)

Nit: please make this explicit.

@@ +113,5 @@
> +
> +  DictionaryFetcher(nsEditorSpellCheck* aSpellCheck)
> +    : mSpellCheck(aSpellCheck) {}
> +
> +  ~DictionaryFetcher() {}

Drop the dtor.

@@ +117,5 @@
> +  ~DictionaryFetcher() {}
> +
> +  NS_IMETHOD Fetch(nsIEditor* aEditor);
> +
> +  NS_IMETHODIMP HandleResult(nsIContentPref* aPref)

Nit: NS_IMETHOD. (same for HandleCompletion and HandleError.)

@@ +367,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // do not fail if UpdateCurrentDictionary fails because this method may
>    // succeed later.
> +  (void)UpdateCurrentDictionary(nullptr);

No need for the void cast here and elsewhere.

@@ +640,5 @@
>  
> +class UpdateDictionaryCallbackCaller
> +{
> +public:
> +  UpdateDictionaryCallbackCaller(nsIEditorSpellCheckCallback* aCallback)

explicit.

::: editor/idl/nsIEditorSpellCheck.idl
@@ +168,5 @@
> +
> +[scriptable, function, uuid(5f0a4bab-8538-4074-89d3-2f0e866a1c0b)]
> +interface nsIEditorSpellCheckCallback : nsISupports
> +{
> +  void editorSpellCheckCallDone();

Hmm, this could probably use a better name, such as dictionaryUpdated.

Comment 4

[Drew Willcoxon :adw]

Thanks Ehsan.  Since I wrote nsIContentPrefsService2, I'm confident I'm using it correctly, and I don't think I need an additional review there.

Comment 5

[Drew Willcoxon :adw]

Attachment: patch 2

Benjamin, would you mind super-reviewing?  This changes nsIEditorSpellCheck's API, and also nsEditorSpellCheck's and mozInlineSpellChecker's slightly.

Comment 6

[(no longer active)]

(In reply to comment #4)
> Thanks Ehsan.  Since I wrote nsIContentPrefsService2, I'm confident I'm using
> it correctly, and I don't think I need an additional review there.

Sounds good to me.  :-)

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 736525 [details] [diff] [review]
patch 2

Tryserver had several errors, so cancelling super-review while I investigate.  Looks like there are other tests that need to accommodate the new asynchronicity.

https://tbpl.mozilla.org/?tree=Try&rev=2ceb97729285

Comment 8

[Drew Willcoxon :adw]

This is proving to be really annoying.

Comment 9

[Drew Willcoxon :adw]

Attachment: WIP 4

https://tbpl.mozilla.org/?tree=Try&rev=d4ecb1958f08

I think all non-reftests are passing now...

I talked with Ehsan about polling for async state.  I mentioned that polling in reftests is not 100% reliable because there's no condition I can test (since reftests don't have chrome privileges).  Afterward I realized that's not the point: polling isn't reliable because the event you're polling for may happen after you've stopped polling, regardless of what your condition is.  So I added nsIInlineSpellChecker.onSpellCheck, a callback that's called when spell checking is done.  All the tests use this to determine when it's safe to do their checks.

Next I'll look into modifying the reftest driver to hook into onSpellCheck (bug 863430) so I can fix the failing reftests: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js#234

Comment 10

[Drew Willcoxon :adw]

Stopped and restarted try due to a mistake:

https://tbpl.mozilla.org/?tree=Try&rev=fa03c3f65d86

Comment 11

[Drew Willcoxon :adw]

Attachment: WIP 5

The last try run showed more non-reftest failures, so I tried to fix them and pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=c604b8aa7aa8

So I haven't had a chance to work on the reftests yet.

Comment 12

[Drew Willcoxon :adw]

Try with reftest fixes:
https://tbpl.mozilla.org/?tree=Try&rev=9c68efbef6d6

I think all tests should pass now, but we'll see!

This patch is sort of in a state of flux because it uses two different methods of trapping spell checks.  The first is the aforementioned nsIInlineSpellChecker.onSpellCheck.  The second uses an nsIObserverService notification, but it also checks whether the nsIEditor has an inline spell checker with real-time spell check enabled, because if not, then no spell check is expected.  Reftests use the second method, and I need to convert all the mochitests to use it, too.

Comment 13

[Drew Willcoxon :adw]

Attachment: WIP 6

Comment 14

[Drew Willcoxon :adw]

I've been working on this for a solid week and it just keeps getting worse and worse.  I change one thing and something else fails.  I fix that but the fix uncovers yet another thing.  The spell check code is messed up.  A couple of the reftests pass now that should not actually pass: they depend on bad behavior that happens to allow them to pass in ways that do not actually test what they are trying to test.  When I fix the code to make one of them pass the way it should, it actually fails because one of the squiggly lines doesn't quite match the corresponding squiggly line in the ref.  I'm trying to fix the code to make the other pass but it's just a disaster.  When you have a contenteditable div, everybody thinks the element being edited is the <body> of the doc for some reason, so nobody can find the lang attribute on the div, so the appropriate dictionary is not used.

Comment 15

[(no longer active)]

I would be happy to try to answer any questions that you might have, but I need more concrete questions.  :-)

(Note that I agree that the spellchecker code is a mess!)

Comment 16

[Drew Willcoxon :adw]

https://tbpl.mozilla.org/?tree=Try&rev=d4d88fe0293c

Comment 17

[Drew Willcoxon :adw]

Attachment: WIP 7

Think this might fix the problem I was having in the previous comment.  I went back to using nsIInlineSpellChecker.onSpellCheck rather than notifications, since it's a little simpler for callers, and the original reason I was using notifications is moot now.  This fixes test failures in the latest try push.

Comment 18

[Drew Willcoxon :adw]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> I would be happy to try to answer any questions that you might have, but I
> need more concrete questions.  :-)

Thanks.  I won't hesitate to ask questions about concrete, discrete pieces, but right now I'm trying to fit all the pieces together.

Comment 19

[Drew Willcoxon :adw]

https://tbpl.mozilla.org/?tree=Try&rev=2aa6f20ce22f

Comment 20

[Drew Willcoxon :adw]

https://tbpl.mozilla.org/?tree=Try&rev=5fac2a8bba05

Forgot to hg add.

Comment 21

[Drew Willcoxon :adw]

https://tbpl.mozilla.org/?tree=Try&rev=feacdd1a796b

The last try run looked good except for Android and b2g, but I can't make out what the "load failed" problems were there.  This one is very similar, but I improved the way that the starting and ending spell check notifications are fired.  If this goes well, I'll clean up the patch and ask for review again.

Comment 22

[Drew Willcoxon :adw]

That did not go well, but it failed in a helpful way!  It revealed a problem in the way I've been using the nsIEditorSpellCheck callbacks, even in the previous OK try run, and I think I know how to fix it.

Comment 23

[Drew Willcoxon :adw]

https://tbpl.mozilla.org/?tree=Try&rev=a0ebe06e959c

Comment 24

[Drew Willcoxon :adw]

Whoops, small but important change: https://tbpl.mozilla.org/?tree=Try&rev=d0059392ee76

Comment 25

[Drew Willcoxon :adw]

https://tbpl.mozilla.org/?tree=Try&rev=70cdb94ec697

Comment 26

[Drew Willcoxon :adw]

Tryserver looks good.  I'll clean up this patch, split it into more manageable pieces, and ask for review.

Comment 27

[Drew Willcoxon :adw]

Attachment: core changes

Sorry about the size of these patches... :-(

Comment 28

[Drew Willcoxon :adw]

Attachment: asyncSpellCheck test helper

Comment 29

[Drew Willcoxon :adw]

Attachment: reftest changes

These assume a change to reftest-content.js that I'll ask dbaron to review in bug 863430.

Comment 30

[Drew Willcoxon :adw]

Attachment: other test changes

Comment 31

[Drew Willcoxon :adw]

https://tbpl.mozilla.org/?tree=Try&rev=064ff40363da

Ehsan, looking at the asyncSpellCheck test helper attachment 746687 [details] [diff] [review] might shed light on the motivation for mozInlineSpellChecker's INLINESPELL_{STARTED,ENDED}_TOPIC notifications, and all the changes to mozInlineSpellChecker to support those notifications (in the core changes attachment 746686 [details] [diff] [review]).  Basically, tests need a reliable way to see when spell checks start and end, and if any spell checks are pending at the time they start.

Comment 32

[Drew Willcoxon :adw]

Hmm, there's an intermittent mochitest-other failure on Win8 that I haven't seen before that last try.

I pushed a couple of new tries that hopefully fix it... Whatever the fix is, I don't anticipate it requiring changes to any of the non-test patches here.  Unless I messed up something in the cleanup patch at the top of the queue.

Very messy patches with lots of printfs:
https://tbpl.mozilla.org/?tree=Try&rev=42c023f7e909

The same + cleanup patch (that also removes printfs), if that makes a difference:
https://tbpl.mozilla.org/?tree=Try&rev=fb6f9079e8f3

Comment 33

[Drew Willcoxon :adw]

Comment on attachment 746686 [details] [diff] [review]
core changes

Sigh, mozInlineSpellChecker::EditorSpellCheckInited uses NS_ASSERT instead of NS_ASSERTION.  (I changed it from NS_ABORT_IF_FALSE in the final patch.)  I've fixed it locally, but I won't bother posting a new patch here unless there are bigger changes needed.

Comment 34

[Drew Willcoxon :adw]

Once more with the NS_ASSERT fixed, and also because apparently not all the patches in my queue were pushed because of hg phase BS.

without cleanup patch:
https://tbpl.mozilla.org/?tree=Try&rev=4e8ebc577857

with cleanup patch:
https://tbpl.mozilla.org/?tree=Try&rev=abc88643f6e3

Comment 35

[(no longer active)]

I'll try to review these patches tomorrow.  Sorry for the delay!

Comment 36

[Drew Willcoxon :adw]

No problem!  Actually I'm still trying to figure out what the difference is between the passing test without my cleanup patch applied and the failing test with it applied.  I've been running tries for the past couple of days, and at this point I'm worried that it's down to different timings due to printf I/O...  But I still don't expect big changes to the core patch, so I think it would be OK to start review.

Comment 37

[(no longer active)]

Comment on attachment 746686 [details] [diff] [review]
core changes

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

This looks mostly good, but it's a very complicated patch, so I'd like to take another stab at it once you address the below and fix the test failures.

::: editor/composer/src/nsEditorSpellCheck.cpp
@@ +110,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  explicit DictionaryFetcher(nsEditorSpellCheck* aSpellCheck,

Drop explicit.

@@ +139,5 @@
> +  nsCOMPtr<nsIEditorSpellCheckCallback> mCallback;
> +  uint32_t mGroup;
> +  nsString mRootContentLang;
> +  nsString mRootDocContentLang;
> +  nsString mDictionary;

Nit: please make these members private.

@@ +285,5 @@
>    return NS_OK;
>  }
>  
> +// Instances of this class can be used as either runnables or RAII helpers.
> +class CallbackCaller : public nsRunnable

MOZ_FINAL.

@@ +387,5 @@
> +  if (NS_FAILED(rv) && aCallback) {
> +    // However, if it does fail, we still need to call the callback since we
> +    // discard the failure.  Do it asynchronously so that the caller is always
> +    // guaranteed async behavior.
> +    CallbackCaller* caller = new CallbackCaller(aCallback);

Please make this an nsRefPtr.

@@ +864,5 @@
>    return NS_OK;
>  }
>  
>  void 
>  nsEditorSpellCheck::ShutDown() {

Can you remove this function please?

::: editor/composer/src/nsEditorSpellCheck.h
@@ +62,5 @@
>    nsString mPreferredLang;
>  
>    bool mUpdateDictionaryRunning;
>  
> +  uint32_t mDictionaryFetcherGroup;

Nit: put this before the bool please.

::: editor/libeditor/base/nsEditor.cpp
@@ +1303,5 @@
>        // this observer recursively.
>        editorSpellCheck->CheckCurrentDictionary();
> +
> +      // update the inline spell checker to reflect the new current dictionary
> +      mInlineSpellChecker->SpellCheckRange(nullptr); // causes recheck

Why is this necessary?

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp
@@ +668,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  explicit InitEditorSpellCheckCallback(mozInlineSpellChecker* aSpellChecker)
> +    : mSpellChecker(aSpellChecker) {}
> +  NS_IMETHOD EditorSpellCheckDone() {

Nit: please put the opening brace on its own line here and elsewhere in this patch.

@@ +674,5 @@
> +  }
> +  void Cancel() {
> +    mSpellChecker = nullptr;
> +  }
> +  nsRefPtr<mozInlineSpellChecker> mSpellChecker;

Nit: Make this private please.

@@ +780,5 @@
> +// becomes zero or nonzero, observers are notified.  See NotifyObservers for
> +// info on the aEditor parameter.
> +void
> +mozInlineSpellChecker::ChangeNumPendingSpellChecks(int8_t aDelta,
> +                                                   nsIEditor* aEditor)

Here you're ignoring aEditor.  Why not just drop this argument?

Also, int32_t aDelta.

@@ +796,5 @@
> +
> +// Broadcasts the given topic to observers.  aEditor is passed to observers if
> +// nonnull; otherwise mEditor is passed.
> +void
> +mozInlineSpellChecker::NotifyObservers(const char* aTopic, nsIEditor* aEditor)

Do you pass in aEditor somewhere?

@@ +1545,5 @@
> +class AutoChangeNumPendingSpellChecks
> +{
> +public:
> +  explicit AutoChangeNumPendingSpellChecks(mozInlineSpellChecker* aSpellChecker,
> +                                           int8_t aDelta)

This doesn't need to be explicit.  Also, int32_t please.

@@ +1551,5 @@
> +  ~AutoChangeNumPendingSpellChecks() {
> +    mSpellChecker->ChangeNumPendingSpellChecks(mDelta);
> +  }
> +  nsRefPtr<mozInlineSpellChecker> mSpellChecker;
> +  int8_t mDelta;

Nit: private.

@@ +1908,5 @@
> +class UpdateCurrentDictionaryCallback MOZ_FINAL : public nsIEditorSpellCheckCallback
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +  explicit UpdateCurrentDictionaryCallback(mozInlineSpellChecker* aSpellChecker,

This doesn't need to be explicit.

@@ +1919,5 @@
> +           NS_OK :
> +           mSpellChecker->CurrentDictionaryUpdated();
> +  }
> +  nsRefPtr<mozInlineSpellChecker> mSpellChecker;
> +  uint32_t mGroup;

Nit: private.

@@ +1935,3 @@
>    }
>  
> +  nsCOMPtr<UpdateCurrentDictionaryCallback> cb =

nsRefPr please.

@@ +1951,5 @@
> +
> +// Called when nsIEditorSpellCheck::UpdateCurrentDictionary completes.
> +nsresult mozInlineSpellChecker::CurrentDictionaryUpdated()
> +{
> +  mNumPendingUpdateCurrentDictionary--;

Please assert that this doesn't underflow.

@@ +1960,5 @@
>        NS_FAILED(mSpellCheck->GetCurrentDictionary(currentDictionary))) {
>      currentDictionary.Truncate();
>    }
>  
> +  nsresult rv;

Move this nsresult inside the if please.

::: extensions/spellcheck/src/mozInlineSpellChecker.h
@@ +178,5 @@
> +  uint32_t mUpdateCurrentDictionaryCallbackGroup;
> +
> +  // When mPendingSpellCheck is non-null, this is the callback passed when
> +  // it was initialized.
> +  nsCOMPtr<InitEditorSpellCheckCallback> mPendingInitEditorSpellCheckCallback;

Nit: please move these up in order to get better packing.

@@ +262,5 @@
> +  nsresult CurrentDictionaryUpdated();
> +
> +  // track the number of pending spell checks and async operations that may lead
> +  // to spell checks, notifying observers accordingly
> +  void ChangeNumPendingSpellChecks(int8_t aDelta, nsIEditor* aEditor = nullptr);

Make this an int32_t.

Comment 38

[(no longer active)]

Comment on attachment 746687 [details] [diff] [review]
asyncSpellCheck test helper

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

I think this code is fragile, it's not clear to me why it's correct in all of the cases.

::: editor/asyncSpellCheck.jsm
@@ +47,5 @@
> +    var isc = editor.getInlineSpellChecker(false);
> +  }
> +  catch (err) {
> +    // getInlineSpellChecker throws if spell checking is not enabled instead of
> +    // just returning null, which seems kind of lame.

It seems to me that we shouldn't just eat this exception here.

@@ +70,5 @@
> +  os.addObserver(observe, SPELL_CHECK_ENDED_TOPIC, false);
> +
> +  let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +  timer.init(function tick() {
> +    if (waitingForEnded || ++count < 2)

Where does the magic value 2 come from?

Comment 39

[(no longer active)]

Comment on attachment 746690 [details] [diff] [review]
other test changes

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

::: editor/composer/test/test_async_UpdateCurrentDictionary.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

This file already exists in the core patch.

::: editor/composer/test/test_bug338427.html
@@ +31,5 @@
> +    onSpellCheck(textarea, function () {
> +        var list = {}, count = {};
> +        spellchecker.spellChecker.GetDictionaryList(list, count);
> +        if (count.value === 0) {
> +            SimpleTest.finish();

please fail the test if this happens.

@@ +40,5 @@
>  
> +        onSpellCheck(textarea, function () {
> +            var dictionary = "";
> +            try {
> +                dictionary = spellchecker.spellChecker.GetCurrentDictionary();

Nit: move 'var' here.

::: editor/composer/test/test_bug678842.html
@@ +37,5 @@
> +  onSpellCheck(elem, function () {
> +    var spellchecker = inlineSpellChecker.spellChecker;
> +    var currentDictonary = "";
> +    try {
> +      currentDictonary = spellchecker.GetCurrentDictionary();

Ditto.

::: editor/libeditor/text/tests/test_bug636465.xul
@@ +53,5 @@
> +        ok(compareSnapshots(spellCheckNone, spellCheckFalse, true)[0],
> +           "Unsetting the spellcheck attribute should work");
> +        SimpleTest.finish();
> +      });
> +    });

Yay!!!

Comment 40

[(no longer active)]

Comment on attachment 746687 [details] [diff] [review]
asyncSpellCheck test helper

Actually I'm not convinced this should be a jsm that everybody can use.  You're only using it for tests, right?  In which case, I'd make it a SimpleTest API or something.

Comment 41

[Drew Willcoxon :adw]

Thanks, Ehsan.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #37)
> ::: editor/libeditor/base/nsEditor.cpp
> @@ +1303,5 @@
> >        // this observer recursively.
> >        editorSpellCheck->CheckCurrentDictionary();
> > +
> > +      // update the inline spell checker to reflect the new current dictionary
> > +      mInlineSpellChecker->SpellCheckRange(nullptr); // causes recheck
> 
> Why is this necessary?

It's probably not necessary.  I changed it while trying to get test_add_remove_dictionaries.xul to pass, but despite my notes I don't see now why it would be necessary.

> @@ +780,5 @@
> > +// becomes zero or nonzero, observers are notified.  See NotifyObservers for
> > +// info on the aEditor parameter.
> > +void
> > +mozInlineSpellChecker::ChangeNumPendingSpellChecks(int8_t aDelta,
> > +                                                   nsIEditor* aEditor)
> 
> Here you're ignoring aEditor.  Why not just drop this argument?

Whoa, that is a bug.  Thanks for pointing it out.  aEditor should be passed to NotifyObservers.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #38)
> Comment on attachment 746687 [details] [diff] [review]
> asyncSpellCheck test helper
> 
> Review of attachment 746687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this code is fragile, it's not clear to me why it's correct in all
> of the cases.

We should clear this up since this test helper is fundamental.  The test changes and much of the mozInlineSpellChecker changes are basically built around it.

Hopefully it's the opposite of fragile!  I was going for robustness, which I tried to explain in the function comment.

There are a couple of related problems it solves.  Sometimes tests don't have full control over when they trigger spell checks or async operations that potentially lead to spell checks.  For example, there was one test that triggered a certain number of spell checks when run alone, but when run in a suite, it triggered a different number of spell checks.

The other problem is that even if a test knows where all the spell checks (and async operations that potentially lead to spell checks) happen, it's really tedious and error-prone -- fragile -- to correctly wait for all of them.  It's much easier to do a bunch of things all at once that may queue up many spell checks, wait for them all to finish, and then move on with the test.  It's also nice to allow for the possibility of no spell check occurring at all.

> ::: editor/asyncSpellCheck.jsm
> @@ +47,5 @@
> > +    var isc = editor.getInlineSpellChecker(false);
> > +  }
> > +  catch (err) {
> > +    // getInlineSpellChecker throws if spell checking is not enabled instead of
> > +    // just returning null, which seems kind of lame.
> 
> It seems to me that we shouldn't just eat this exception here.

Originally it didn't eat it, but NS_ERROR_FAILURE was getting raised by the Android reftests, turning them orange (due to mozInlineSpellChecker::CanEnableInlineSpellChecking returning false [1]).  I don't understand why getInlineSpellChecker would raise an exception at all in this case: if inline spell checking isn't enabled, then just return null.  It's just a getter.

But anyway, the point is only to determine if a spell check is pending at this moment.  If getInlineSpellChecker throws, then no spell check is pending.

[1] http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#1265

> @@ +70,5 @@
> > +  os.addObserver(observe, SPELL_CHECK_ENDED_TOPIC, false);
> > +
> > +  let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > +  timer.init(function tick() {
> > +    if (waitingForEnded || ++count < 2)
> 
> Where does the magic value 2 come from?

It's arbitrary.  I wanted to allow a small number of turns of the event loop to pass before declaring that no more spell checks are forthcoming.

This is the only part that polls.  I think it's OK because there's nothing, as far as I know, in the testing environment that would cause a spell check to be started after such a period.

Maybe it should be increased to 5 or so.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #40)
> Comment on attachment 746687 [details] [diff] [review]
> asyncSpellCheck test helper
> 
> Actually I'm not convinced this should be a jsm that everybody can use. 
> You're only using it for tests, right?  In which case, I'd make it a
> SimpleTest API or something.

I don't think reftest-content.js would be able to access it then, would it?  That's why I put it in a jsm: so that both tests and reftest-content.js could use it without copying and pasting it.

Comment 42

[(no longer active)]

(In reply to comment #41)
> Thanks, Ehsan.
> 
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #37)
> > ::: editor/libeditor/base/nsEditor.cpp
> > @@ +1303,5 @@
> > >        // this observer recursively.
> > >        editorSpellCheck->CheckCurrentDictionary();
> > > +
> > > +      // update the inline spell checker to reflect the new current dictionary
> > > +      mInlineSpellChecker->SpellCheckRange(nullptr); // causes recheck
> > 
> > Why is this necessary?
> 
> It's probably not necessary.  I changed it while trying to get
> test_add_remove_dictionaries.xul to pass, but despite my notes I don't see now
> why it would be necessary.

Then let's revert this hunk if it doesn't break anything.  :-)

> > @@ +780,5 @@
> > > +// becomes zero or nonzero, observers are notified.  See NotifyObservers for
> > > +// info on the aEditor parameter.
> > > +void
> > > +mozInlineSpellChecker::ChangeNumPendingSpellChecks(int8_t aDelta,
> > > +                                                   nsIEditor* aEditor)
> > 
> > Here you're ignoring aEditor.  Why not just drop this argument?
> 
> Whoa, that is a bug.  Thanks for pointing it out.  aEditor should be passed to
> NotifyObservers.

Hmm, I though that NotifyObservers would just use mEditor, which should be fine as it will be kept alive by the caller of this function.  Am I missing something?

> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #38)
> > Comment on attachment 746687 [details] [diff] [review]
> > asyncSpellCheck test helper
> > 
> > Review of attachment 746687 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think this code is fragile, it's not clear to me why it's correct in all
> > of the cases.
> 
> We should clear this up since this test helper is fundamental.  The test
> changes and much of the mozInlineSpellChecker changes are basically built
> around it.
> 
> Hopefully it's the opposite of fragile!  I was going for robustness, which I
> tried to explain in the function comment.
> 
> There are a couple of related problems it solves.  Sometimes tests don't have
> full control over when they trigger spell checks or async operations that
> potentially lead to spell checks.  For example, there was one test that
> triggered a certain number of spell checks when run alone, but when run in a
> suite, it triggered a different number of spell checks.
> 
> The other problem is that even if a test knows where all the spell checks (and
> async operations that potentially lead to spell checks) happen, it's really
> tedious and error-prone -- fragile -- to correctly wait for all of them.  It's
> much easier to do a bunch of things all at once that may queue up many spell
> checks, wait for them all to finish, and then move on with the test.  It's also
> nice to allow for the possibility of no spell check occurring at all.

Hmm, perhaps I judged too quickly.  To be honest, that number 2 really threw me off.  But I'd be happy to look at it again.  Feel free to reflag me for review when you address my comment about the magic number below!

> > ::: editor/asyncSpellCheck.jsm
> > @@ +47,5 @@
> > > +    var isc = editor.getInlineSpellChecker(false);
> > > +  }
> > > +  catch (err) {
> > > +    // getInlineSpellChecker throws if spell checking is not enabled instead of
> > > +    // just returning null, which seems kind of lame.
> > 
> > It seems to me that we shouldn't just eat this exception here.
> 
> Originally it didn't eat it, but NS_ERROR_FAILURE was getting raised by the
> Android reftests, turning them orange (due to
> mozInlineSpellChecker::CanEnableInlineSpellChecking returning false [1]).  I
> don't understand why getInlineSpellChecker would raise an exception at all in
> this case: if inline spell checking isn't enabled, then just return null.  It's
> just a getter.

Have you debugged this?  IIRC we disable spell checking on Android using the pref, so that might have something to do with what you're seeing.

> But anyway, the point is only to determine if a spell check is pending at this
> moment.  If getInlineSpellChecker throws, then no spell check is pending.

Hmm, OK, can you at least clarify this in the comment?

> > @@ +70,5 @@
> > > +  os.addObserver(observe, SPELL_CHECK_ENDED_TOPIC, false);
> > > +
> > > +  let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > > +  timer.init(function tick() {
> > > +    if (waitingForEnded || ++count < 2)
> > 
> > Where does the magic value 2 come from?
> 
> It's arbitrary.  I wanted to allow a small number of turns of the event loop to
> pass before declaring that no more spell checks are forthcoming.
> 
> This is the only part that polls.  I think it's OK because there's nothing, as
> far as I know, in the testing environment that would cause a spell check to be
> started after such a period.
> 
> Maybe it should be increased to 5 or so.

Setting it to 5 would just make me want to ask where 5 comes from.  ;-)  Isn't a better solution to poll indefinitely until we get what we're expecting (and info() a message every time to make it obvious that we're waiting on something to happen)?  I think if we end up waiting too few iterations then we're going to end up with hard to debug test failures.

> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #40)
> > Comment on attachment 746687 [details] [diff] [review]
> > asyncSpellCheck test helper
> > 
> > Actually I'm not convinced this should be a jsm that everybody can use. 
> > You're only using it for tests, right?  In which case, I'd make it a
> > SimpleTest API or something.
> 
> I don't think reftest-content.js would be able to access it then, would it? 
> That's why I put it in a jsm: so that both tests and reftest-content.js could
> use it without copying and pasting it.

Hmm, you're right.  The point is that I absolutely do not want this to end up something that add-ons would start to use...  But I guess we can't avoid using a jsm.  Can you please rename it to something scary at least, such as SpellCheckerTestingHelper.jsm or something, so that we send a message to potential (ab)users of this that they'll get what they deserve the next time we decide to touch this code?  ;-)

Last but not least, thanks a lot for sticking around addressing my comments and nits here.  I understand that this turned out to be a lot more painful than what you were hoping for (as is the usual experience of people who touch this code!)

Comment 43

[Drew Willcoxon :adw]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #42)
> Hmm, I though that NotifyObservers would just use mEditor, which should be
> fine as it will be kept alive by the caller of this function.  Am I missing
> something?

There's one path where mEditor is nulled out.

When passed false, mozInlineSpellChecker::SetEnableRealTimeSpell calls Cleanup, which nulls out mEditor, and it also decreases the number of pending spell checks, which will trigger the "ended" notification if the number reaches zero.  However, the notification should happen *after* the Cleanup call because Cleanup changes the selection, and observers should be notified after all selection changes and whatnot have stopped.  If the editor were not passed to ChangeNumPendingSpellChecks (and then to NotifyObservers), then NotifyObservers would use mEditor, which at that point would be null.

> > Originally it didn't eat it, but NS_ERROR_FAILURE was getting raised by the
> > Android reftests, turning them orange (due to
> > mozInlineSpellChecker::CanEnableInlineSpellChecking returning false [1]).  I
> > don't understand why getInlineSpellChecker would raise an exception at all in
> > this case: if inline spell checking isn't enabled, then just return null.  It's
> > just a getter.
> 
> Have you debugged this?  IIRC we disable spell checking on Android using the
> pref, so that might have something to do with what you're seeing.

Only insofar as finding that mozInlineSpellChecker::CanEnableInlineSpellChecking returns false.  If spell check is disabled on Android, then that's certainly a plausible explanation for why CanEnableInlineSpellChecking would return false and GetInlineSpellChecker would therefore raise an error.

I'll clarify the comment.

> Setting it to 5 would just make me want to ask where 5 comes from.  ;-) 

Yeah, fair enough.

> Isn't a better solution to poll indefinitely until we get what we're
> expecting

Well, what we're waiting for is either no more spell checks or a new spell check.

We can wait indefinitely in the opposite case, when a spell check has started.  Then you just wait for the end.  (Which is why I added nsIInlineSpellChecker.spellCheckPending and the "ended" notification.)

You can't generally guarantee that if a spell check doesn't start in N turns of the event loop, then it won't start in > N turns.  But my thinking was that in a testing environment, you could safely assume it, since each test is vaguely in control of when spell checks happen, and internally the spell check code doesn't post events or runnables or anything that would kick off a spell check so far into the future.

> Hmm, you're right.  The point is that I absolutely do not want this to end
> up something that add-ons would start to use...

Yeah, the entire surface of Firefox is its add-on API. :-(

> Last but not least, thanks a lot for sticking around addressing my comments
> and nits here.  I understand that this turned out to be a lot more painful
> than what you were hoping for (as is the usual experience of people who
> touch this code!)

Thank you!  Reviewing giant patches like these is hard.

Comment 44

[(no longer active)]

(In reply to Drew Willcoxon :adw from comment #43)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #42)
> > Hmm, I though that NotifyObservers would just use mEditor, which should be
> > fine as it will be kept alive by the caller of this function.  Am I missing
> > something?
> 
> There's one path where mEditor is nulled out.
> 
> When passed false, mozInlineSpellChecker::SetEnableRealTimeSpell calls
> Cleanup, which nulls out mEditor, and it also decreases the number of
> pending spell checks, which will trigger the "ended" notification if the
> number reaches zero.  However, the notification should happen *after* the
> Cleanup call because Cleanup changes the selection, and observers should be
> notified after all selection changes and whatnot have stopped.  If the
> editor were not passed to ChangeNumPendingSpellChecks (and then to
> NotifyObservers), then NotifyObservers would use mEditor, which at that
> point would be null.

Hmm, right.  I guess the moral of the story is that this code is way too complicated.  :/

> > > Originally it didn't eat it, but NS_ERROR_FAILURE was getting raised by the
> > > Android reftests, turning them orange (due to
> > > mozInlineSpellChecker::CanEnableInlineSpellChecking returning false [1]).  I
> > > don't understand why getInlineSpellChecker would raise an exception at all in
> > > this case: if inline spell checking isn't enabled, then just return null.  It's
> > > just a getter.
> > 
> > Have you debugged this?  IIRC we disable spell checking on Android using the
> > pref, so that might have something to do with what you're seeing.
> 
> Only insofar as finding that
> mozInlineSpellChecker::CanEnableInlineSpellChecking returns false.  If spell
> check is disabled on Android, then that's certainly a plausible explanation
> for why CanEnableInlineSpellChecking would return false and
> GetInlineSpellChecker would therefore raise an error.
> 
> I'll clarify the comment.

Sounds good.

> > Isn't a better solution to poll indefinitely until we get what we're
> > expecting
> 
> Well, what we're waiting for is either no more spell checks or a new spell
> check.
> 
> We can wait indefinitely in the opposite case, when a spell check has
> started.  Then you just wait for the end.  (Which is why I added
> nsIInlineSpellChecker.spellCheckPending and the "ended" notification.)
> 
> You can't generally guarantee that if a spell check doesn't start in N turns
> of the event loop, then it won't start in > N turns.  But my thinking was
> that in a testing environment, you could safely assume it, since each test
> is vaguely in control of when spell checks happen, and internally the spell
> check code doesn't post events or runnables or anything that would kick off
> a spell check so far into the future.

Well, it doesn't now, but who knows what it's going to do in the future?

OK, let's make this magic number something unreasonably big then, like 50.  Does that sound reasonable?

Comment 45

[Drew Willcoxon :adw]

Status update.  test_bug366682.html is stubbornly intermittent on try, and I'm trying to figure out why.  In some runs on Windows 8, spell checking just doesn't happen.  (The test loads a page in an iframe and turns on design mode on it.)  So far I have it narrowed down to mozInlineSpellChecker::DoSpellCheck's call to mSpellCheck->CheckCurrentWordNoSuggest sometimes failing.  I'm debugging over tryserver with printfs, so it's slow going, and frustratingly the printfs seem to affect whether the failure appears.

Comment 46

[(no longer active)]

Perhaps you can ask RelEng to give you remote access to one of the boxes on which this failure happens?  That should shorten the length of your debugging cycles.

Comment 47

[Drew Willcoxon :adw]

Attachment: core changes 2

Aha.  When I broke apart nsEditorSpellCheck::UpdateCurrentDictionary into UpdateCurrentDictionary and DictionaryFetched, I declared nsresult rv at the top of DictionaryFetched.  But I didn't define it before one path accesses it.  So it looks like sometimes it happens to be 0 (NS_OK) and other times not, the latter causing the intermittent failure in test_bug366682.html, but interestingly only on Windows 8.  Or at least that's the only place it popped up initially.

Rookie mistake.

https://tbpl.mozilla.org/?tree=Try&rev=6b67c1f0f7d1

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #37)
> Comment on attachment 746686 [details] [diff] [review]
> core changes
> 
> Review of attachment 746686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +139,5 @@
> > +  nsCOMPtr<nsIEditorSpellCheckCallback> mCallback;
> > +  uint32_t mGroup;
> > +  nsString mRootContentLang;
> > +  nsString mRootDocContentLang;
> > +  nsString mDictionary;
> 
> Nit: please make these members private.

nsEditorSpellCheck accesses all of them but mDictionary, so I made mDictionary private.  (It seems unnecessary in this little file-scoped helper class to make the others private and expose them with public methods.)

Comment 48

[Drew Willcoxon :adw]

Attachment: test helper 2

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #44)
> OK, let's make this magic number something unreasonably big then, like 50. 
> Does that sound reasonable?

Sure.

Comment 49

[Drew Willcoxon :adw]

The try run in the previous comment had a couple of errors.  The mochitest error is just my using is() when I should have used ok(), and I've fixed it locally.  The reftest error is yet another editor reftest that needs to be updated to wait for spell check.  First time it's failed.  I should go over all editor reftests and make them all wait even if they haven't failed on try by this point, but I should probably bug David to review the reftest-content.js change first, since reftest changes depend on it.

But review on these two new patches here can go ahead.

Comment 50

[Drew Willcoxon :adw]

Attachment: core changes 3

Working on bug 863430 led me to find a small problem with the core patch.  I won't ask for review again since the changes are pretty small, to class mozInlineSpellResume in mozInlineSpellChecker.cpp, to mozInlineSpellChecker::SetEnableRealTimeSpell, and renaming mozInlineSpellChecker.mUpdateCurrentDictionaryCallbackGroup to mDisabledAsyncToken.

When a mozInlineSpellChecker is disabled, it cancels pending nsEditorSpellCheck initialization and UpdateCurrentDictionary calls, but it doesn't cancel scheduled spell checks started by ScheduleSpellCheck.  If there's a scheduled spell check at that time, then its runnable resumes some time later and calls ResumeSpellCheck, which causes the number of pending spell checks to become zero, which causes the ENDED notification to be broadcasted.  So ENDED is correctly broadcasted -- that's not the problem.  The problem is that it happens too late, which has a couple of bad consequences:

(1) After the mozInlineSpellChecker was disabled but before ENDED is broadcasted, the editor may have created another mozInlineSpellChecker, which may have broadcasted more STARTEDs and ENDEDs.  So observers are now observing interleaved notifications for the same editor, which can easily confuse them, e.g., by receiving two consecutive STARTEDs.

(2) mEditor is nulled out by that point, so observers don't receive it, so they may not be able to pair the ENDED with the earlier STARTED, so they can't tell that spell checking has ended.

Comment 51

[Drew Willcoxon :adw]

Attachment: reftest changes 2

This updates the reftests for the new patch in bug 863430.  Since that patch builds waiting for spell check into the harness, many fewer tests had to be updated than my previous reftest patch here updated.  I guess I should ask for review on this again, but probably after David reviews the patch in bug 863430.

Comment 52

[Drew Willcoxon :adw]

Attachment: test helper 3

I had to change const EXPORTED_SYMBOLS to this.EXPORTED_SYMBOLS in the test helper jsm to make the tests not bizarrely fail on b2g (due to bug 810719, bug 813766).

Comment 53

[Drew Willcoxon :adw]

Comment on attachment 756916 [details] [diff] [review]
core changes 3

Benjamin, would you mind super-reviewing?  There are idl changes to editor/idl/nsIEditorSpellCheck.idl and nsIInlineSpellChecker.idl, and also changes to header files related to spell checking.

Comment 54

[Drew Willcoxon :adw]

Oh, and tryserver with these patches and the one in bug 863430:

https://tbpl.mozilla.org/?tree=Try&rev=f1ee5f64cad9

Comment 55

[Drew Willcoxon :adw]

Comment on attachment 756918 [details] [diff] [review]
reftest changes 2

This has substantial changes from the previous patch so it needs another review, but the patch is much smaller this time.

Instead of adding a reftestWaitForSpellCheck attribute to all reftests affected by spell checking, only reftests that can't rely on the built-in waiting in reftest-content.js need to be updated.  These tests use class="spell-checked" on elements for which script will turn on spell checking to signal to the harness that it should wait for them.

Comment 56

[Benjamin Smedberg]

Comment on attachment 756916 [details] [diff] [review]
core changes 3

I am not qualified to sr any of this.

Comment 57

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 756916 [details] [diff] [review]
core changes 3

There are a couple of hits on the add-ons MXR for one of the methods you're making async:
https://mxr.mozilla.org/addons/search?string=InitSpellChecker

Only two hits, though. One of them is https://addons.mozilla.org/en-US/firefox/addon/spellbound/ (which is incompatible with recent Firefoxes anyways), and the other is https://addons.mozilla.org/en-US/thunderbird/addon/buttons/. It would be nice to take a quick look at whether it's affected by this and dropping the author a note.

Comment 58

[:Gavin Sharp [email: [e-mail]]]

(CCing the author of the relevant add-on from comment 57)

Comment 59

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f48c49425ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/eac7a8c8d5d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b29a5c94955
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f4ad11d56da

Comment 60

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8f48c49425ab
https://hg.mozilla.org/mozilla-central/rev/eac7a8c8d5d4
https://hg.mozilla.org/mozilla-central/rev/6b29a5c94955
https://hg.mozilla.org/mozilla-central/rev/3f4ad11d56da