Closed Bug 307052 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: Spelling checker, defect)

1.8 Branch
x86
Windows XP
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: unghost, Assigned: mscott)

References

Details

(4 keywords, Whiteboard: [nvn-dl])

Attachments

(4 files, 2 obsolete files)

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)
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.
Summary: 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 Spell dictionary is installed
(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.
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
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. 
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?
(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.
I've downloaded and checked Bulgarian spell checker. It has same error.
Posting screenshots.
Broken Bulgarian spellchecker (2005-01-13-06-trunk)
FYI, Bulgarian is using cyrillic alphabet, that's why I'm posting it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
> 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...
Blocks: 240600
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
You probably have to clone/break out/rewrite the conversion code from mozMySpell::Check, and whomever does that should of course fix the leak...
Attached patch a fix (obsolete) — Splinter Review
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?
Attachment #205417 - Flags: review?
Comment on attachment 205417 [details] [diff] [review]
a fix

weird, bugzilla didn't show me a list of possible matches for your name.
Attachment #205417 - Flags: review? → review?(mvl)
This fix does indeed fix the problem with the russion dictionary for me.
Attached patch updated fix (obsolete) — Splinter Review
the function prototype in the mozMySpell class had the class name scope attached to it by accident. (mozMySpell::ConvertCharset --> ConvertCharset in mozMySpell.h).
Attachment #205417 - Attachment is obsolete: true
Attachment #205422 - Flags: review?(mvl)
Attachment #205417 - Flags: review?(mvl)
Doesn't ConvertCharset() double-free if Convert() fails?  It'll free, and then the nsXPIDLCString on the caller's end will free...
Attachment #205422 - Attachment is obsolete: true
Attachment #205459 - Flags: review?(mvl)
Attachment #205422 - Flags: review?(mvl)
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.
Status: NEW → ASSIGNED
> 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).
(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.

 
Ah, ok.  That makes sense, then.
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 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
Attachment #205459 - Flags: review?(mvl) → review+
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.
Attachment #205459 - Flags: superreview?(bzbarsky)
Blocks: 319707
Blocks: 319710
Blocks: 319709
Keywords: regression
Attachment #205459 - Attachment description: updatee to fix the double free in ConvertName → update to fix the double free in ConvertName
Comment on attachment 205459 [details] [diff] [review]
update to fix the double free in ConvertName

This is good stuff, Scott!

sr=bzbarsky
Attachment #205459 - Flags: superreview?(bzbarsky) → superreview+
fixed on the trunk.
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.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
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.
Flags: blocking1.8.0.1- → blocking1.8.0.1?
Daniel, there are other consumers of the 1.8 branch spellchecker than Thunderbird (Nvu and Seamonkey come to mind).
Attachment #205459 - Flags: approval1.8.1?
Attachment #205459 - Flags: approval1.8.0.1?
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1+
Comment on attachment 205459 [details] [diff] [review]
update to fix the double free in ConvertName

a=dveditz for drivers
Attachment #205459 - Flags: approval1.8.1?
Attachment #205459 - Flags: approval1.8.1+
Attachment #205459 - Flags: approval1.8.0.1?
Attachment #205459 - Flags: approval1.8.0.1+
fixed on both branches. Clearing the 1.9a1 blocking flag since it's already fixed on the trunk. 
Flags: blocking1.9a1?
Target Milestone: --- → mozilla1.9alpha
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!
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.
Status: RESOLVED → VERIFIED
1.8.0.2 is technically the first release with this Thunderbird fix. 
Whiteboard: [nvn-dl]
verified per comment #34 - just adding whiteboard note
Whiteboard: [nvn-dl] → [nvn-dl] [qa:verified-tb-1802]
Whiteboard: [nvn-dl] [qa:verified-tb-1802] → [nvn-dl]
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.
Keywords: verified1.8.1.3
You need to log in before you can comment on or make changes to this bug.