1193293 - Wrong dictionary fetched when page contains <div contentEditable> together with <input>

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Jan Horak [:jhorak]]

Attachment: testcase page

When content preference of spell checking for URI is set, it's reset to first directory in list because document contains more 'editor' instances (one for input and the other for element with contentEditable attribute). 

Result is that when user set they own preference of dictionary on some page, when the page loads again his preference is lost (and also set wrongly in content prefs).

Call of CheckCurrentDirectory which makes this reset is made during handle of SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditor.cpp#1323

When there is only one instance of editor problem is solved by 'holder':
http://mxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#741

Could making the nsEditorSpellCheck a service which shares its same instance for all editor objects solve this problem?

Testcase repro:
1) have more than one spellchecking dictionary installed
2) open testcase
3) enable spell checking for input element by context menu/Check spelling
4) set preferred dictionary (to save it to content prefs)
5) restart firefox, open testcase
6) enable spell checking again and check the dictionary name
7) dictionary may be wrong (depends on which dictionary is the first in dictionary list).

Comment 1

[Jan Horak [:jhorak]]

Downstream bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1193293

Comment 2

[Jan Horak [:jhorak]]

I guess it is a duplicate of bug 1204147.

Comment 3

[Jorg K (CEST = GMT+2)]

I wasn't aware of this bug. I found bug 1204147 late on Saturday night and fixed it while cleaning up dictionary related issues. Your test case is simpler than mine, ah well.

Comment 4

[Jorg K (CEST = GMT+2)]

Sadly, this is still not 100% fixed. With the fix to bug 1204147, the correct content preference is written. However, after enabling spell checking and first clicking on the field I get "en-GB", the first dictionary in my list. On second click, I get "fr-modern", which is what the content preference says.
The debug always says:
***** mPreferredLang (element) ||
***** mPreferredLang (content-language) ||
***** Assigned from content preferences |fr-modern|
but sadly, the menu doesn't follow.

So there is another problem in this terrible event driven asynchronous stuff. From what I saw while debugging the other bug 1204147 is that the editor tries to get the dictionary while not fully initialised: see bug 1204147 comment #1 (quote):
===
observer [...] gets called first. It isn't fully initialised, since the mSpellCheckingEngine is NULL here:
http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozSpellChecker.cpp#365
Hence the current dictionary is returned empty.
===
... and then the first one is picked from the list.

All the tests we run use textareas, but as we can see, those "contenteditable"s behave differently. Too bad.

Comment 5

[Jorg K (CEST = GMT+2)]

One more observation:
In my test case, FF 40 stores "fr-modern" in the content preference. Upon restart, that gets *overwritten* by "en-GB", the first in my list. With the fix from bug 1204147 we stopped the overwriting, but as detailed in comment #4, the correct dictionary is not displayed upon first click. So whilst the "dataloss" problem is solved, the user confusion isn't solved.

Comment 6

[Jorg K (CEST = GMT+2)]

More debug:
***** mPreferredLang (element) || (*)
***** mPreferredLang (content-language) || (*)
***** Observer spellcheck-dictionary-update on 04FBB840
***** Observer spellcheck-dictionary-update on 0B779000
+++++ mozSpellChecker::GetCurrentDictionary - !mSpellCheckingEngine, truncating
+++++ nsEditorSpellCheck::CheckCurrentDictionary() - setting first in list |en-GB|
***** Observer spellcheck-dictionary-update on 04FBB840
***** Observer spellcheck-dictionary-update on 0B779000
***** Assigned from content preferences |fr-modern| (**)

So what happens is this:
While the language is retrieved for the element in nsEditorSpellCheck::DictionaryFetched(), marked with (*) in the debug, an update happens. At that stage the language isn't set yet, since it gets set at (**).

The update cuts right into the processing in nsEditorSpellCheck::DictionaryFetched() and sets an undesired dictionary.

Question is: Why is this update triggered. Sadly this event driven stuff is very hard to debug in the debugger.

Comment 7

[Jorg K (CEST = GMT+2)]

Damn, got confused by my own debug. The 
  Assigned from content preferences |fr-modern|
is printed after the dictionary has been set. So the updates we're seeing originate from that. Here are some more comments:

***** mPreferredLang (element) || (*)
***** mPreferredLang (content-language) || (*)

As this point we retrieve the content preferences and set (1) the dictionary according to these preferences.
The result is that the observers are called with the update.
***** Observer spellcheck-dictionary-update on 04FBB840
***** Observer spellcheck-dictionary-update on 0B779000

