Last Comment Bug 307052 - Spellchecker doesn't display suggestion list for misspelled words if Russian Spell dictionary is installed
: Spellchecker doesn't display suggestion list for misspelled words if Russian ...
Status: VERIFIED FIXED
[nvn-dl]
: fixed1.8.1, regression, verified1.8.0.2, verified1.8.1.3
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: 1.8 Branch
: x86 Windows XP
: -- major with 9 votes (vote)
: mozilla1.9alpha1
Assigned To: Scott MacGregor
:
Mentors:
Depends on:
Blocks: 240600 319707 319709 319710
  Show dependency treegraph
 
Reported: 2005-09-04 12:44 PDT by Alexander L. Slovesnik
Modified: 2007-04-03 13:03 PDT (History)
11 users (show)
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot of working ru spellcheck (2005-01-11-06 build) (12.39 KB, image/png)
2005-09-11 13:14 PDT, Alexander L. Slovesnik
no flags Details
Working Bulgarian spellchecker (2005-01-11-06-trunk) (12.47 KB, image/png)
2005-09-11 13:20 PDT, Alexander L. Slovesnik
no flags Details
Broken Bulgarian spellchecker (2005-01-13-06-trunk) (12.33 KB, image/png)
2005-09-11 13:22 PDT, Alexander L. Slovesnik
no flags Details
a fix (8.95 KB, patch)
2005-12-09 13:37 PST, Scott MacGregor
no flags Details | Diff | Review
updated fix (9.21 KB, patch)
2005-12-09 13:53 PST, Scott MacGregor
no flags Details | Diff | Review
update to fix the double free in ConvertName (8.90 KB, patch)
2005-12-09 18:28 PST, Scott MacGregor
mvl: review+
bzbarsky: superreview+
dveditz: approval1.8.0.1+
dveditz: approval1.8.1+
Details | Diff | Review

Description Alexander L. Slovesnik 2005-09-04 12:44:30 PDT
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 2005-09-04 14:09:36 PDT
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 2005-09-06 08:44:04 PDT
(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 2005-09-11 12:22:53 PDT
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 2005-09-11 12:39:23 PDT
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@) 2005-09-11 12:41:34 PDT
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 2005-09-11 13:12:44 PDT
(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 2005-09-11 13:14:06 PDT
Created attachment 195661 [details]
Screenshot of working ru spellcheck (2005-01-11-06 build)
Comment 8 Alexander L. Slovesnik 2005-09-11 13:20:32 PDT
Created attachment 195662 [details]
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 2005-09-11 13:22:48 PDT
Created attachment 195664 [details]
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 [:bz] (Out June 25-July 6) 2005-12-08 21:47:24 PST
> 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 neil@parkwaycc.co.uk 2005-12-09 01:23:57 PST
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 2005-12-09 13:37:28 PST
Created attachment 205417 [details] [diff] [review]
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 2005-12-09 13:39:12 PST
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 2005-12-09 13:49:08 PST
This fix does indeed fix the problem with the russion dictionary for me.
Comment 15 Scott MacGregor 2005-12-09 13:53:03 PST
Created attachment 205422 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2005-12-09 15:44:56 PST
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 2005-12-09 18:28:15 PST
Created attachment 205459 [details] [diff] [review]
update to fix the double free in ConvertName
Comment 18 Scott MacGregor 2005-12-09 18:32:03 PST
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 [:bz] (Out June 25-July 6) 2005-12-09 22:05:31 PST
> 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 2005-12-09 22:08:00 PST
(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 [:bz] (Out June 25-July 6) 2005-12-09 22:11:30 PST
Ah, ok.  That makes sense, then.
Comment 22 Andras Timar 2005-12-10 01:23:13 PST
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@) 2005-12-11 04:20:31 PST
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 2006-01-03 13:45:18 PST
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 [:bz] (Out June 25-July 6) 2006-01-04 22:20:15 PST
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 2006-01-05 11:11:33 PST
fixed on the trunk.
Comment 27 Scott MacGregor 2006-01-06 13:00:22 PST
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] 2006-01-06 13:03:59 PST
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 2006-01-06 13:08:36 PST
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 [:bz] (Out June 25-July 6) 2006-01-06 15:35:12 PST
Daniel, there are other consumers of the 1.8 branch spellchecker than Thunderbird (Nvu and Seamonkey come to mind).
Comment 31 Daniel Veditz [:dveditz] 2006-01-09 11:58:38 PST
Comment on attachment 205459 [details] [diff] [review]
update to fix the double free in ConvertName

a=dveditz for drivers
Comment 32 Scott MacGregor 2006-01-10 12:38:51 PST
fixed on both branches. Clearing the 1.9a1 blocking flag since it's already fixed on the trunk. 
Comment 33 Scott MacGregor 2006-02-27 14:12:43 PST
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 2006-02-27 14:39:48 PST
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 2006-03-03 17:33:35 PST
1.8.0.2 is technically the first release with this Thunderbird fix. 
Comment 36 Tracy Walker [:tracy] 2006-03-16 10:10:34 PST
verified per comment #34 - just adding whiteboard note
Comment 37 Bob Clary [:bc:] 2007-04-03 13:03:11 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.