Closed Bug 620626 (hunspell-1.3.2) Opened 14 years ago Closed 14 years ago

Upgrade to Hunspell 1.3.2

Categories

(Core :: Spelling checker, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: hendrik, Assigned: RyanVM)

References

Details

Attachments

(1 file, 7 obsolete files)

Hunspell 1.2.13 has a lot of new features which we of the Dutch spelling list maker group heartily welcome. It would really improve spell checking for Dutch (and probably a lot of other languages) a lot. Is it going to be included in the next release?
To request this for Ubuntu I need URL to release and URL to changenotes. Can you provide these?
Yes, I am sorry, I’ve been too fast. 1.2.13 is up and coming. So the question is: will we convert, once it is there…
1.2.13 is currently still under development. But it is to be published within a month or 2.
See also bug 355178.
I'm certainly planning to post a patch once it goes final as I've done with the past few Hunspell releases. FWIW, I'm tracking bugs fixed in 1.2.13 reported in Bugzilla with the [fixed-in-hunspell-1.2.13] whiteboard comment if you want to search on it here. That will give you at least a partial set of fixed bugs.
Also, it's going to be up to the Firefox release drivers as to whether or not they'll accept a patch for this bug at this point in the Firefox 4.0 release process. But we'll worry about that once there's a patch up for landing.
For good langauge support, since we are working leading edge with Hunspell, it is essentail we knwo which Hunspell version is in which release for Mozilla products. So far, we have not been able to find this info.
Trunk (mozilla-central) is at 1.2.12. This will eventually branch for Firefox 4.0.x. 3.6.x branch is at 1.2.8, though it will be updated to 1.2.12 soon in bug 579649 for Firefox 3.6.14. 3.5.x branch is at 1.2.8 and is unlikely to be updated further. 3.0.x branch is at 1.1.12 and will not be updated further. You can use http://mxr.mozilla.org/ to see the source for the various trees/branches of the Mozilla codebase. Hunspell lives in extensions/spellcheck/hunspell/src. README.hunspell states what version lives there along with any additional patches that have been taken since the release.
I should probably note: Gecko 2.0 = Firefox 4.0.x (hasn't branched yet, though) Gecko 1.9.2 = Firefox 3.6.x Gecko 1.9.1 = Firefox 3.5.x Gecko 1.9.0 = Firefox 3.0.x
I just downloaded a current tarball of Hunspell CVS and made a patch for Mozilla. Unfortunately, I'm getting crashes in the following new code in affixmgr.cpp: for (int i = 0; i <= 255; i++) { if ( (csconv[i].cupper != csconv[i].clower) && ---> (! strchr(expw, (char) i))) { *(expw + strlen(expw) + 1) = '\0'; *(expw + strlen(expw)) = (char) i; } } Stack: xul.dll!AffixMgr::parse_file(const char * affpath, const char * key) Line 756 + 0x15 bytes xul.dll!AffixMgr::AffixMgr(const char * affpath, HashMgr * * ptr, int * md, const char * key) Line 160 + 0x10 bytes xul.dll!Hunspell::Hunspell(const char * affpath, const char * dpath, const char * key) Line 84 + 0x34 bytes xul.dll!mozHunspell::SetDictionary(const wchar_t * aDictionary) Line 168 + 0x3b bytes xul.dll!mozSpellChecker::SetCurrentDictionary(const nsAString_internal & aDictionary) Line 384 + 0x34 bytes xul.dll!nsEditorSpellCheck::SetCurrentDictionary(const wchar_t * aDictionary) Line 447 + 0x29 bytes xul.dll!nsEditorSpellCheck::InitSpellChecker(nsIEditor * aEditor, int aEnableSelectionChecking) Line 222 + 0x17 bytes xul.dll!mozInlineSpellChecker::SetEnableRealTimeSpell(int aEnabled) Line 733 + 0x24 bytes xul.dll!nsEditor::SyncRealTimeSpell() Line 1322 xul.dll!nsEditor::SetFlags(unsigned int aFlags) Line 474 + 0x11 bytes xul.dll!nsEditor::PostCreate() Line 284 + 0x17 bytes xul.dll!nsTextEditorState::PrepareEditor(const nsAString_internal * aValue) Line 1354 xul.dll!nsHTMLTextAreaElement::CreateEditor() Line 563 xul.dll!nsTextControlFrame::EnsureEditorInitialized() Line 412 + 0x19 bytes xul.dll!nsTextControlFrame::EditorInitializer::Run() Line 287 xul.dll!nsContentUtils::RemoveScriptBlocker() Line 4725 xul.dll!nsAutoScriptBlocker::~nsAutoScriptBlocker() Line 1897 xul.dll!PresShell::FlushPendingNotifications(mozFlushType aType) Line 4862 xul.dll!nsRefreshDriver::Notify(nsITimer * __formal) Line 300 xul.dll!nsTimerImpl::Fire() Line 429 xul.dll!nsTimerEvent::Run() Line 519 xul.dll!nsThread::ProcessNextEvent(int mayWait, int * result) Line 626 + 0x19 bytes xul.dll!NS_ProcessNextEvent_P(nsIThread * thread, int mayWait) Line 250 + 0x16 bytes xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 110 + 0xe bytes xul.dll!MessageLoop::RunInternal() Line 220 xul.dll!MessageLoop::RunHandler() Line 203 xul.dll!MessageLoop::Run() Line 177 xul.dll!nsBaseAppShell::Run() Line 198 xul.dll!nsAppShell::Run() Line 258 + 0x9 bytes xul.dll!nsAppStartup::Run() Line 191 + 0x1c bytes xul.dll!XRE_main(int argc, char * * argv, const nsXREAppData * aAppData) Line 3694 + 0x25 bytes firefox.exe!NS_internal_main(int argc, char * * argv) Line 158 + 0x12 bytes firefox.exe!wmain(int argc, wchar_t * * argv) Line 128 + 0xd bytes firefox.exe!__tmainCRTStartup() Line 552 + 0x17 bytes kernel32.dll!76f03677() [Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll] ntdll.dll!779e9d42() ntdll.dll!779e9d15()
Attached patch Current Hunspell CVS patch (obsolete) (deleted) — Splinter Review
For posterity's sake, this is the current patch that's crashing.
Assignee: nobody → ryanvm
Is there either a) a build available b) some more debugging info to see if csconv is NULL or not, and what the value of "wordchars" and "enc" and "expw" are at that point c) or what to checkout and build to get the above crash ?
Tryserver builds with 1.2.13 applied are available below. It's still building, so not everything is showing yet. Debug builds are included in tryserver runs. http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/ryanvm@gmail.com-285670a7ad2f/ The Linux tests are already showing the crashing, though.
Doesn't seems to have debugging symbols usable with gdb, which suggests I'd need to use fetch-symbols.py firefox http://build.mozilla.org/tryserver-symbols to get them. Which seems I need level 1 commit access to download those, which seems to need a twelve step account request procedure :-) I assume that the moz-specific encoding thing is failing and that its actually the line above i.e. csconv which is crashing with a NULL csconv. Is there an anon source tree that I can pull and/or something available with debugging symbols ?
I guess the symbol tarballs in the build directories don't work with gdb? You can always pull the current source from the Mozilla hg repo and manually apply the patch from this bug, but getting a working build environment can be a bit of a pain :-). https://developer.mozilla.org/en/Build_Documentation
Attached patch Release Hunspell 1.2.13 patch (obsolete) (deleted) — Splinter Review
Patch updated to tip and based on the final release of Hunspell 1.2.13.
Attachment #499654 - Attachment is obsolete: true
oky doky: the mozilla custom get_current_cs returns NULL for the caseconversion service. Looking at the mozilla mercurial log. I see this is because of bug 575043, but that change didn't get submitted to hunspell itself, so upstream hunspell was unchanged, and on upgrade to it that reintroduced the old caseconversion stuff again, which fails.
Attached patch So, here's a replacement upgrade patch (obsolete) (deleted) — Splinter Review
So here's the original upgrade patch with that additional changed merged into it. I've committed the removal of nsICaseConversion into upstream hunspell now as well.
Gah, sorry I missed that in July. I'll try out the new patch tonight.
Hunspell 1.2.14 is out and contains the upstreamed code. Updating bug title accordingly.
Summary: Upgrade to Hunspell 1.2.13 → Upgrade to Hunspell 1.2.14
Attached patch Hunspell 1.2.14 patch (obsolete) (deleted) — Splinter Review
Hunspell 1.2.14 is working nicely in my local build. I'll give it a push to the tryserver at some point in case anyone else wants to try it out.
Attachment #501038 - Attachment is obsolete: true
Attachment #501297 - Attachment is obsolete: true
Attachment #501846 - Flags: review?(Olli.Pettay)
Well, please push it to tryserver anyway.
Could anyone please specify which versions of Firefox and Thunderbird this release will be compiled into? That way we can tune the new Dutch dictionary add-on to check for that version, because it will not work on lower versions.
Ruud, as I said in comment #8, it's unclear at this point which Firefox/Thunderbird releases will be the first to pick this up due to where things stand in the Firefox 4.0 release process. Once this lands, I'll be able to tell you.
All green on the tryserver. gcc does show the following build warning: affixmgr.cpp: In member function 'hentry* AffixMgr::compound_check(const char*, int, short int, short int, short int, short int, hentry**, char, char)': affixmgr.cpp:1538: warning: 'ch' may be used uninitialized in this function
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Comment on attachment 501846 [details] [diff] [review] Hunspell 1.2.14 patch rs=me
Attachment #501846 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 501846 [details] [diff] [review] Hunspell 1.2.14 patch Requesting approval2.0. This fixes a few memory leaks found in 1.2.12 (see the dependency list) and improves our spellchecking support for at least Dutch users, potentially others as well. Per comment #29, it builds and passes tryserver OK.
Attachment #501846 - Flags: approval2.0?
There is an important patch in the Hunspell CVS, also attached to the following issue: https://sourceforge.net/tracker/?func=detail&aid=3158994&group_id=143754&atid=756395 Without this patch, UTF-8 encoded dictionaries with PHONE feature halt Hunspell and Firefox.
Németh, is that issue fixed in 1.2.14, or should we wait for 1.2.15, or just take that patch separately?
Comment on attachment 501846 [details] [diff] [review] Hunspell 1.2.14 patch The crash is in the 1.2.14 release code. I was able to reproduce it in my test builds and can also confirm that it is fixed in current Hunspell CVS. We are also investigating bug 626195 to see if that bug is also fixed by the current code or if it can be fixed before the next release. Cancelling the approval2.0 request due to the known crash bug. I will post a new patch for 1.2.15 upon its release.
Attachment #501846 - Attachment is obsolete: true
Attachment #501846 - Flags: approval2.0?
Summary: Upgrade to Hunspell 1.2.14 → Upgrade to Hunspell 1.2.15
Blocks: 626195
Hunspell 1.2.15/1.3.0 has been released. Per Nemeth, the two releases are identical. Tryserver builds will be in the directory below as they finish: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/ryanvm@gmail.com-812d3b9c129d
Summary: Upgrade to Hunspell 1.2.15 → Upgrade to Hunspell 1.3.0
Attached patch Hunspell 1.2.15/1.3.0 patch (obsolete) (deleted) — Splinter Review
Attachment #507294 - Flags: review?(Olli.Pettay)
Attached patch 1.2.14 to 1.3.0 interdiff (obsolete) (deleted) — Splinter Review
Nemeth: is there anything in the 1.2.12 -> 1.3.0 which could change *any* behavior? If there is, I'm afraid that we can't take the 1.3.0 upgrade for Firefox 4, because of the possible risk...
Also, what is the risk for things like comment 32? I think I have asked this before: does Hunspell have lots of tests, hopefully run automatically whenever something in the code changes?
It would increase confidence if hunspell could provide regression test reports, e.g. a comparison between unit test results of 1.2.12 and 1.3.0.
There's a 110 (or there abouts) test test-suite run as part of "make check" in hunspell and optionally when export VALGRIND=memcheck is set they're also run under valgrind with any valgrind warnings triggering a fail. Checking 1.2.15 today they all continue to pass.
I hope there was a new test added to catch the case mentioned in comment 32 ;)
(In reply to comment #38) > Nemeth: is there anything in the 1.2.12 -> 1.3.0 which could change *any* > behavior? If there is, I'm afraid that we can't take the 1.3.0 upgrade for > Firefox 4, because of the possible risk... There is no intended back-compatibility, only bug fixes and some new features, mostly in compound word handling. I have compared Hunspell 1.2.12 and 1.3.0 on en_US and de_DE (German dictionary uses some compound features fixed by version 1.3.0) word lists, too. Both Hunspell versions accept the same words. The suggested words are a little different, because Hunspell 1.3.0 has an improved suggestion mechanism. The new code contains two important bug fixes for the Hungarian and the new French dictionaries (http://sourceforge.net/tracker/?func=detail&atid=756395&aid=3136796&group_id=143754, https://bugzilla.mozilla.org/show_bug.cgi?id=626195).
(In reply to comment #42) > I hope there was a new test added to catch the case mentioned in comment 32 ;) The fixes were verified on the new French dictionary. The second problem (http://sourceforge.net/tracker/?func=detail&atid=756395&aid=3136796&)group_id=143754) caught and verified by Valgrind.
Ehsan/Olli - What do we want to do with this? I've been running builds with this patch for the last couple days without issue. It's green on try. My thoughts are to land this for beta 11, keep a very close eye on crash stats (I'll volunteer to help), and yank it at the first sign of trouble.
Attachment #507294 - Flags: review?(Olli.Pettay) → review+
Is 1.3.0 used by any other large project? OOo or LibreOffice or other browsers?
1.3.0 is currently being implemented in LibreOffice. Laszlo Nemeth knows all about this.
About browsers: Mozilla will be the only one doing a good language job by implemting this. Opera fails, Chrome fails een more.
Comment on attachment 507294 [details] [diff] [review] Hunspell 1.2.15/1.3.0 patch Requesting approval2.0. Drivers, see comment #45 and this bug's dependencies. This release fixes numerous known memory leaks and potential crashes, and it is also believed to fix a topcrash bug (though not verified at this time). Due to the difficulty in consistently reproducing the topcrash, a widespread beta audience is our best bet for confirming the fix. We will keep a close eye for any regressions and back out at the first sign of trouble.
Attachment #507294 - Flags: approval2.0?
(In reply to comment #49) > Comment on attachment 507294 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=507294 > Hunspell 1.2.15/1.3.0 patch > > Requesting approval2.0. Drivers, see comment #45 and this bug's dependencies. > This release fixes numerous known memory leaks and potential crashes, and it is > also believed to fix a topcrash bug (though not verified at this time). Due to > the difficulty in consistently reproducing the topcrash, a widespread beta > audience is our best bet for confirming the fix. We will keep a close eye for > any regressions and back out at the first sign of trouble. Do we have any idea what in this upgrade exactly fixed those crashes? Was it a major rewrite, or small scoped fixed which can be cherry-picked? If the latter, I'd advocate to not take this for Firefox 4, and take the individual fixes instead. If the former, I'd advocate to land this AS SOON AS POSSIBLE, with the option of backing it out for final if it causes stability issues. (And I'm holding Ryan up for his promise of keeping an eye on crash data if we decide to take this. :-) )
CCing some drivers here. We need a decision here soon if we're going to take this. See comment 50 for more info.
I should also note that all of the dependencies except for bug 626195 have small scoped patches which look safe. But we really need to fix bug 626195 too... :(
Nemeth posted specific changes in bug 626195 comment #37. Initial feedback in that bug is that those changes do indeed fix the crash. Ehsan, my thought was that specific scoped patches were more appropriate for 1.9.2 where we're already on a security/stability updates only policy anyway. I will happily post that patch to bug 626195. However, 1.3.0 also provides an opportunity to make some significant spellchecking improvements for non-English users. For that reason, I'd still argue in favor of landing 1.3.0 for the next beta release. I understand where things are at in the 4.0 release cycle, which is why it certainly makes sense to keep the patch on a very short leash. However, spellcheck is a reasonably well-contained component, so regressions caused by the update should be made readily apparent with the exposure of a beta release.
While I understand the desire to improve spell checking (being swedish, it sounds like there might be some benefits in there for me even), we're way way way past feature freeze. At this point I would treat FF4 as FF3.6. I.e. if we'd only take the spot crash-fix on 3.6, then that's what I think we should take for FF4. However, don't fret. Next version of firefox is only 3 months out, so we'll get the features in users hands before long.
(In reply to comment #53) > Nemeth posted specific changes in bug 626195 comment #37. Initial feedback in > that bug is that those changes do indeed fix the crash. OK, then, we should take that patch on 2.0. Can you please knock up a patch and submit it for review and approval in that bug? > Ehsan, my thought was that specific scoped patches were more appropriate for > 1.9.2 where we're already on a security/stability updates only policy anyway. I > will happily post that patch to bug 626195. > > However, 1.3.0 also provides an opportunity to make some significant > spellchecking improvements for non-English users. For that reason, I'd still > argue in favor of landing 1.3.0 for the next beta release. I understand where > things are at in the 4.0 release cycle, which is why it certainly makes sense > to keep the patch on a very short leash. However, spellcheck is a reasonably > well-contained component, so regressions caused by the update should be made > readily apparent with the exposure of a beta release. Still, this change is too big and scary, and given the fact that it doesn't really solve stability issues which can't be fixed using small scoped patches, I recommend that we shouldn't take this for Firefox 4.
Attachment #507294 - Flags: approval2.0? → approval2.0-
Blocks: post2.0
Alias: hunspell-1.3.0
No longer blocks: post2.0
Depends on: post2.0
Attached patch Hunspell 1.3.1 patch (obsolete) (deleted) — Splinter Review
Version 1.3.1 is out. In addition to the previous fixes, version 1.3.1 also improves English spellchecking suggestions. Also rebased on top of the dependent bugs now that they've all landed independently. Passes tryserver without issue.
Attachment #507294 - Attachment is obsolete: true
Attachment #507295 - Attachment is obsolete: true
Attachment #509332 - Flags: review?(ehsan)
Alias: hunspell-1.3.0 → hunspell-1.3.1
Summary: Upgrade to Hunspell 1.3.0 → Upgrade to Hunspell 1.3.1
Attachment #509332 - Flags: review?(ehsan) → review+
Blocks: 631607
Attached patch Hunspell 1.3.1 patch v2 (deleted) — Splinter Review
*yawn* Yet another rebasing...
Attachment #509332 - Attachment is obsolete: true
Alias: hunspell-1.3.1 → hunspell-1.3.2
Status: NEW → RESOLVED
Closed: 14 years ago
No longer depends on: post2.0
Resolution: --- → FIXED
Summary: Upgrade to Hunspell 1.3.1 → Upgrade to Hunspell 1.3.2
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: