Closed Bug 1257902 Opened 4 years ago Closed 4 years ago

Upgrade to Hunspell 1.4.0

Categories

(Core :: Spelling checker, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed
firefox-esr38 --- wontfix
firefox-esr45 47+ fixed

People

(Reporter: dveditz, Assigned: RyanVM)

References

Details

(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main47+][adv-esr45.2+])

Attachments

(1 file, 1 obsolete file)

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
I can take this.
Assignee: nobody → 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
Attachment #8732571 - Flags: review?(ehsan)
Group: core-security → dom-core-security
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.
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.
Summary: Upgrade to Hunspell 1.3.4 → Upgrade to Hunspell 1.3.5
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.
Summary: Upgrade to Hunspell 1.3.5 → Upgrade to Hunspell 1.4.x
I would have expected https://github.com/hunspell/hunspell/commit/2824e4ed8fe9c476234edba87bf6042ca862ec8f to fix the problem related to the above commit
I haven't run a new Try push in the last 6 days, so let me try it out and report back!
VICTORY
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9561ae118402

TEST-PASS | extensions/spellcheck/hunspell/tests/unit/test_hunspell.js | took 34759ms
Tyson, do you want to give https://hg.mozilla.org/try/rev/9561ae118402e0aaded5375b0f7b6ed11fb7ce38 another round of fuzzing?
Flags: needinfo?(twsmith)
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.
Flags: needinfo?(twsmith)
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.
Flags: needinfo?(caolanm)
Oh, maybe tied to the affixmgr .rej files that got checked into the tree recently? Unresolved conflict?
what exactly doesn't work, give an example ? I presume this is with the mozilla en_US dictionary ?
Flags: needinfo?(caolanm)
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/ryanvm@gmail.com-3d408148d6ee831ae026ad0247213179a8b28832/
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]
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
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.
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.
Summary: Upgrade to Hunspell 1.4.x → Upgrade to Hunspell 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.
Attachment #8732571 - Attachment is obsolete: true
Attachment #8746910 - Flags: review?(ehsan)
Attachment #8746910 - Flags: review?(ehsan) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/f4317b12c1a3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1269941
Group: dom-core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main47+][adv-esr45.2+]
Group: core-security-release
Depends on: 1322666
You need to log in before you can comment on or make changes to this bug.