Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 11

Status

()

Core
Spelling checker
--
critical
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: marcia, Assigned: Ehsan)

Tracking

({crash, regression, topcrash})

9 Branch
mozilla12
x86
Windows XP
crash, regression, topcrash
Points:
---

Firefox Tracking Flags

(firefox11 fixed)

Details

(Whiteboard: [fixed-in-hunspell-1.3.3][qa-], startupcrash, crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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

6 years ago
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
Created attachment 585563 [details] [diff] [review]
Patch (v1)
Attachment #585563 - Flags: review?(bugs)

Comment 4

6 years ago
Around 52% of these happen in < 1 min.
Whiteboard: startupcrash

Comment 5

6 years ago
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+
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.
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
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 12

6 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/3301ea9d78b0
status-firefox11: --- → fixed

Comment 16

6 years ago
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

6 years ago
(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
Depends on: 1022262
You need to log in before you can comment on or make changes to this bug.