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).

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

I guess it is a duplicate of bug 1204147.

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.

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.

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.

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.

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.

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 ;-)

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.

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

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

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?

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 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?

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.

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 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.

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.

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?

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).

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 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 :-).

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 ?

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.

Requested backing out of this patch.

(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

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

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.

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

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

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.

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 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

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

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

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.