Open the enclosed page. There is a textarea followed by a <div contenteditable>. Click on the textarea. Having compiled editor/composer/nsEditorSpellCheck.cpp with DEBUG_DICT set, I get this debug: ***** mPreferredLang (element) |en| ***** Assigned from element/doc |en| ***** Setting of |en| failed (or it wasn't available) ***** Trying preference value |en-GB| since it matches language code ***** Writing content preferences for |en-GB| ***** Storing spellchecker.dictionary |en-GB| As one can clearly see, the content preference is set and also the "spellchecker.dictionary" preference is set, just by visiting a page and without any further user interaction. This is REALLY bad, since we advertised, that content preferences are only written when the user sets a dictionary. Also "spellchecker.dictionary" should only be set at that time. Further analysis: Here is the callstack when that case happens: nsEditorSpellCheck::SetCurrentDictionary() Line 616 C++ nsEditorSpellCheck::CheckCurrentDictionary() Line 657 C++ nsEditor::Observe() Line 1336 C++ nsObserverList::NotifyObservers() Line 114 C++ nsObserverService::NotifyObservers() Line 319 C++ mozHunspell::SetDictionary() Line 236 C++ mozSpellChecker::SetCurrentDictionary() Line 412 C++ nsEditorSpellCheck::SetCurrentDictionary() Line 636 C++ nsEditorSpellCheck::TryDictionary() Line 765 C++ nsEditorSpellCheck::DictionaryFetched() Line 917 C++ <====== DictionaryFetcher::HandleCompletion() Line 137 C++ CheckCurrentDictionary() is called and inside it GetCurrentDictionary() is called with return an empty one, so it picks the first from the list, in this case en-GB, and sets it. Weird is that nsEditor::Observe() does NOT call nsEditorSpellCheck::CheckCurrentDictionary() directly, it calls mInlineSpellChecker->SpellCheckRange(nullptr); and this comes from extensions/spellcheck/src/mozInlineSpellChecker.cpp, however, this doesn't show up on the stack. Oh, I see, the spell check is run asynchronously. It is also weird that this happens, since nsEditorSpellCheck::DictionaryFetched() is on the stack and there is a gate around setting the content preferences that has the following comment: // The purpose of mUpdateDictionaryRunning is to avoid doing all of this if // UpdateCurrentDictionary's helper method DictionaryFetched, which calls us, // is on the stack. In other words: Only do this, if the user manually selected a // dictionary to use. if (!mUpdateDictionaryRunning) { However, the debugger shows that mUpdateDictionaryRunning is false. This must have to do with the fact that there are two editors on the page, a text area and a <div contenteditable>.

This is really bad. On another bug someone said that content preferences and "spellchecker.dictionary" got written just by visiting a site, and I didn't want to believe it. But here I have the proof. All our good clean-up work is in vain and the attempt to explain to the user what's going on, since due to this bug here, content preferences and "spellchecker.dictionary" are written where they shouldn't be written. What seems to happen is that nsEditorSpellCheck::DictionaryFetched() is called to retrieve the dictionary of an element. It selects en-GB, since the element "en". Then it calls into the Hunspell stuff, with in turn notifies the observers. One observer calls CheckCurrentDictionary(), it decides that the current dictionary is empty and sets the first one from the list, en-GB, and that really gets set, with content preferences and everything. Somehow the gate failed, since, if at 3:00 AM I read correctly, there is one gate per editor, but here we have two editors in the test case. By the looks of it, while setting the dictionary for element A, the observer of element B 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. Or maybe I'm interpreting it wrong. This is not a regression we caused, this can be reproduced with the current FF 40. This bug may well be a root cause of the confusion surrounding the spell checking. Many pages have more than one editor, so this case is not uncommon. What do you say?

Sorry, nsEditorSpellCheck::CheckCurrentDictionary() *is* called from the observer at: https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditor.cpp#1332 Not at line 1336 as shown in the stack above.

Robert, can you please review this obvious one line fix to a fatal problem. Obviously, when implementing content preferences in bug 678842 this was overlooked. See bug 678842 comment #17 and http://hg.mozilla.org/mozilla-central/diff/a624f57a9e6f/editor/composer/src/nsEditorSpellCheck.cpp The call to SetCurrentDictionary is missing the gate, which has fatal consequences. My bet is that this is the root cause to much dictionary confusion, since this stores content preferences *at random*. This fix shouldn't affect anything, since nothing should rely on this faulty behaviour. I'm trying it anyway, just to be sure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a275329c3ad5

Oops. Fixed spelling of patch comment.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a275329c3ad5 is completely green.

STR, you don't need a debug build or anything: - Remove content-prefs.sqlite from your profile. - Start FF and open attachment 8660200 [details]. - Click into the textarea. - Now inspect content-prefs.sqlite with your favourite SQLite viewer. You will find bug1204147.bmoattachments.org in the groups table.

Comment on attachment 8660259 [details] [diff] [review] Obvious fix to a fatal problem. Review of attachment 8660259 [details] [diff] [review]: ----------------------------------------------------------------- Please supply a test. Thanks!!!

I was afraid you'd say that. Sorry, but do we need a test for every obvious coding problem in the system? It's almost a miracle that this regression from bug 678842 was detected at all after four years! This event driven (focus) and asynchronous (spell check) stuff is fiendish complicated, and as you can see in bug 697981 (working on which discovered this), tests that run on Windows might not run on Linux due to some timing issues. Here we have the problem of two observers being triggered, one of them isn't quite ready, so if runs through the erroneous code. How can I guarantee that the same order of execution is taking place in the test as in my manual debugging? I'll see what I can do.

The test doesn't have to always show the problem on every platform. It just has to sometimes show the problem on at least one platform.

On Windows, the test fails if the code patch is *not* applied. Debug and inspection with the SQLite browser show the content preference was written. With the code patch, the test passes. Sadly, the test is rather quirky, since the <div contenteditable> badly spits into our broth. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87cf383c7eb3

Comment on attachment 8660593 [details] [diff] [review] Test case. Review of attachment 8660593 [details] [diff] [review]: ----------------------------------------------------------------- Great!

Carrying forward Robert's r+. Robert: Thanks! Changed the introductory comment (use interdiff to see it).

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=87cf383c7eb3 is green. Test is provided. Everyone is happy. Let's squash this nasty bug and land the two patches. Dear Sheriff, no special instructions here. Apply in any order.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3994f4ac9df https://hg.mozilla.org/integration/mozilla-inbound/rev/361c6d3c4678

If you look at bug 1204540 comment #2 you can see that this fix here fixes five more cases of content preferences being written erroneously in our editor/composer/test test suite (mochitest-o "other").

https://hg.mozilla.org/mozilla-central/rev/a3994f4ac9df https://hg.mozilla.org/mozilla-central/rev/361c6d3c4678