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

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Marcia Knous [:marcia]]

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]

At #7 on the top crash list for the trunk. Adding the keyword.

Comment 2

[(no longer active)]

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

[(no longer active)]

Attachment: Patch (v1)

Comment 4

[Sheila Mooney]

Around 52% of these happen in < 1 min.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

[(no longer active)]

Attachment: 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 7

[(no longer active)]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Nemeth/Caolan, can you land this upstream please?

Comment 9

[(no longer active)]

(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]]

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] (Away Apr 25 - May 5)]

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

Comment 12

[Caolan McNamara]

upstreamed this fix to hunspell cvs

Comment 13

[(no longer active)]

(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]]

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 15

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3301ea9d78b0

Comment 16

[amai]

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]

(In reply to amai from comment #16)
Where the old bug is Bug 608639

Comment 18

[u279076]

Is there anything that QA can do to verify this fix (apart from checking crashstats)?

Comment 20

[(no longer active)]

(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.