One of the observers has the urgent need to check the dictionary, and that results in |en-GB| being set (2).
+++++ mozSpellChecker::GetCurrentDictionary - !mSpellCheckingEngine, truncating
+++++ nsEditorSpellCheck::CheckCurrentDictionary() - setting first in list |en-GB|

This then triggers a second round of updates:
***** Observer spellcheck-dictionary-update on 04FBB840
***** Observer spellcheck-dictionary-update on 0B779000

In the end we return from the first set and get this print.
***** Assigned from content preferences |fr-modern| (**)

There two observers, since there are two editors in the example tripping over one another.

<html>
<body>
<div contentEditable="true"></div>
<input type="text">
</body>
</html>

To be continued.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: testcase page (made the contenteditable visible)

Comment 9

[Jorg K (CEST = GMT+2)]

Robert, any hints on how to approach this dilemma?

In bug 697981 we switched off the updating for editors that don't have focus, but the contenteditable always receives focus events, even when it's not clicked on. I've noticed that with attachment 8661400 [details]. If I click on the input, I get two focus events, one for the input, one for the div. If I click on the div, I get only one.

Looks like we need to come up with the plan you were looking for ;-)

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: WIP - trying stuff.

Let's see what breaks if we remove the trouble maker altogether.
Removed SetCurrentDictionary(dictList[0]); from nsEditorSpellCheck::CheckCurrentDictionary():
https://treeherder.mozilla.org/#/jobs?repo=try&revision=255931fd4df3

A check function that also does some misguided setting of stuff. Hmm. Also, looking at the debug: We set one dictionary and as a result another one is set, just crazy!

The attached test case works with this removed.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: WIP - trying stuff - take 2

Green so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=255931fd4df3

Robert, how do you feel about gutting this function (see attachment take 2)? I think it had its validity when there was one dictionary used for everything, and you had to set another one, when this one disappeared. But these days are long gone.

Let's try a bit harder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f1b22ee55aa

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This patch doesn't look good, because ChecKCurrentDictionary no longer does what nsIEditorSpellCheck says it will do.

Comment 13

[Jorg K (CEST = GMT+2)]

Robert, I'm sorry, I have to disagree on this one as well.

Please look at the try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f1b22ee55aa

It is *compeltely* green (not even one of the little intermittent failures raised it's ugly head).

IMHO "checking" something and then selecting something else *at random* is not good practice. This code predates a) the introduction of site dependent dictionary choice (bug 338427) and b) the introduction of content preferences (bug 678842).

In bug 1200533 we *carefully* cleaned up the dictionary selection, please view:
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#796
(the last fallback there is indeed to use the first dictionary in the list).

And then comes a dinosaur and tramples over the finely tuned dictionary choice and says:
  Oh, I know, I'll just pick the first.
That's not right.

As you stated, the comment on the function says:
http://mxr.mozilla.org/mozilla-central/source/editor/nsIEditorSpellCheck.idl#19
"If the current dictionary is no longer available, then pick another one.",
but comments can be changed, can't they?

Another option would be to remove the call to CheckCurrentDictionary() from the editor and see what happens. I will try that.

What do you think?

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: WIP - trying stuff - take 3, even more aggressive this time.

Removed checking of the dictionary from the editor.
No idea why this was there in the first place, seems like an anachronism. The dictionary we're updating to should always exist, so no need to check, especially with a check that tries to know better, sets something else and causes a recursive call. Let's see whether I am right ;-)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7689230a7fd

Perhaps the fix in bug 1204147 wasn't right. There we stopped content preferences being written by CheckCurrentDictionary(). If this is a "public" interface, perhaps it should write the content preference.

In conclusion:
If removing the call to CheckCurrentDictionary() from the editor works, I would like to make the case for the complete *removal* of this function, whose purpose isn't even clear.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8661621 [details] [diff] [review]
WIP - trying stuff - take 3, even more aggressive this time.

The try run is (almost) completely green (apart from the usual failure):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7689230a7fd
I'd like to go ahead and remove this code.

I think that this is an anachronism from the time where we set the dictionary do one that didn't exist. We no longer do that, so this code can go.

Depending on your opinion, I would remove nsEditorSpellCheck::CheckCurrentDictionary()
altogether. This is not to be confused with mozSpellChecker::CheckCurrentDictionary().

What do you think?

Comment 16

[Jorg K (CEST = GMT+2)]

