1257902 - Upgrade to Hunspell 1.4.0

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Daniel Veditz [:dveditz]]

Our current version of Hunspell is 1.3.3 which came out in June 2014. The most recent version 1.3.4 came out in Jan 2016, but includes lots of Coverity-inspired code fixes in late 2014. Some of those are potential security problems.

Note the project now appears to live at http://hunspell.github.io/, code at https://github.com/hunspell/hunspell

Comment 1

[Ryan VanderMeulen [:RyanVM]]

I can take this.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: update Hunspell to version 1.3.4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f3f89cc1f90

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8732571 [details] [diff] [review]
update Hunspell to version 1.3.4

Test failures, joy.
https://treeherder.mozilla.org/logviewer.html#?job_id=18320250&repo=try

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Test appears to be failing in ignore.good, but I'm afraid I'm not in a position to helpfully debug it :\
PROCESS | 17003 | Setting dictionary to ignore
PROCESS | 17003 | getting file by line for file /builds/slave/test/build/tests/xpcshell/tests/extensions/spellcheck/hunspell/tests/unit/data/ignore.good
PROCESS | 17003 | using charset iso-8859-1
PROCESS | 17003 | Expect word example is spelled correctly
TEST-UNEXPECTED-FAIL | extensions/spellcheck/hunspell/tests/unit/test_hunspell.js | do_run_test - [do_run_test : 178] false == true

https://hg.mozilla.org/mozilla-central/raw-file/a235bfcc8c411169b82420c503775c1a3e7edad5/extensions/spellcheck/hunspell/tests/unit/data/ignore.good

Should would be nice if the test told us what word it didn't like.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

I'm going to skip version 1.3.4 in favor of 1.3.5, which should contain fixes for the various fuzz bugs Tyson has been finding and filing.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

FYI, I've managed to bisect down to the specific upstream commit that's causing the test failures on Try. No updates on the root cause, though.
https://github.com/hunspell/hunspell/commit/bdd99b7ec02c1f751c0d4fa232f6ece5357760fb

I am wondering if it has something to do with our recentish changes to a more-recent upstream SCOWL dictionary update (bug 1137544). Caolan says he's been unable to reproduce the failures when running Hunspell's |make check| locally.

Comment 7

[Caolan McNamara]

I would have expected https://github.com/hunspell/hunspell/commit/2824e4ed8fe9c476234edba87bf6042ca862ec8f to fix the problem related to the above commit

Comment 8

[Ryan VanderMeulen [:RyanVM]]

I haven't run a new Try push in the last 6 days, so let me try it out and report back!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

VICTORY
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9561ae118402

TEST-PASS | extensions/spellcheck/hunspell/tests/unit/test_hunspell.js | took 34759ms

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Tyson, do you want to give https://hg.mozilla.org/try/rev/9561ae118402e0aaded5375b0f7b6ed11fb7ce38 another round of fuzzing?

Comment 11

[Tyson Smith [:tsmith]]

I've been fuzzing hunspell revision c961e8b7372a1c2a999d216871c965fb8cf843df (2 revisions past 1.4) since it was released and haven't found any issues. I have been fuzzing the sample utf8 dictionary and the sample Korean dictionary.

Caolán has been fixing potential issues faster than I can find them! From a fuzzing perspective this looks good.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Caolan, I did a build off rev cefc6f2, and spellchecking words with suffixes appears to be broken. Basically, anything plural or otherwise is getting flagged.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Oh, maybe tied to the affixmgr .rej files that got checked into the tree recently? Unresolved conflict?

Comment 14

[Caolan McNamara]

what exactly doesn't work, give an example ? I presume this is with the mozilla en_US dictionary ?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Correct, Fx browser build. I pushed it to Try if you want to play with the resulting builds yourself.
http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-3d408148d6ee831ae026ad0247213179a8b28832/

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Also, Android builds are busted:
/builds/slave/try-and-api-15-000000000000000/build/src/extensions/spellcheck/hunspell/src/hunzip.hxx:85:45: error: passing 'const ifstream {aka const std::basic_ifstream<char, std::char_traits<char> >}' as 'this' argument of 'bool std::basic_ifstream<_CharT, _Traits>::is_open() [with _CharT = char; _Traits = std::char_traits<char>]' discards qualifiers [-fpermissive]

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Unsurprisingly, this is failing tests too:
https://treeherder.mozilla.org/logviewer.html#?job_id=19889511&repo=try

PROCESS | 17150 | Expect word try is spelled correctly
TEST-PASS | extensions/spellcheck/hunspell/tests/unit/test_hunspell.js | do_run_test - [do_run_test : 178] true == true
PROCESS | 17150 | Expect word tried is spelled correctly
TEST-UNEXPECTED-FAIL | extensions/spellcheck/hunspell/tests/unit/test_hunspell.js | do_run_test - [do_run_test : 178] false == true

Comment 18

[Caolan McNamara]

I can see the failure in your build, but if I try and build my own by
hg clone http://hg.mozilla.org/integration/fx-team
and copying the .cxxs and hxxs of cefc6f2 over the ones in that tree it results in an apparently working spellchecking experience so I can't debug it. A bit odd.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

The final 1.4.0 release passes tests on Try and contains all the relevant fuzzer fixes, so let's just go with it. There's a lot of upstream refactoring going on, so I expect the next release to be a ways out still.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Attachment: update Hunspell to version 1.4.0

Ehsan, I know you're busy and not working on Editor anymore, but I really have no clue who else to ask to review this. Feel free to redirect it to someone more appropriate if there is one.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ffff1927e77a2c3df8cdc7ffa2801e8033aea4b

Just one note, this drops the patch from bug 983817, which never got upstreamed. I asked Caolan about it and he said that Hunspell is using std::sort now, so it's very unlikely to be an issue anymore. But we might want to keep an eye on crash-stats for any similar-looking signatures and re-add the patch if needed.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4317b12c1a3

I'm going to let this bake on Nightly for a few days and have my Softvision team give this a QA pass, then I'll request uplift to the various release branches.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f4317b12c1a3

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Fixed on the release branches via bug 1269941.