631607 - Crash while spell checking with French modern dictionary 4.0.3 [@ mozalloc_abort(char const* const) ][@ mozalloc_abort(char const* const) | mozalloc_handle_oom() ] or [@ linux-gate.so@0x424 ]

Status: VERIFIED FIXED

(Core :: Spelling checker, defect)

Description

[Ben]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10) Gecko/20100101 Firefox/4.0b10
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11) Gecko/20100101 Firefox/4.0b11 ID:20110203141415

Using a french FF 4.0 b11


Reproducible: Always

Steps to Reproduce:
1. Open http://www.quackit.com/html/codes/html_text_box_code.cfm
2. Clear the first text box, the one that says "Enter your comments here..." and
type in "L'Islam ", and right click on the word.
3. Continue working, open new tabs, etc.
Actual Results:  
Firefox crashes very soon after the right click.
Once it did not crash immediatly and crashed at Quit.



Multiple crash logs

bp-68616d07-69cf-4c06-9ed2-bd8312110204
bp-546ad495-728e-41fc-ae31-305a12110204
bp-8bb1ddeb-baee-4c1b-ae64-c2a7d2110204
bp-ce04724b-89d9-4218-87d1-498362110204
bp-df9b12bd-02f9-45f6-94bb-58dc82110204 [crash at exit].
bp-824d322f-b8db-4b85-a7f8-750842110204 [crash later]

the trace is everytime different, but most of them have a OOM condition or a malloc issue.

