717433 - Chosen dictionary intermittently resets from en-GB to en-US after session restart

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Ed Morley [:emorley]]

Attachment: about:support

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120111 Firefox/12.0a1
See attachment for about:support dump.

Installed dictionaries: Only en-GB (British English Dictionary addon v1.19.1)

I haven't been able to get it to occur consistently, but STR along the lines of:
1) In a gmail app-tab start new draft email
2) Type a word that has a different spelling in the US eg neighbour
3) Spot that the word is marked as misspelt, check context menu - find it is set to US English
4) Set back to UK English
5) Finish email + send, return to gmail inbox
6) Carry on browsing, restart session at some point.

Expected:
Once set to en-GB, preference is remembered even after Nightly restart.

Actual:
~10-20% of the time the pref is reset back to en-US after session restart.

Comment 1

[Paul [pwd]]

I'm seeing the same thing.

Comment 2

[(no longer active)]

Ed, did you get a chance to debug this?

Comment 3

[Andrew Quartey [:drexler]]

I will look into this.

Comment 4

[Ed Morley [:emorley]]

Sorry for no rely so far - I was having great trouble identifying when I'd successfully repeated the STR, since the change only seemed to become apparent on Firefox restart. Inspection of prefs.js after app exit, showed the pref had changed from en-GB to en-US - so I decided to annotate parts of mozHunspell::SetDictionary in a local debug build to be able to see in the console when I'd successfully recreated the STR, but then struggled to get it to do so in my local build :-( 

I also tried doing what ehsan suggested and setting a breakpoint on mozHunspell::SetDictionary, but my lack of RAM/ancient CPU makes MSVC debugging pretty much unusable :-(( 
(even ignoring the incessant assertions on startup and on gmail, eg bug 714398 - how people put up with debug builds is beyond me - though maybe things are less annoying on an i7 or something)

Comment 5

[Andrew Quartey [:drexler]]

I couldn't reproduce this with the latest nightly builds. Paul, are you able to get this still?

Comment 6

[Matt Price]

I'm seeing something similar -- a switch from en-CA to fr-BE, happening over and over again.  Using 12.0~b3+build1-0ubuntu0.12.04.1~mfn2 (so, an ubuntu-built package) on ubuntu precise.  inspection of prefs.js shows 
user_pref("spellchecker.dictionary", "fr_BE");

But about:config shows:
spellchecker.dictionary;en_CA

not sure what the issue is...

Comment 7

[M** A****]

Seeing similar problem in latest 64 bit nightly. 38.0a1
STR:
Change dictionary to English Australia. 
Shut browser down
Restart Browser 
Dictionary has reverted to en-us

Comment 8

[Matt Price]

I also continue to see this problem, with current FDE and also with Firefox Beta 36.0b2 (both on Arch linux)

Comment 9

[Jorg K (CEST = GMT+2)]

This was analysed in bug 1200533. The problem is that en-AU or en-GB should be stored as so-called "content preference", however, these only get stored if the dictionary is set to another language. So setting de-DE or es-ES on an "en" site sticks, setting en-AU and en-GB is explicitly discarded. Code here:
https://dxr.mozilla.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#616

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution

(In reply to Ehsan Akhgari (don't ask for review please) from comment #2)
> Ed, did you get a chance to debug this?

I'm not Ed, but yes, this has been analysed.

Perhaps, despite Ehsan's general veto to touch this code, we can land a small fix here that affects many English speakers who prefer "en-GB" or "en-AU" over "en-US".

Comment 11

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

Comment on attachment 8655814 [details] [diff] [review]
Proposed solution

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

I agree

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #2)
> Ed, did you get a chance to debug this?

Comment 13

[Lalo Martins]

Jorg, does the patch also cover the case where some other non-"en" language is propagated to the default (via the preference), causing it to pop up on all windows with neither content pref or lang attribute? E.g. in the situation described in the reproduction, where you also have the set of "de" dictionaries installed, and visit a "de" site?

(There's also the fact that those sites pick a “random” de, usually de-at, even if I tell them I want de-de — but this I think should be covered by the patch)

Comment 14

[Jorg K (CEST = GMT+2)]

The patch covers exactly the problem described. A specific en-XX choice gets reverted to en-US. Of course similar examples exist for de-XX being reverted to de-YY.

When you first visit an "en" site, an en-XX dictionary will be chosen at random, equally for a "de" site, perhaps de-AT. If you changed this, before this user selection was discarded, now it will stick. 

The other thing you're complaining about, if I understand it correctly, is not part of the bug.

The following problem still persists:
Have two windows, window 1 and window 2. In window 2 show a site that has no "lang" attribute and where no content preference has been set. So this site uses a fallback, typically from the preference "spellchecker.dictionary" (or if not set, from the locale).

Now in window 1, change the language. This will currently always set "spellchecker.dictionary". If you now return to window 2, the language will change to the one set in window 1 based on the preference.

This is (sadly) established behaviour. Changing this behaviour was subject to bug 1073840(*), but that has met too much opposition. I am hoping to fix up the fallback behaviour a little in bug 1200533, but the latter will maintain the established behaviour.

(*) The aim of bug 1073840 was *not* to change "spellchecker.dictionary" every time a dictionary was set elsewhere.

Comment 15

[Pulsebot]

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

Comment 16

[(no longer active)]

Can you please add a test for this?  test_add_remove_dictionaries.xul shows you how you can install dictionaries in a test and helper_bug1170484.js used in test_bug1170484.html shows you how to click on the spell checking context menu to change the language (helper_bug1170484.js shows how to pick a spelling suggestion from the context menu which should be similar.)

Comment 17

[Jorg K (CEST = GMT+2)]

I can, but only if do you a (kind) review of bug 1200533 for me (keeping in mind that the first part of the patch already landed here) ;-)

Since it's doable to add dictionaries while testing, bug 1200533 should be fully testable.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: WIP: test case

Sadly, I need your help. Since I need to load the page twice, I took a copy of test_bug678842.html and its helper bug678842_subframe.html.

I created a fake en-GB dictionary and adapted this stuff from test_add_remove_dictionaries.xul:
+  var dir = Components.classes["@mozilla.org/file/directory_service;1"].
+            getService(Components.interfaces.nsIProperties).
+            get("CurWorkD", Components.interfaces.nsIFile);
+  dir.append("chrome");
+  dir.append("editor");
+  dir.append("composer");
+  dir.append("test");
+
+  var hunspell = Components
+    .classes["@mozilla.org/spellchecker/engine;1"]
+    .getService(Components.interfaces.mozISpellCheckingEngine);
+
+  // install en-GB dictionary
+  var en_GB = dir.clone();
+  en_GB.append("en-GB");
+  ok(en_GB.exists());
+  hunspell.addDirectory(en_GB);

Sadly it already dies early on:
JavaScript error: resource://gre/modules/FileUtils.jsm, line 63: NS_ERROR_FAILUR
E: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties
.get]
0 INFO SimpleTest START
1 INFO TEST-START | editor/composer/test/test_bug717433.html
2 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug717433.html | undefin
ed assertion name
loadListener@chrome://mochitests/content/chrome/editor/composer/test/test_bug717
433.html:47:1
EventListener.handleEvent*@chrome://mochitests/content/chrome/editor/composer/te
st/test_bug717433.html:78:1
3 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug717433.html | uncaugh
t exception - TypeError: elem is null at chrome://mochitests/content/chrome/edit
or/composer/test/test_bug717433.html:52

So I wonder whether this needs to be a XUL test?

Also, on the screen I can see a quick
  404 Not Found
  /tests/editor/composer/test/bug717433_subframe.h
going past.

Any hints would be appreciated. I'm not asking for review ;-) ... yet.

Comment 19

[Jorg K (CEST = GMT+2)]

Forget that. The
JavaScript error: resource://gre/modules/FileUtils.jsm, line 63: NS_ERROR_FAILUR
E: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties
.get]
comes up in other tests.

Also, I need to register the bug717433_subframe.htlm as a support file.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: WIP: test case (v2)

Sadly, I need your help. Since I need to load the page twice, I took a copy of test_bug678842.html and its helper bug678842_subframe.html.

I created a fake en-GB dictionary ...
   Note:
   The files seem to need an underscore, see here:
   https://dxr.mozilla.org/mozilla-central/source/extensions/spellcheck/tests/chrome/base
   to get produce a dictionary with a hyphen in the end, see here:
   https://dxr.mozilla.org/mozilla-central/source/extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.xul#71

... and adapted this stuff from test_add_remove_dictionaries.xul.

I have trouble loading the dictionary. Do I need to do this in XUL or can I do this in JS?
If the latter, how do I address the directory?

Also, the ok() function that was in the XUL file
https://dxr.mozilla.org/mozilla-central/source/extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.xul#57
didn't seem to work in the JS version:

2 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug717433.html | undefined assertion name
loadListener@chrome://mochitests/content/chrome/editor/composer/test/test_bug717433.html:47:1
(before at line 47 I had: ok(en_GB.exists());)

Please help.

Comment 21

[(no longer active)]

I really don't have enough bandwidth these days.  Please ask roc.  He should have not r+'ed without a test.  ;-)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: Test for the patch (v1)

Here's the test so we don't get into trouble with Ehsan ;-)

Limited try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=130f54e04540
(no idea how I managed to push 19 changesets, I intended to push two, see whether it works).

I should say that I copied this from test_bug678842.html and test_add_remove_dictionaries.xul.
So if it's not right, I blame it on the original author(s) ;-)

Comment 24

[Lalo Martins]

I don't know if this is even the right bug to answer your question, but I'm getting kind of confused with all the activity, thanks a lot by the way for wading into this.

(In reply to Jorg K (GMT+2) from comment #14)
> The other thing you're complaining about, if I understand it correctly, is
> not part of the bug.
> 
> The following problem still persists:
> Have two windows, window 1 and window 2. In window 2 show a site that has no
> "lang" attribute and where no content preference has been set. So this site
> uses a fallback, typically from the preference "spellchecker.dictionary" (or
> if not set, from the locale).
> 
> Now in window 1, change the language. This will currently always set
> "spellchecker.dictionary". If you now return to window 2, the language will
> change to the one set in window 1 based on the preference.

Almost, but not quite. Let me try to put it into these terms.

- In window 1, visit a site with no lang attribute and no content preference. It uses the fallback, which, let's just assume, will be the correct dictionary.
- In window 2, visit a site with a "lang" attribute of "de". This sets "spellchecker.dictionary" to "de". Both windows at this point have the correct dictionary set (I think).
- Restart the browser (with "Show my windows and tabs from last time").
- Now both windows have some "de-XX" dictionary selected.
- Bonus: if I then select "en-gb" on window 1, and restart the browser again, window 1 will be back to "de-XX".

Can probably also be replicated by closing window 1 and opening the same site again on window 3. I'm not willing to test right now (I have a dirty work-around in place) but I can if you say it would help.

Comment 25

[Jorg K (CEST = GMT+2)]

This bug is not a good place to discuss anything any more, the fix has landed, and as soon as the test lands, this will be marked fixed.

Also, your description is sadly wrong:
Window 1 will use a fallback, most likely the last value of "spellchecker.dictionary".
If in window 2 you visit a site with lang="de", this WILL NOT set the "spellchecker.dictionary".

"spellchecker.dictionary" will only be set when you actively select another dictionary via the right-click (context) menu. As detailed before, this can affect all other windows that are based on "spellchecker.dictionary". The "spellchecker.dictionary" is always set to a full dictionary name, never the language code ("de") alone. This behaviour won't change.

My remaining fixes are in bug 1200533. However, what you're describing is not covered there, so there would also not be the right place to comment.

If you want to be heard, please give a 100% reproducible description of what you are experiencing and put it into the meta-bug 1073827. From there, we can allocate it to the right bug.

I repeat: The problem must be 100% reproducible, you should remove/rename your content-prefs.sqlite before you start, or start on a new profile (start Firefox with the -p option). Please specify all the dictionaries you have installed and all the websites you visit.

Comment 26

[Lalo Martins]

Thanks.

My list of installed dictionaries won't change any time soon, so with a fresh profile, I can reproduce this easily at any point.

To avoid creating redundant work for you, what I'll do is I'll wait for your batch of fixes to land, then build that tree, and see if it still happens. I have a feeling it won't, because what I'm seeing might be a side-effect of one of the things you're fixing. If it does happen, I'll give all the detail I can produce.

Comment 27

[Jorg K (CEST = GMT+2)]

The fix has landed (comment #22). Of course you can use a Nightly build in a day or so.
Sadly my try server build was for Linux only.

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: Test for the patch (v2)

Need to remove en-GB dictionary after use.
If not, later tests use it and fail, since it's a fake dictionary with nothing in it.

New try run (just for the "oth" test that failed before):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d83581b40d5

Comment 29

[Jorg K (CEST = GMT+2)]

Oops, got the try run wrong, it's -o for "other", not -oth.
So here we go again with: try: -b o -p linux64 -u mochitest-o[x64] -t none
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd977eb0180f

Comment 30

[Jorg K (CEST = GMT+2)]

Try run green, so all good.

Dear Sheriff:

Please check in the second patch "Test for the patch (v2)".
The first part was already checked here: (comment #22)
https://hg.mozilla.org/mozilla-central/rev/c11d024e51fd

Sorry about the extra hassle.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ecb75931636

Comment 32

[(no longer active)]

Thanks for the test!

Comment 33

[Jorg K (CEST = GMT+2)]

And there will be many more tests in bug 1200533 ;-)

Comment 34

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

https://hg.mozilla.org/mozilla-central/rev/3ecb75931636

Comment 36

[Ed Morley [:emorley]]

Thank you for fixing this Jorg! :-)