Closed Bug 710940 Opened 8 years ago Closed 8 years ago

Firefox Crash [@ AffixMgr::parse_file(char const*, char const*) ]

Categories

(Core :: Spelling checker, defect, critical)

9 Branch
x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla12
Tracking Status
firefox11 --- fixed

People

(Reporter: marcia, Assigned: ehsan)

References

Details

(Keywords: crash, regression, topcrash, Whiteboard: [fixed-in-hunspell-1.3.3][qa-], startupcrash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
At #7 on the top crash list for the trunk. Adding the keyword.
Keywords: topcrash
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.
Assignee: nobody → ehsan
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #585563 - Flags: review?(bugs)
Around 52% of these happen in < 1 min.
Whiteboard: startupcrash
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 :/
Attachment #585563 - Flags: review?(bugs) → review+
[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.
Attachment #585563 - Attachment is obsolete: true
Attachment #587186 - Flags: approval-mozilla-beta?
Attachment #587186 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4778618b053
Target Milestone: --- → mozilla12
Nemeth/Caolan, can you land this upstream please?
(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.
The MOZILLA_CLIENT code is upstream as well. It will make future updates easier if it's included there as well.
https://hg.mozilla.org/mozilla-central/rev/b4778618b053
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
upstreamed this fix to hunspell cvs
Whiteboard: startupcrash → [fixed-in-hunspell-1.3.3], startupcrash
(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 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.
Attachment #587186 - Flags: approval-mozilla-beta?
Attachment #587186 - Flags: approval-mozilla-beta-
Attachment #587186 - Flags: approval-mozilla-aurora?
Attachment #587186 - Flags: approval-mozilla-aurora+
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!
(In reply to amai from comment #16)
Where the old bug is Bug 608639
Is there anything that QA can do to verify this fix (apart from checking crashstats)?
Whiteboard: [fixed-in-hunspell-1.3.3], startupcrash → [fixed-in-hunspell-1.3.3][qa?], startupcrash
Duplicate of this bug: 608639
(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.
Whiteboard: [fixed-in-hunspell-1.3.3][qa?], startupcrash → [fixed-in-hunspell-1.3.3][qa-], startupcrash
You need to log in before you can comment on or make changes to this bug.