I tried using other words, but only "L'Islam" causes problems so far !
Frame 	Module 	Signature [Expand] 	Source
0 	mozalloc.dll 	mozalloc_abort 	memory/mozalloc/mozalloc_abort.cpp:77
1 	mozalloc.dll 	mozalloc_handle_oom 	memory/mozalloc/mozalloc_oom.cpp:54
2 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::CreateOrRecycleThebesLayer 	layout/base/FrameLayerBuilder.cpp:755
3 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1307
4 	xul.dll 	mozilla::FrameLayerBuilder::BuildContainerLayerFor 	layout/base/FrameLayerBuilder.cpp:1568
5 	xul.dll 	nsDisplayOpacity::BuildLayer 	layout/base/nsDisplayList.cpp:1600
6 	xul.dll 	mozilla::BuildTempManagerForInactiveLayer 	layout/base/FrameLayerBuilder.cpp:1169
7 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1285
8 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1212
9 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1212
10 	xul.dll 	mozilla::FrameLayerBuilder::BuildContainerLayerFor 	layout/base/FrameLayerBuilder.cpp:1568
11 	xul.dll 	nsDisplayOpacity::BuildLayer 	layout/base/nsDisplayList.cpp:1600
12 	xul.dll 	mozilla::BuildTempManagerForInactiveLayer 	layout/base/FrameLayerBuilder.cpp:1169
13 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1285
14 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1212
15 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1212
16 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1212
17 	xul.dll 	mozilla::FrameLayerBuilder::BuildContainerLayerFor 	layout/base/FrameLayerBuilder.cpp:1568
18 	xul.dll 	nsDisplayOwnLayer::BuildLayer 	layout/base/nsDisplayList.cpp:1667
19 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1246
20 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1212
21 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1212
22 	xul.dll 	mozilla::`anonymous namespace'::ContainerState::ProcessDisplayItems 	layout/base/FrameLayerBuilder.cpp:1212
23 	xul.dll 	mozilla::FrameLayerBuilder::BuildContainerLayerFor 	layout/base/FrameLayerBuilder.cpp:1568
24 	xul.dll 	nsDisplayList::PaintForFrame 	layout/base/nsDisplayList.cpp:515
25 	xul.dll 	nsLayoutUtils::PaintFrame 	layout/base/nsLayoutUtils.cpp:1557
26 	xul.dll 	PresShell::Paint 	layout/base/nsPresShell.cpp:6174
27 	xul.dll 	nsRegion::Or 	gfx/src/nsRegion.cpp:840
28 	xul.dll 	nsViewManager::Refresh 	view/src/nsViewManager.cpp:424
29 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:925
30 		@0x54833ab

Comment 1

[Scoobidiver (away)]

Good! You are going to unblock bug 627727.

Stack traces are various.

I can reproduce with an English Minefield (French as first preferred language and French modern dictionary 4.0.3):
bp-73b3f886-26b2-47b1-9971-801ab2110204
bp-3f55d9ad-3408-4f11-9b03-fc2542110204

Comment 2

[Ben]

I am available if you need more information (a full dump, etc), IRC:yaben

Comment 3

[Ben]

The web page on which you may trigger the crash may be as simple as this

<html>
<body>

<form method="post" action="">
<textarea name="commentsbox" cols="40" rows="5"></textarea><br>
<input type="submit" value="Submit" />
</form>

</body>
</html>

And it seems that the trigger of the problem is when you are asking for suggestions on a word where you have a Capital letter followed by an apostrophe (') followed by a Capitalised word.

Have you looked at http://sourceforge.net/tracker/index.php?func=detail&aid=3158994&group_id=143754&atid=756395

Comment 4

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

According to that bug, the issue was introduced in version 1.2.13. Firefox is currently on 1.2.12 (with a few extra bonus fixes). If you want to see if it's the same issue, though, I've made a build with Hunspell 1.3.1 included. You can download it from the link below:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-6b11a6e95151/

Comment 5

[Ben]

I tried your build and I am not able to crash it.

BTW all 3.6.13, 4.0b10, 4.0b11 crash with the same STR, even if 3.6.13 has a different crash signature:

5d2b1ce2-ee81-4344-abd4-122da2110204 args_resolve

*** This problem definitely hurts French users with the new dictionary. It should block final release.

Comment 6

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Ehsan, how many cherry-picked patches do we want to take before we just bite the bullet and just take the Hunspell update?

Comment 7

[chris hofmann]

looks like this missed beta11.

I added hardblocker status whiteboard and this should be marked 2.0 beta N+ 

especially if we take a the full hunspell update.

Comment 8

[Ben]

Note that 4.0b11 also crashes under Linux with the same STR

Crash ID bp-89b858a4-ef1c-4599-ab85-004332110206

Signature: linux-gate.so@0x424

Comment 9

[(no longer active)]

(In reply to comment #6)
> Ehsan, how many cherry-picked patches do we want to take before we just bite
> the bullet and just take the Hunspell update?

Do you have any idea which change in hunspell code fixed this crash?

Comment 10

[(no longer active)]

Attachment: Patch (v1)

This is the upstream patch which fixes this crash (taken from here: <http://sourceforge.net/tracker/index.php?func=detail&aid=3158994&group_id=143754&atid=756395>).  I managed to reproduce this crash locally using the French dictionary, and the fix looks correct (and in my testing, it seems to fix the crash as well).

Ben, it would be very helpful if you can verify this fix on your end too.  I have submitted a try server build with this patch for you to test.  The builds should be available at <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-dad73f9e5311> in a couple of hours.

Comment 11

[(no longer active)]

(Note to Olli: this is a betaN hardblocker...)

Comment 12

[Ben]

Ehsan, sorry but it still crashes, I was able to reproduce within seconds on 2 of my 3 tries on Windows XP or Seven. 

bp-61201154-a3eb-49d2-9394-bd52d2110207    	
bp-4e003440-65d9-4297-9de5-ba7812110207
bp-2b3948ab-3fde-4383-8179-7cf382110207

The signature changed to

mozalloc.dll@0x1a39 maybe because socorro does not have the symbols.

Comment 13

[Scoobidiver (away)]

> This is the upstream patch which fixes this crash
The current version of Hunspell in 4.0 is 1.2.12. The attached patch fixes a bug introduced in 1.2.13 so that is why it still crashes.
See comment 4 and comment 5 to fix this bug.
Is there a risk to have Hunspell 1.3.1 in nightlies to test it before the Beta 12 release?

Comment 14

[Németh László]

Comment on attachment 510203 [details] [diff] [review]
Patch (v1)

This is the same patch as in http://sourceforge.net/tracker/?func=detail&aid=3158994&group_id=143754&atid=756395 to fix the bug introduced in 2010-11-03 in Hunspell CVS (before Hunspell 1.2.13 release). In fact, this is not enough for the full fix: non-BMP Unicode characters in the input can freeze Hunspell during suggestion search. This problem was fixed by Hunspell 1.3.1.

Comment 15

[(no longer active)]

Comment on attachment 510203 [details] [diff] [review]
Patch (v1)

Ah, indeed, the crash still happens.  :(

Nemeth: is there any way for us to cherry-pick the fix to this crash?

Comment 16

[(no longer active)]

Attachment: Patch (v2)

OK, I finally debugged and fixed this on my own!

The problem is in the duplication removal code in Hunspell::suggest <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/hunspell/src/hunspell.cpp#1008>.

In the loop, the variable l is meant to track the number of items in the duplicate-free array.  In case there are no duplicates, l will be equal to ns at the end of the loop.  But if there are duplicates, as is the case for "L'Islam" with the French dictionary, l will be less than ns, and the ns-l last items in the array would be freed memory.  The output conversion loop assumes that there are ns items in the array <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/hunspell/src/hunspell.cpp#1023>, which means that if later on RepList::conv returns non-zero for those items (which have "ZZZZ..." in the debug mode, as the memory will be filled with 0xa5, and valid strings in optimized mode, as free doesn't touch the memory contents), we will try to free this memory again, which results in malloc traces in debug mode and crashes in optimized mode.

The fix is simple, we should synchronize the values of ns and l after the duplicate removal loop.  hunspell-1.3.1 has this fix.  I also added a break to the inner loop to prevent unneeded extra loops (because once we find a match in an iteration, all of the future items are guaranteed to not match, as any other possible duplicates have been removed in previous iterations.  I mainly added that break to ease the merging of this cherry-picked fix for the next hunspell upgrade.

Comment 17

[Ben]

Hello, thanks for your fix. I can test a trybuild !

Comment 18

[(no longer active)]

(In reply to comment #17)
> Hello, thanks for your fix. I can test a trybuild !

Oh, sorry I thought I pushed a build with this, but apparently I did not.  Here's where you can find builds in a few hours: <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-c8b91fca1269>.

Thanks!

Comment 19

[(no longer active)]

(In reply to comment #18)
> (In reply to comment #17)
> > Hello, thanks for your fix. I can test a trybuild !
> 
> Oh, sorry I thought I pushed a build with this, but apparently I did not. 
> Here's where you can find builds in a few hours:
> <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-c8b91fca1269>.
> 
> Thanks!

The builds are all available, BTW.

Comment 20

[Ben]

Ehsan, your trybuild tested ok on Windows XP and Seven with french dictionary.

I hope the patch will be reviewed and approved for beta12 !

Comment 21

[(no longer active)]

I landed the fix, but I'd still like to have Nemeth's review on the patch, if possible.

http://hg.mozilla.org/mozilla-central/rev/fd47f7c9a3b8

Comment 22

[Daniel Veditz [:dveditz]]

Not a blocker for the branch but we'll look at the approval request when it gets the review from nemeth.

Comment 23

[Németh László]

It is ok. Indeed, newer Hunspell versions contain the same patch, too.

Comment 24

[(no longer active)]

Comment on attachment 510406 [details] [diff] [review]
Patch (v2)

I'll take comment 23 as Németh's r+.  :-)

Comment 25

[Németh László]

(In reply to comment #24)
> Comment on attachment 510406 [details] [diff] [review]
> Patch (v2)
> 
> I'll take comment 23 as Németh's r+.  :-)

Thanks, I meant it. (I didn't find the flag.)

Comment 26

[LegNeato]

Comment on attachment 510406 [details] [diff] [review]
Patch (v2)

a=LegNeato for 1.9.2.15

Comment 27

[Ben]

Verified OK on 4.0b12 build 1

Mozilla/5.0 (Windows NT 5.1; rv:2.0b12) Gecko/20100101 Firefox/4.0b12 ID:20110222210221

Comment 28

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1abc3d4ba037

Comment 29

[Daniel Veditz [:dveditz]]

The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.