307052 - Spellchecker doesn't display suggestion list for misspelled words if Russian Spell dictionary is installed

Status: VERIFIED FIXED

(Core :: Spelling checker, defect)

Description

[Alexander L. Slovesnik]

Steps to reproduce:
1. Install latest Thunderbird Branch Build (I've tested on Mozilla/5.0 (Windows;
U; Windows NT 5.1; ru; rv:1.8b4) Gecko/20050901 Thunderbird/1.0+ ID:2005090112
and  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050904
Thunderbird/1.4 ID:2005090406)
2. Install Russian Spell Checker from
http://downloads.mozdev.org/dictionaries/spell-ru.xpi
3. Select Russian in Tools - Options - Composition - Spelling.
4. Go to File - New - Message and write misspelled word in Russian.
5. Select this word (it's underlined with red line if "spell as you type is
enabled) and right-click on it or go to Options - Spell Checking...

Actual results:
Thunderbird shows "No suggestions found"

Expected results:
Thunderbird should suggest list of words instead misspelled one.

Additional information:
This bug doesn't exist in Thunderbird 1.0.6
This bug doesn't exist with en-US spell checker.
This bug also exists in Seamonkey (Mozilla/5.0 (Windows; U; Windows NT 5.1;
ru-RU; rv:1.9a1) Gecko/20050904 SeaMonkey/1.1a)

Comment 1

[Adam Guthrie]

AFAIK we only supports the en-US dictionary. All other problems with
dictionaries should be taken up with their respective owners. See bug 296123
comment 1.

Comment 2

[Alexander L. Slovesnik]