Some more analysis:
The code I'd like to remove was added here:
http://hg.mozilla.org/mozilla-central/diff/e3cfe5ae818c/editor/libeditor/base/nsEditor.cpp
for bug 591780 (landed bug 591780 comment #104).

As of bug 697981 of we handle dictionary removal differently. There is a special notification for it. We have a test for it and the update on removal works even when the editor is not focused.

Let's recall what happens. There are two editors on the page. One receives focus, so it determines the dictionary that needs to be used and sets it. Sadly, the trouble-making contexteditable also has also received a focus event and registered an observer. The observers run, so by accident, while "checking", the dictionary gets set to the first one in the list, this causes a recursive call, everything is checked (asynchronously) in the wrong language. Then the recursion returns and everything gets checked in the language we wanted to set in the first place. Crazy, is it not?

So I think this should go and apparently it doesn't affect anything.

Comment 17

[Jorg K (CEST = GMT+2)]

Actually, bug 697981 also created nsEditorSpellCheck::CheckCurrentDictionary():
https://hg.mozilla.org/mozilla-central/diff/e3cfe5ae818c/editor/composer/src/nsEditorSpellCheck.cpp
(checkin bug 591780 comment #94, not #104).

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8661621 [details] [diff] [review]
WIP - trying stuff - take 3, even more aggressive this time.

This isn't complete yes, so I'd better ask for feedback only.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: Take 4, removed call to CheckCurrentDictionary.

Actually, I'll sleep better with this change ;-)
Using TryDictionary() makes 100% sure that we will *never* call SetCurrentDictionary() with a dictionary that doesn't exist. So the editor does really not need to do any checking any more. I should have made this change during the clean-up in bug 1200533, ah well.

Another try here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=724b8843c819
The result will be the same, I suppose.

Comment 20

[Jorg K (CEST = GMT+2)]

The try https://treeherder.mozilla.org/#/jobs?repo=try&revision=724b8843c819 is green (although this time with more orange sprinkles, seems to depend on the hour of the day and the server load).

So this is my proposed solution: Just remove the call to nsEditorSpellCheck::CheckCurrentDictionary(). In fact, I'd remove the function as well, since I'd be removing its only call.

While I've got the box open I'd also like to use TryDictionary() where indicated, so that even if the user manipulates the SQLite database, we will never attempt to set an invalid dictionary any more.

What do you think?

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution: CheckCurrentDictionary() removed completely.

Sorry, this bug is becoming too wordy. My fault.
Here is my proposed solution: Just remove the faulty code. There is no need for it. Green try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5317e451110b
(IMHO try works best with low server load in the AM hours in Europe when the US are still sleeping).

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: testcase page as used in the manual test case documented.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: Documenting manual testing.

Sadly, in this bug automated testing is not possible, since the problem only happens directly after restart.

Testing steps:
0) Open the attachment and set the language to Spanish.
   Close the session.

Without the fix:
1) Start FF again, open the page.
2) Click into the input field, select "Check Spelling".
3) Result: div and input field are spelled in English.
Why: Trying to spell the div, found no valid dictionary yet,
so CheckCurrentDictionary() desperately sets the first one it found: en-GB.
(Note: In bug 1204147 we stopped en-GB being written as content preference in this situaltion.)

With the fix:
1) Start FF again, open the page.
2) Click into the input field, select "Check Spelling".
3) Result: div is not checked, input field is checked in Spanish.
4) Click on the div: div now also spelled in Spanish.
Why: Trying to spell the div found no valid dictionary, so no checking took place. Later a valid dictionary was found and the spell check proceeded in Spanish.

Clearly its a very subtle initialisation and timing issue when an element whose language hasn't been determined yet is called to update itself. It's better not to spell this element, since the element doesn't have focus(*), than to spell *all* elements in the wrong language.

Remark: (*) Sadly internally the div receives focus although it shouldn't, but to the user it doesn't look like it has focus. The superfluous focus of the div is another problem we should address elsewhere.

I think this fix clears up any remaining confusion in this area and I'd like to see it landed on FF 43 together with the other dictionary clean-up in bug 1200533, bug 717433, bug 697981 and bug 1204147.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8662344 [details] [diff] [review]
Proposed solution: CheckCurrentDictionary() removed completely.

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

This looks good. Making things simpler is very good :-).

Comment 25

[Carsten Book [:Tomcat]]

hey Jorg, when trying to checkin i got:

remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsIEditorSpellCheck in changeset 698f66612238
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsIEditorSpellCheck
remote: 
remote: If you intentionally want to keep the current UUID, include
remote: 'IGNORE IDL' in the commit message.

is this expected ?

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: Same patch, but with ./mach update-uuids nsIEditorSpellCheck run.

Hey Carsten, yes, that's expected, since I removed an interface.
I've just run it myself and enclose a modified patch.
Sorry about that.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c79d3947c307

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c79d3947c307

