Last Comment Bug 710940 - Firefox Crash [@ AffixMgr::parse_file(char const*, char const*) ]
: Firefox Crash [@ AffixMgr::parse_file(char const*, char const*) ]
Status: RESOLVED FIXED
[fixed-in-hunspell-1.3.3][qa-], start...
: crash, regression, topcrash
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: 9 Branch
: x86 Windows XP
: -- critical (vote)
: mozilla12
Assigned To: :Ehsan Akhgari
:
Mentors:
: 608639 (view as bug list)
Depends on: hunspell-1.3.3
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-14 17:11 PST by Marcia Knous [:marcia - use ni]
Modified: 2014-06-16 06:42 PDT (History)
11 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch (v1) (3.07 KB, patch)
2012-01-03 14:37 PST, :Ehsan Akhgari
bugs: review+
Details | Diff | Splinter Review
Review comments addressed (3.08 KB, patch)
2012-01-09 16:04 PST, :Ehsan Akhgari
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Marcia Knous [:marcia - use ni] 2011-12-14 17:11:25 PST
Seen while looking at trunk crash reports. Crashes started showing up using the 2011121400 build on trunk but the crash is seen on other versions as well. https://crash-stats.mozilla.com/report/list?signature=AffixMgr::parse_file%28char%20const*,%20char%20const*%29. Some of the reports look like they are probably the same person, but not all.

Not sure what caused the spike on trunk, but here is what was checked in: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3c321d2c9884&tochange=fd6ab19f312c

https://crash-stats.mozilla.com/report/index/3114185e-8dee-4790-ad92-f01352111214

Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	AffixMgr::parse_file 	extensions/spellcheck/hunspell/src/affixmgr.cpp:808
1 	xul.dll 	AffixMgr::AffixMgr 	extensions/spellcheck/hunspell/src/affixmgr.cpp:167
2 	xul.dll 	Hunspell::Hunspell 	extensions/spellcheck/hunspell/src/hunspell.cpp:84
3 	xul.dll 	mozHunspell::SetDictionary 	extensions/spellcheck/hunspell/src/mozHunspell.cpp:220
4 	xul.dll 	PromiseFlatString 	obj-firefox/dist/include/nsTPromiseFlatString.h:134
5 	xul.dll 	mozSpellChecker::SetCurrentDictionary 	extensions/spellcheck/src/mozSpellChecker.cpp:385
Comment 1 Sheila Mooney 2011-12-21 22:05:12 PST
At #7 on the top crash list for the trunk. Adding the keyword.
Comment 2 :Ehsan Akhgari 2012-01-03 14:34:29 PST
This is a real bug which has existed since forever.  Our implementation of get_current_cs can return null.  hunspell's implementation can't, and relies on this assumption all over the place.  We should stop returning null, and just return dummy data in case of failures, just as hunspell's implementation does.

I have a patch for this.
Comment 3 :Ehsan Akhgari 2012-01-03 14:37:19 PST
Created attachment 585563 [details] [diff] [review]
Patch (v1)
Comment 4 Sheila Mooney 2012-01-06 13:32:33 PST
Around 52% of these happen in < 1 min.
Comment 5 Olli Pettay [:smaug] 2012-01-07 11:54:36 PST
Comment on attachment 585563 [details] [diff] [review]
Patch (v1)


> // XXX This function was rewritten for mozilla. Instead of storing the
> // conversion tables static in this file, create them when needed
> // with help the mozilla backend.
> struct cs_info * get_current_cs(const char * es) {
>-  struct cs_info *ccs;
>+  struct cs_info *ccs = new cs_info[256];
>+  // Initialze the array with dummy data so that we wouldn't need
>+  // to return null in case of failures.
>+  for (int i = 0; i < 256; ++i) {
Could you use 0xff here, so that the loop looks like the one below.
This error handling is very strange, but it is what the code below does too :/
Comment 6 :Ehsan Akhgari 2012-01-09 16:04:48 PST
Created attachment 587186 [details] [diff] [review]
Review comments addressed

[Approval Request Comment]
Regression caused by (bug #): This has been broken since forever.
User impact if declined: A crash not being fixed.
Testing completed (on m-c, etc.): I tested this patch locally, and I believe the code changes are sane.
Risk to taking this patch (and alternatives if risky):  This is relatively low-risk.  The alternative is patching all of the callers of get_current_cs, which is way more risky.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-01-09 16:09:14 PST
Nemeth/Caolan, can you land this upstream please?
Comment 9 :Ehsan Akhgari 2012-01-09 16:16:38 PST
(In reply to Ryan VanderMeulen from comment #8)
> Nemeth/Caolan, can you land this upstream please?

This is a bug in Mozilla specific customizations to hunspell, it doesn't need to be upstreamed.
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-01-09 16:22:49 PST
The MOZILLA_CLIENT code is upstream as well. It will make future updates easier if it's included there as well.
Comment 11 Marco Bonardo [::mak] 2012-01-10 01:58:12 PST
https://hg.mozilla.org/mozilla-central/rev/b4778618b053
Comment 12 Caolan McNamara 2012-01-10 02:01:45 PST
upstreamed this fix to hunspell cvs
Comment 13 :Ehsan Akhgari 2012-01-10 10:19:32 PST
(In reply to Ryan VanderMeulen from comment #10)
> The MOZILLA_CLIENT code is upstream as well. It will make future updates
> easier if it's included there as well.

Ah, I didn't know that, thanks!
Comment 14 Alex Keybl [:akeybl] 2012-01-10 14:56:51 PST
Comment on attachment 587186 [details] [diff] [review]
Review comments addressed

[Triage Comment]
Approving for Aurora, but since this has been broken forever, we'll let it ride 11 and up.
Comment 16 amai 2012-01-28 09:23:24 PST
Since that is an old bug blocking functionality and a no-risk fix (w/o it is just segfaulting) I vote for bring the fix to a version <11!
Comment 17 amai 2012-01-28 09:23:45 PST
(In reply to amai from comment #16)
Where the old bug is Bug 608639
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-01 13:03:59 PST
Is there anything that QA can do to verify this fix (apart from checking crashstats)?
Comment 19 Wayne Mery (:wsmwk, NI for questions) 2012-03-18 05:21:18 PDT
*** Bug 608639 has been marked as a duplicate of this bug. ***
Comment 20 :Ehsan Akhgari 2012-03-19 09:37:09 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #18)
> Is there anything that QA can do to verify this fix (apart from checking
> crashstats)?

No, I think that is all which can be done here.

(In reply to amai from comment #16)
> Since that is an old bug blocking functionality and a no-risk fix (w/o it is
> just segfaulting) I vote for bring the fix to a version <11!

Firefox <11 is no longer being maintained, and Firefox 11 has fixed this bug.  You should upgrade to the latest version of Firefox.

Note You need to log in before you can comment on or make changes to this bug.