(In reply to comment #1)
> AFAIK we only supports the en-US dictionary. All other problems with
> dictionaries should be taken up with their respective owners. See bug 296123
> comment 1.
This bug is not about "some words are missing in spell dictionary". This bug is
about "Thunderbird 1.0+/1.4 is not working with Russian Spell dictionary at
ALL". Thunderbird 1.0+/1.4 is not displaying suggestion list for misspelled
russian words. Never.
This is clearly a regression, cause Thunderbird 1.0.6 works pretty well with
Russian Spell dictionary.
And I never have seen in Thunderbird FAQ or any other Mozilla documentation that
only en-US spell dictionary is supported. Are you want to say that Thunderbird
developers should care only about compatibility with en-US spell checker and
free to break compatibility with all other spell checkers?
I'll try to find regression window on this week.

Comment 3

[Alexander L. Slovesnik]

OK, I found regression window.
http://archive.mozilla.org/pub/thunderbird/nightly/2005-01-12-06-trunk/ doesn't
have this bug.
http://archive.mozilla.org/pub/thunderbird/nightly/2005-01-13-06-trunk/ have
this bug.

Looking at regression window, it looks like caused by Bug 240600

Comment 4

[Adam Guthrie]

I just tested this out with the Spanish (Mexican) dictionary and it works fine
(gives me the correct suggestion, etc.). So this must be a problem with the
Russian dictionary itself, which in that case, it's not Thunderbird's problem,
but rather the person who made the dictionary's. 

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Or it is a problem with dictionaries in other charsets, in wich case it is a
mozilla problem.
Alexander, can you give an example of a russian word that should give a suggestion?

Comment 6

[Alexander L. Slovesnik]

(In reply to comment #5)
> Alexander, can you give an example of a russian word that should give a
suggestion?

Sure. For example enter word "кройний" and spellcheck it. Proposed replacement
is "крайний". I'll post screenshot.

Comment 7

[Alexander L. Slovesnik]

Attachment: Screenshot of working ru spellcheck (2005-01-11-06 build)

Comment 8

[Alexander L. Slovesnik]

Attachment: Working Bulgarian spellchecker (2005-01-11-06-trunk)

I've downloaded and checked Bulgarian spell checker. It has same error.
Posting screenshots.

Comment 9

[Alexander L. Slovesnik]

Attachment: Broken Bulgarian spellchecker (2005-01-13-06-trunk)

Broken Bulgarian spellchecker (2005-01-13-06-trunk)
FYI, Bulgarian is using cyrillic alphabet, that's why I'm posting it.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Or it is a problem with dictionaries in other charsets

I bet this is it.  The patch for bug 240600 has the following in mozMySpell::Suggest :

336   *aSuggestionCount = mMySpell->suggest(&wlst, NS_LossyConvertUCS2toASCII(aWord).get());

That'll work for ASCII or ISO-8859-1, but will absolutely fail for any other encoding.  Shouldn't we be using UTF8 here?  I guess that will break all the ISO-8859-1 dictionaries...  What encoding are myspell dictionaries defined to be in?

We should really fix this on branch, imo...

Comment 11

[[e-mail]]

You probably have to clone/break out/rewrite the conversion code from mozMySpell::Check, and whomever does that should of course fix the leak...

Comment 12

[Scott MacGregor]

Attachment: a fix

I haven't tested this on the russian dictionary yet. 

I fixed (hopefully) a bunch of memory leaks that were listed in individual bugs: Bug 319710, Bug 319709, Bug319707 . I stepped through the various failure points in the debugger by hand to verify that the arrays and strings were getting released properly.

mvl, what do you think of this?

Comment 13

[Scott MacGregor]

Comment on attachment 205417 [details] [diff] [review]
a fix

weird, bugzilla didn't show me a list of possible matches for your name.

Comment 14

[Scott MacGregor]

This fix does indeed fix the problem with the russion dictionary for me.

Comment 15

[Scott MacGregor]

Attachment: updated fix

the function prototype in the mozMySpell class had the class name scope attached to it by accident. (mozMySpell::ConvertCharset --> ConvertCharset in mozMySpell.h).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Doesn't ConvertCharset() double-free if Convert() fails?  It'll free, and then the nsXPIDLCString on the caller's end will free...

Comment 17

[Scott MacGregor]

Attachment: update to fix the double free in ConvertName

Comment 18

[Scott MacGregor]

Axel and I talked this over and we aren't going to respin Thunderbird 1.5 for this. We don't ship the l10n dictionary files by default, users have to go install them and this regression only effects a few of the locales.

But we would like to see it go into 1.8.0.1 after a reviewed patch bakes for a while on the trunk for 1.5.0.1. 

We'll be releasing noting this for the handful of locales (Russian, Bulgarian, Greek, Hebrew, etc) which are effected by this.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> only effects a few of the locales.

I'd expect it to affect at least (from the 1.0.x localization list):

Chinese, Greek, Gujarati, Hebrew, Japanese, Korean, Punjabi, Russian, Turkish.

Not sure how this matches up with the list of 1.5 localizations, of course.  Also not sure what our actual userbase looks like (eg I know there're a lot of Firefox users in Japan, but I dunno about Thunderbird).

Comment 20

[Scott MacGregor]

(In reply to comment #19)
> > only effects a few of the locales.
> 
> I'd expect it to affect at least (from the 1.0.x localization list):
> 
> Chinese, Greek, Gujarati, Hebrew, Japanese, Korean, Punjabi, Russian, Turkish.

There are no dictionaries available for Chinese, Japanese and Korean and thus this issue does not apply to those locales.

 

Comment 21

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  That makes sense, then.

Comment 22

[Andras Timar]

On the long run a good solution might be to migrate to hunspell which will the successor of myspell in OpenOffice.org code from version 2.0.2. Hunspell is backward compatible with myspell dictionaries but can handle Unicode and has other good enhancements over the myspell codebase. http://hunspell.sourceforge.net/

Comment 23

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 205459 [details] [diff] [review]
update to fix the double free in ConvertName


>-  nsresult rv = mEncoder->GetMaxLength(aWord, inLength, &outLength);
>-  // NS_ERROR_UENC_NOMAPPING is a NS_SUCCESS, no error.
>-  if (NS_FAILED(rv) || rv == NS_ERROR_UENC_NOMAPPING) {
>-    // not a word in the current charset, so likely not
>-    // a word in the current language
>-    return PR_FALSE;
>-  }

That check for NS_ERROR_UENC_NOMAPPING seems to be gone now. Is it not needed anymore?

Rest looks good. r=mvl

Comment 24

[Scott MacGregor]

Comment on attachment 205459 [details] [diff] [review]
update to fix the double free in ConvertName

bz since he showed original interest in this bug. I can find someone else if your too busy.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 205459 [details] [diff] [review]
update to fix the double free in ConvertName

This is good stuff, Scott!

sr=bzbarsky

Comment 26

[Scott MacGregor]

fixed on the trunk.

Comment 27

[Scott MacGregor]

Triage Team: I've verified that the patch is working on a trunk release build from this morning. Using a russian dictionary, the spell checker is now properly detecting misspelled words.

Comment 28

[Daniel Veditz [:dveditz]]

Is this going to be accepted for the Thunderbird 1.5 release? If so then maybe it should go in. If it's not then I don't see why we'd need it on the 1.8.0 branch in time for .0.1, and could wait until 0.2 and get more testing

Comment 29

[Scott MacGregor]

Dan, we didn't have time nor the resources to respin thunderbird 1.5 for this bug but it's a prety bad bug for cyrillic langauges and we would have liked to have taken it. 

The code change itself is to the spell checker which isn't built for Firefox so the testing impact for this is limited to Thunderbird only and if we had the resources we would have tried to respin 1.5 for it. Asking again just in case, in light of this not effecting Firefox.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Daniel, there are other consumers of the 1.8 branch spellchecker than Thunderbird (Nvu and Seamonkey come to mind).

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 205459 [details] [diff] [review]
update to fix the double free in ConvertName

a=dveditz for drivers

Comment 32

[Scott MacGregor]

fixed on both branches. Clearing the 1.9a1 blocking flag since it's already fixed on the trunk. 

Comment 33

[Scott MacGregor]

Alexander, do you want to verify that the fix is working in the 1.5.0.2 test builds?

The russian translation can be found here:

ftp://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-mozilla1.8.0-l10n

Thanks!

Comment 34

[Alexander L. Slovesnik]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.8.0.1) Gecko/20060227 Thunderbird/1.5.0.2 ID:2006022704
Thanks a lot, Scott.

Comment 35

[Scott MacGregor]

1.8.0.2 is technically the first release with this Thunderbird fix. 

Comment 36

[Tracy Walker [:tracy]]

verified per comment #34 - just adding whiteboard note

Comment 37

[Bob Clary [:bc] (inactive)]

verified tb2.0 rc2 linux with ru dictionary. Note had to disable extension checking since the ru dictionary is not compatible with 2.0.0.0.