Comment 29

[Jorg K (CEST = GMT+2)]

Requested backing out of this patch.

Comment 30

[Carsten Book [:Tomcat]]

(In reply to Jorg K (GMT+2) from comment #29)
> Requested backing out of this patch.

backed out from m-c by request from jork

Comment 31

[Jorg K (CEST = GMT+2)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/5da1fc351aba

Comment 32

[Jorg K (CEST = GMT+2)]

Robert, we gutted too much here.

My initial feeling was right, we should have just gutted the setting of a new dictionary.

By gutting the whole thing, we removed the call to mozSpellChecker::CheckCurrentDictionary().

And I believe that is needed.

Comment 33

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution: CheckCurrentDictionary() gutted but not removed.

Here's a less aggressive approach, the same as in comment #13. Back then, the try was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f1b22ee55aa
We shouldn't have culled the call to we removed the call to mozSpellChecker::CheckCurrentDictionary(). It fixes up some internals when a dictionary gets removed.

While I had the box open, I made two more changes:
1) Replaced the internal calls to SetCurrentDictionary with calls to
   TryDictionary.
2) In TryDictionary, also do not call nsEditorSpellCheck::SetCurrentDictionary
   but instead call mozSpellChecker::SetCurrentDictionary directly.
   This is what nsEditorSpellCheck::SetCurrentDictionary does anyway.

Ultimately, I'd like to remove that RAII stuff, so UpdateDictionaryHolder, etc. But I'll do this in another bug, if at all.

Anyway, I'm sending this off for another run (together with 1204540 and bug 772796):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9998db64bbb

Comment 34

[Jorg K (CEST = GMT+2)]

Didn't compile, again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4545a180602

Comment 35

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4545a180602 is (almost) all green now.

Note: It looks like only the patch for bug 772796 was submitted to try. It's always confusing, since the unmodified patches that were sent a few minutes before in
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9998db64bbb
didn't get listed again.

Comment 36

[Jorg K (CEST = GMT+2)]

An interdiff
https://bugzilla.mozilla.org/attachment.cgi?oldid=8662344&action=interdiff&newid=8663655&headers=1
shows the differences between the new proposal and the patch that I had backed out.

- nsEditorSpellCheck::CheckCurrentDictionary() is back
- call mozSpellChecker::SetCurrentDictionary directly (further clean-up)
- use TryDictionary() internally (further clean-up).

I can tell you why I had it backed out: I had forgotten to remove nsEditorSpellCheck::CheckCurrentDictionary() from /editor/txtsvc/nsISpellChecker.h.
Removing to that resulted in a compile error here:
http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozSpellChecker.h#51
due to the override. Then I considered to also remove
mozSpellChecker::CheckCurrentDictionary()
http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozSpellChecker.cpp#432

But I stopped when I saw "mSpellCheckingEngine = nullptr;" at:
http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozSpellChecker.cpp#452

So some internal structures *are* cleaned up when the dictionary has been removed.

I know it worked after the complete removal, but I just wasn't comfortable with my change. Hence backing it out and then redoing it less aggressively. I hope you agree.

I'm really sorry about this, Ehsan was right, the moment you touch it, there is always one problem more. But I'm in good faith that we are "converging" to a final solution here.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8663655 [details] [diff] [review]
Proposed solution: CheckCurrentDictionary() gutted but not removed.

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

This does look better.

Please make sure that in future you have a successful try run before requesting checkin-needed on anything.

::: editor/libeditor/nsEditor.cpp
@@ +1327,5 @@
>    SyncRealTimeSpell();
>  
>    // When nsIEditorSpellCheck::GetCurrentDictionary changes
>    if (mInlineSpellChecker) {
> +    // Do the right thing in the spellckecker, if the dictionary is no longer

spellchecker

Comment 38

[Jorg K (CEST = GMT+2)]

Thanks. Please note that there *IS* as successful try run, see comment #35.

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution: CheckCurrentDictionary() gutted but not removed. Typo corrected.

Carrying forward Robert's (roc) r+. Corrected typo.

Comment 40

[Jorg K (CEST = GMT+2)]

Dear Sheriff,

Please land bug 1193293 and bug 1204540 together. Please apply the patch from bug 1193293 first.

Please note: This patch changes an IDL file. However, only a comment has changed.
No interface was modified and no UUID needs to change.

If - as in comment #25 - you receive a push failure
  Push rejected because the following IDL interfaces were modified
  without changing the UUID: ...
please include 'IGNORE IDL' in the commit message.

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6212d71ed93

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c6212d71ed93