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)
Core
Spelling checker
Tracking
()
RESOLVED
FIXED
mozilla5
People
(Reporter: hendrik, Assigned: RyanVM)
References
Details
Attachments
(1 file, 7 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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?
At the moment, I can only find 1.2.12 http://hunspell.cvs.sourceforge.net/viewvc/hunspell/hunspell/ChangeLog?view=log&pathrev=release-1-2-12 but no later release...
Please post the URLs also here https://bugs.launchpad.net/ubuntu/+source/hunspell/+bug/692908
Reporter | ||
Comment 4•14 years ago
|
||
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.
Comment 6•14 years ago
|
||
See also bug 355178.
Assignee | ||
Comment 7•14 years ago
|
||
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.
Assignee | ||
Comment 8•14 years ago
|
||
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.
Assignee | ||
Comment 10•14 years ago
|
||
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.
Assignee | ||
Comment 11•14 years ago
|
||
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
Assignee | ||
Comment 12•14 years ago
|
||
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()
Assignee | ||
Comment 13•14 years ago
|
||
For posterity's sake, this is the current patch that's crashing.
Assignee: nobody → ryanvm
Comment 14•14 years ago
|
||
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 ?
Assignee | ||
Comment 15•14 years ago
|
||
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.
Comment 16•14 years ago
|
||
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 ?
Assignee | ||
Comment 17•14 years ago
|
||
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
Assignee | ||
Comment 18•14 years ago
|
||
Patch updated to tip and based on the final release of Hunspell 1.2.13.
Attachment #499654 -
Attachment is obsolete: true
Assignee | ||
Comment 19•14 years ago
|
||
BTW, these are the symbol tarballs I was referring to:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/ryanvm@gmail.com-285670a7ad2f/try-lnx/firefox-4.0b9pre.en-US.linux-i686.crashreporter-symbols.zip
Comment 20•14 years ago
|
||
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.
Comment 21•14 years ago
|
||
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.
Assignee | ||
Comment 22•14 years ago
|
||
Gah, sorry I missed that in July. I'll try out the new patch tonight.
Assignee | ||
Comment 23•14 years ago
|
||
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
Assignee | ||
Comment 24•14 years ago
|
||
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)
Comment 25•14 years ago
|
||
Well, please push it to tryserver anyway.
Comment 26•14 years ago
|
||
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.
Assignee | ||
Comment 27•14 years ago
|
||
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.
Assignee | ||
Comment 28•14 years ago
|
||
(In reply to comment #25)
> Well, please push it to tryserver anyway.
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/ryanvm@gmail.com-afdf445360a5/
Assignee | ||
Comment 29•14 years ago
|
||
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 30•14 years ago
|
||
Comment on attachment 501846 [details] [diff] [review]
Hunspell 1.2.14 patch
rs=me
Attachment #501846 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 31•14 years ago
|
||
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?
Comment 32•14 years ago
|
||
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.
Comment 33•14 years ago
|
||
Németh, is that issue fixed in 1.2.14, or should we wait for 1.2.15, or just
take that patch separately?
Assignee | ||
Comment 34•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Summary: Upgrade to Hunspell 1.2.14 → Upgrade to Hunspell 1.2.15
Assignee | ||
Comment 35•14 years ago
|
||
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
Assignee | ||
Updated•14 years ago
|
Summary: Upgrade to Hunspell 1.2.15 → Upgrade to Hunspell 1.3.0
Assignee | ||
Comment 36•14 years ago
|
||
Attachment #507294 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 37•14 years ago
|
||
Comment 38•14 years ago
|
||
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...
Comment 39•14 years ago
|
||
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?
Comment 40•14 years ago
|
||
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.
Comment 41•14 years ago
|
||
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.
Comment 42•14 years ago
|
||
I hope there was a new test added to catch the case mentioned in comment 32 ;)
Comment 43•14 years ago
|
||
(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).
Comment 44•14 years ago
|
||
(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.
Assignee | ||
Comment 45•14 years ago
|
||
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.
Updated•14 years ago
|
Attachment #507294 -
Flags: review?(Olli.Pettay) → review+
Comment 46•14 years ago
|
||
Is 1.3.0 used by any other large project? OOo or LibreOffice or other browsers?
Comment 47•14 years ago
|
||
1.3.0 is currently being implemented in LibreOffice.
Laszlo Nemeth knows all about this.
Comment 48•14 years ago
|
||
About browsers: Mozilla will be the only one doing a good language job by implemting this. Opera fails, Chrome fails een more.
Assignee | ||
Comment 49•14 years ago
|
||
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?
Comment 50•14 years ago
|
||
(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. :-) )
Comment 51•14 years ago
|
||
CCing some drivers here. We need a decision here soon if we're going to take this. See comment 50 for more info.
Comment 52•14 years ago
|
||
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... :(
Assignee | ||
Comment 53•14 years ago
|
||
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.
Comment 55•14 years ago
|
||
(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.
Updated•14 years ago
|
Attachment #507294 -
Flags: approval2.0? → approval2.0-
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 56•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Alias: hunspell-1.3.0 → hunspell-1.3.1
Summary: Upgrade to Hunspell 1.3.0 → Upgrade to Hunspell 1.3.1
Updated•14 years ago
|
Attachment #509332 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 57•14 years ago
|
||
*yawn* Yet another rebasing...
Attachment #509332 -
Attachment is obsolete: true
Comment 58•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d981206620ff
http://hg.mozilla.org/mozilla-central/rev/d6eac984c86f
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.
Description
•