620626 - (hunspell-1.3.2) Upgrade to Hunspell 1.3.2

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Hendrik Maryns]

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?

Comment 1

[Pander]

To request this for Ubuntu I need URL to release and URL to changenotes. Can you provide these?

Comment 2

[Pander]

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...

Comment 3

[Pander]

Please post the URLs also here https://bugs.launchpad.net/ubuntu/+source/hunspell/+bug/692908

Comment 4

[Hendrik Maryns]

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…

Comment 5

[R Baars]

1.2.13 is currently still under development. 
But it is to be published within a month or 2.

Comment 6

[Reinout van Schouwen]

See also bug 355178.

Comment 7

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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.

Comment 9

[R Baars]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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()

Comment 13

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Attachment: Current Hunspell CVS patch

For posterity's sake, this is the current patch that's crashing.

Comment 14

[Caolan McNamara]

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 ?

Comment 15

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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/[e-mail]-285670a7ad2f/

The Linux tests are already showing the crashing, though.

Comment 16

[Caolan McNamara]

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 ?

Comment 17

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Attachment: Release Hunspell 1.2.13 patch

Patch updated to tip and based on the final release of Hunspell 1.2.13.

Comment 19

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

BTW, these are the symbol tarballs I was referring to:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-285670a7ad2f/try-lnx/firefox-4.0b9pre.en-US.linux-i686.crashreporter-symbols.zip

Comment 20

[Caolan McNamara]

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

[Caolan McNamara]

Attachment: So, here's a replacement upgrade patch

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.

Comment 22

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Gah, sorry I missed that in July. I'll try out the new patch tonight.

Comment 23

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Hunspell 1.2.14 is out and contains the upstreamed code. Updating bug title accordingly.

Comment 24

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Attachment: Hunspell 1.2.14 patch

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.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Well, please push it to tryserver anyway.

Comment 26

[R Baars]

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.

Comment 27

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

(In reply to comment #25)
> Well, please push it to tryserver anyway.

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-afdf445360a5/

Comment 29

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 501846 [details] [diff] [review]
Hunspell 1.2.14 patch

rs=me

Comment 31

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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.

Comment 32

[Németh László]

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

[Olli Pettay [:smaug][[e-mail]]]

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 34

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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.

Comment 35

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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/[e-mail]-812d3b9c129d

Comment 36

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Attachment: Hunspell 1.2.15/1.3.0 patch

Comment 37

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Attachment: 1.2.14 to 1.3.0 interdiff

Comment 38

[(no longer active)]

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

[Olli Pettay [:smaug][[e-mail]]]

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

[Pander]

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

[Caolan McNamara]

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

[Olli Pettay [:smaug][[e-mail]]]

I hope there was a new test added to catch the case mentioned in comment 32 ;)

Comment 43

[Németh László]

(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

[Németh László]

(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.

Comment 45

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Is 1.3.0 used by any other large project? OOo or LibreOffice or other browsers?

Comment 47

[R Baars]

1.3.0 is currently being implemented in LibreOffice.
Laszlo Nemeth knows all about this.

Comment 48

[R Baars]

About browsers: Mozilla will be the only one doing a good language job by implemting this. Opera fails, Chrome fails een more.

Comment 49

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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.

Comment 50

[(no longer active)]

(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

[(no longer active)]

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

[(no longer active)]

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... :(

Comment 53

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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.

Comment 54

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[(no longer active)]

(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.

Comment 56

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Attachment: Hunspell 1.3.1 patch

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.

Comment 57

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Attachment: Hunspell 1.3.1 patch v2

*yawn* Yet another rebasing...

Comment 58

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/d981206620ff
http://hg.mozilla.org/mozilla-central/rev/d6eac984c86f