307875 - Crash if I select the correct spell "hello" from context menu [@ nsFontMetricsWin::ResolveForwards]

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Win32, defect)

Description

[Masahiko Imanaka [:marsf]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; ja-JP; rv:1.7.9) Gecko/20050711 Firefox/1.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050906 Thunderbird/1.4 ID:2005090619

On spell checking, select the correct word "hello" from context menu then crashed.
Any other words that listed bellow dosen't crash.
Check from toolbar button dosen't crash, and it can replace to "hello".

That's very fanny!

Reproducible: Always

Steps to Reproduce:
1.Type "hellow" similar to "hello" in text compose area and hit enter.
2.Right click it underlined word.
3.Select the item "hello" from top of context menu.

Actual Results:  
Thunderbird crashed.

Expected Results:  
Replace the bad spell to "hello".

Comment 1

[dan]

bug not reproducible in version 1.5 Beta 1 (20050908) xp home

Comment 2

[Adam Guthrie]

Okay. If it's working in 1.5 then it's probably been fixed somewhere along the
way. If you see it again in 1.5 try to get a talkback ID for the crash and
reopen this bug.

Comment 3

[Niels Madsen]

Bug confirmed on Thunderbird 1.5 Beta 1

Comment 4

[Adam Guthrie]

Okay. We need a talkback ID then. Just uninstall Thunderbird (you won't lose
your e-mails) and chose to do a custom install. Then when it crashes post the
talkback ID here.

Comment 5

[Masahiko Imanaka [:marsf]]

Talkback ID is TB9233061H.

And, I used this build on Windows 2000 SP4.
/thunderbird/nightly/2005-09-10-11-mozilla1.8/thunderbird-1.4.en-US.win32.zip

Comment 6

[Adam Guthrie]

Incident ID: 9233061
Stack Signature	nsFontMetricsWin::ResolveForwards c11b34e7
Product ID	Thunderbird15
Build ID	2005091010
Trigger Time	2005-09-10 22:59:54.0
Platform	Win32
Operating System	Windows NT 5.0 build 2195
Module	thunderbird.exe + (000d167b)
URL visited	
User Comments	Check the spell "hellow" and select "hello" from context menu,
then Thunderbird crashed. 1. Open compose window. 2. Type "hellow" in text
compose area. 3. Right click it. 4. Select correct word "hello". 5. Then crashed.
Since Last Crash	30 sec
Total Uptime	45 sec
Trigger Reason	Access violation
Source File, Line No.
e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/gfx/src/windows/nsFontMetricsWin.cpp,
line 4008
Stack Trace 	
nsFontMetricsWin::ResolveForwards 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/gfx/src/windows/nsFontMetricsWin.cpp,
line 4008]
nsRenderingContextWin::GetWidth 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/gfx/src/windows/nsRenderingContextWin.cpp,
line 1514]
nsTextFrame::GetPointFromOffset 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/generic/nsTextFrame.cpp,
line 4214]
nsTypedSelection::GetPointFromOffset 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/generic/nsSelection.cpp,
line 6747]
nsTypedSelection::GetCachedFrameOffset 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/generic/nsSelection.cpp,
line 5090]
nsCaret::GetCaretRectAndInvert 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/base/nsCaret.cpp,
line 988]
nsCaret::DrawAtPositionWithHint 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/base/nsCaret.cpp,
line 704]
nsCaret::DrawCaret 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/base/nsCaret.cpp,
line 908]
nsCaret::StartBlinking 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/base/nsCaret.cpp,
line 481]
nsTypedSelection::NotifySelectionListeners 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/generic/nsSelection.cpp,
line 7295]
nsSelection::NotifySelectionListeners 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/generic/nsSelection.cpp,
line 3007]
mozInlineSpellChecker::ReplaceWord 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp,
line 391]
XPTC_InvokeByIndex 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 102]
XPCWrappedNative::CallMethod 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,
line 2139]
XPC_WN_CallMethod 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1402]
js_Invoke 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 1163]
js_Interpret 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 3459]
js_Invoke 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 1183]
js_InternalInvoke 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/js/src/jsinterp.c,
line 1260]
JS_CallFunctionValue 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/js/src/jsapi.c, line
4017]
nsJSContext::CallEventHandler 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/dom/src/base/nsJSEnvironment.cpp,
line 1414]
nsJSEventListener::HandleEvent 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/dom/src/events/nsJSEventListener.cpp,
line 185]
nsEventListenerManager::HandleEventSubType 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1685]
nsEventListenerManager::HandleEvent 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1786]
nsXULElement::HandleDOMEvent 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/content/xul/content/src/nsXULElement.cpp,
line 2201]
PresShell::HandleDOMEventWithTarget 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6477]
nsMenuFrame::Execute 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/xul/base/src/nsMenuFrame.cpp,
line 1621]
nsMenuFrame::HandleEvent 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/xul/base/src/nsMenuFrame.cpp,
line 452]
PresShell::HandleEventInternal 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6420]
PresShell::HandleEvent 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6215]
nsViewManager::HandleEvent 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 2559]
nsViewManager::DispatchEvent 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 2246]
HandleEvent 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/view/src/nsView.cpp,
line 174]
nsWindow::DispatchEvent 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1252]
nsWindow::DispatchMouseEvent 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 5981]
ChildWindow::DispatchMouseEvent 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 6232]
nsWindow::WindowProc 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1434]
USER32.dll + 0x2a3d0 (0x77e0a3d0)
USER32.dll + 0x4605 (0x77de4605)
USER32.dll + 0xa7ba (0x77dea7ba)
nsAppStartup::Run 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,
line 146]
main 
[e:/builds/tinderbox/Tb-Mozilla1.8/WINNT_5.0_Depend/mozilla/mail/app/nsMailApp.cpp,
line 62]
KERNEL32.DLL + 0x2893d (0x77e7893d)

Comment 7

[Mats Palmgren (inactive)]

WFM, debug branch build of Thunderbird on Linux.
Can someone get the regression window please?  Is it branch only?

Comment 8

[Masahiko Imanaka [:marsf]]

I have tested on latest trunk build with new profile.
The error was occured too.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050911
Thunderbird/1.6a1 ID:2005091106

Comment 9

[Mats Palmgren (inactive)]

Thanks, does it occur with the oldest builds available from here?
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/

Comment 10

[Masahiko Imanaka [:marsf]]

Yes, old trunk build also.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050828
Thunderbird/1.6a1 ID:2005082808

Comment 11

[Mats Palmgren (inactive)]

Thank you very much for your help with testing. There are even older builds
available here:  http://archive.mozilla.org/pub/thunderbird/nightly/
It would help to know if this also occurs in very old builds... and if it
doesn't, when the crash first started to appear.

Comment 12

[Mats Palmgren (inactive)]

(In reply to comment #3)
> Bug confirmed on Thunderbird 1.5 Beta 1

Niels, which version of Windows did you use?

Comment 13

[Niels Madsen]

I used Windows XP Home SP2 and Firefox 1.5 Beta 1

Comment 14

[Masahiko Imanaka [:marsf]]

(In reply to comment #11)
The error was occured since "version 0.6+ (20050203)".

This is the first build that Inline Spell Checker function was checked in at Bug
278312.

Comment 15

[rbs]

This is the same problem as bug 252970.

mozInlineSpellChecker::ReplaceWord() whirls into the selection which ulimately
does: 

nsFontMetricsWin::ResolveForwards
nsRenderingContextWin::GetWidth 
nsTextFrame::GetPointFromOffset 
nsTypedSelection::GetPointFromOffset
[...]
mozInlineSpellChecker::ReplaceWord

The crash arises when there are still pending reflows.  Such crashes are still
in the selection code because it is the primary consumer of APIs that expect
frames to have been reflowed and are clean (bug 302728). In fact, I am even
amazed that there aren't many of them.

Comment 16

[Boris Zbarsky [:bzbarsky]]

So can we fix the selection api entry points to flush reflow?

Comment 17

[rbs]

*** Bug 308181 has been marked as a duplicate of this bug. ***

Comment 18

[rbs]

*** Bug 308579 has been marked as a duplicate of this bug. ***

Comment 19

[Frank Wein [:mcsmurf]]

This is the #2 (actually #1) topcrasher for TB on 1.8 Branch and for TB 1.5 Beta 1.

Comment 20

[rbs]

Attachment: never crash

I propose experimenting this patch since we are still early in 1.9. (Add
yourself to the #ifdef if your are interested in getting the warnings.)

Rationale for the patch:
- Better fail than crash.
- See it as null checks--you can never be too sure of calls beyond your
control.
- Those who get there should not have gone there in the first place. I noted
that most of the calls arise because of selection.Collapse(), which is why
these out-of-sync issues have not been too damaging so far (in terms of the
user experience) even when the crashes do not happen.
- It is messy (and costly) to flush reflows all over the place in the
selection.

Comment 21

[rbs]

Attachment: never crash

take this one instead... I clicked the wrong file in my previous attachment.

Comment 22

[rbs]

Attachment: simpler

Take 3. Simplify the |if| check because first-reflow implies dirty. 

I was using NS_ASSERTION instead of NS_WARNING and the !isZero test only helps
to cut the number of assertions/warnings.

Comment 23

[rbs]

Comment on attachment 196599 [details] [diff] [review]
never crash

I am off for a trip until the end of the week. If you r/sr before then, please
follow-up with the check-in (and the the unexpected...).

Comment 24

[rbs]

I am back and can follow-up in the next two weeks (after which I will be
travelling again).

Comment 25

[Boris Zbarsky [:bzbarsky]]

I'm going to try to get to this in the next day or two...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196687 [details] [diff] [review]
simpler

So...  For trunk this looks ok, I guess, but I'd like a followup on documenting
which methods expect layout to be flushed, seeing asserts to that effect added
in general, and then fixing callers to flush as needed.  Or addressing this in
some other way, if necessary...

For branch, this looks pretty scary, but if we're hitting these crashes a lot
maybe it's worth trying after some trunk bake time.

Comment 27

[rbs]

Checked in the trunk.

Will wait for it to bake on the trunk a bit before asking for approval on the
branch. I don't feel the patch is too scary (compared to those crashes that are
_guaranted_ to be there otherwise). It is common sense to me. In fact, I feel
this defense (with a hard assertion instead of a mere warning) should have been
there in the very early days to assist the development. People have since piled
things up without enforcing the constraint and/or realising the consequences.

Bug 302728 is meant for the follow-up.

Comment 28

[Stephen Donner [:stephend] Not actively reading bugmail]

Is it possible this caused bug 310227?

Comment 29

[Boris Zbarsky [:bzbarsky]]

This might have also caused bug 310318.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If you cannot fix bug 310318 soon, please back out this patch.
We cannot input Japanese text. This is very serious problem.

Comment 31

[rbs]

Only two bugs so far? Not too bad... I was rubbing my hands for nastier
things... I will take a look at them. Perhaps we might be able to secure the
much bigger reward of this fix after all.

Masayuki, simply change the NS_WARNING to NS_ASSERTION, and add yourself to the
#ifdef. This way, you will quickly see the precise spot where to flush the
reflow. The assertion might also give you another nice and lightweight idea to
resolve the IME matter.

Of course, the backout is there if the issues can't be resolved soon enough.

Comment 32

[Asa Dotzler [:asa]]

RBS and Masayuki, time is running out for 1.5 and we need traction on those
regressions before we can consider this for the branch. Can you give us an
update here today?

Comment 33

[rbs]

I have checked a fix for the two known regressions so far. I simply disabled
parts of the patch.

Fortunately, the parts that are still active are the parts that protect against
the reported crashes. We still need a few days to ascertain if things have settled.

Comment 34

[Scott MacGregor]

rbs, can you post a branch only patch that backs out the changes you no longer
need so we can get a look at what exactly we need to change on the branch?

Comment 35

[rbs]

Attachment: patch for the branch

Here is a branch only patch without the changes that caused the regressions.
Note that althougth this is a topcrasher, I can't help be a bit nervous about
this patch because it hasn't got enough baking on the trunk yet. Unfortunately,
the beta is going to take the focus and nobody is going to bother to
download/test the trunk anymore when the beta is out.

Comment 36

[Asa Dotzler [:asa]]

peterv, roc, dbaron, bz, what do you guys think about taking this change without
much opportunity for testing before we ship beta (maybe one day of builds.)

Comment 37

[Boris Zbarsky [:bzbarsky]]

Pretty much what I said in comment 26.  It's pretty scary to me, but the thing
that really scared me (things that would consistently fail because the always
happen when the frame is dirty) is what would have gotten caught first (and some
did).  So it might be ok if people test the heck out of everything
selection-related during that one day.  Especially on pages with animated stuff
(marquee, tickers, that sort of thing).

Past beta 1, by the way, I think we should make these real assertions on the
trunk (I know that sucks on windows but there is no way anyone will notice
warnings -- I sure don't in the mass of debug spew I get from debug builds) and
start fixing callers (trunk and possibly branch, depending on the caller and the
patch) so that we can make sure issues really don't arise.

Comment 38

[Patrick Brunschwig]

If it helps you -- I had applied the "simpler" patch to my self-built
Thunderbird 1.5b1, and I'm using it since Sept. 22 for my daily mail. While TB
1.5b1 without that patch was almost unusable when replying to emails, the
patched version is much more stable.

Comment 39

[Scott MacGregor]

we're going to take this for the Release Candidate right after the branch
re-opens for post beta 2 work. The risk was just too high to take it without any
time to test it. 

Comment 40

[Boris Zbarsky [:bzbarsky]]

This had also caused another regression:  Highlighting text via keyboard and
then using Ctrl-X and Ctrl-V to move it made the selection all weird.  The
backout that fixed the other regressions helped those bugs too...

Comment 41

[Asa Dotzler [:asa]]

we'll try for RC1.

Comment 42

[Asa Dotzler [:asa]]

Comment on attachment 198363 [details] [diff] [review]
patch for the branch

moving request to 1.8rc1 since 1.8b5 has shipped.

Comment 43

[rbs]

As per my comment 24, I am off again for a trip and will be away for a week.
Feel free to check in the patch at the opportune time.

Comment 44

[Scott MacGregor]

ok, this patch for the branch is now checked in. 

Marking this closed.