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

RESOLVED FIXED

Status

Core Graveyard
GFX: Win32
--
critical
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: marsf, Assigned: rbs)

Tracking

({fixed1.8, topcrash})

Trunk
x86
Windows 2000
fixed1.8, topcrash
Dependency tree / graph
Bug Flags:
blocking1.8b5 -
blocking1.8rc1 +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

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

Comment 2

12 years ago
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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WORKSFORME

Comment 3

12 years ago
Bug confirmed on Thunderbird 1.5 Beta 1

Comment 4

12 years ago
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.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
(Reporter)

Comment 5

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

12 years ago
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)
Component: Message Compose Window → Spelling checker
Product: Thunderbird → Core
Summary: Crash if I select the correct spell "hello" from context menu. → Crash if I select the correct spell "hello" from context menu [ @ nsFontMetricsWin::ResolveForwards]
Version: unspecified → 1.8 Branch

Updated

12 years ago
Component: Spelling checker → GFX: Win32

Updated

12 years ago
Assignee: mscott → win32
QA Contact: ian

Comment 7

12 years ago
WFM, debug branch build of Thunderbird on Linux.
Can someone get the regression window please?  Is it branch only?
(Reporter)

Comment 8

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

12 years ago
Thanks, does it occur with the oldest builds available from here?
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 1.8 Branch → Trunk
(Reporter)

Comment 10

12 years ago
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
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.
(In reply to comment #3)
> Bug confirmed on Thunderbird 1.5 Beta 1

Niels, which version of Windows did you use?

Comment 13

12 years ago
I used Windows XP Home SP2 and Firefox 1.5 Beta 1
(Reporter)

Comment 14

12 years ago
(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.
(Assignee)

Comment 15

12 years ago
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.
Depends on: 252970
So can we fix the selection api entry points to flush reflow?
(Assignee)

Comment 17

12 years ago
*** Bug 308181 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 18

12 years ago
*** Bug 308579 has been marked as a duplicate of this bug. ***
This is the #2 (actually #1) topcrasher for TB on 1.8 Branch and for TB 1.5 Beta 1.
Flags: blocking1.8b5?
Keywords: topcrash
(Assignee)

Comment 20

12 years ago
Created attachment 196597 [details] [diff] [review]
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.
Attachment #196597 - Flags: superreview?(bzbarsky)
Attachment #196597 - Flags: review?(bzbarsky)
(Assignee)

Comment 21

12 years ago
Created attachment 196599 [details] [diff] [review]
never crash

take this one instead... I clicked the wrong file in my previous attachment.
Attachment #196597 - Attachment is obsolete: true
Attachment #196599 - Flags: superreview?(bzbarsky)
Attachment #196599 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
Attachment #196597 - Flags: superreview?(bzbarsky)
Attachment #196597 - Flags: review?(bzbarsky)
(Assignee)

Comment 22

12 years ago
Created attachment 196687 [details] [diff] [review]
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.
Attachment #196599 - Attachment is obsolete: true
Attachment #196687 - Flags: superreview?(bzbarsky)
Attachment #196687 - Flags: review?(bzbarsky)
(Assignee)

Comment 23

12 years ago
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...).
Attachment #196599 - Flags: superreview?(bzbarsky)
Attachment #196599 - Flags: review?(bzbarsky)

Updated

12 years ago
Flags: blocking1.8b5? → blocking1.8b5+
Blocks: 309564

Updated

12 years ago
Whiteboard: [needs review+SR bzbarsky]
(Assignee)

Comment 24

12 years ago
I am back and can follow-up in the next two weeks (after which I will be
travelling again).
I'm going to try to get to this in the next day or two...
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.
Attachment #196687 - Flags: superreview?(bzbarsky)
Attachment #196687 - Flags: superreview+
Attachment #196687 - Flags: review?(bzbarsky)
Attachment #196687 - Flags: review+
(Assignee)

Updated

12 years ago
Assignee: win32 → rbs
Whiteboard: [needs review+SR bzbarsky]
(Assignee)

Comment 27

12 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Is it possible this caused bug 310227?
Blocks: 310227
No longer blocks: 310227
Depends on: 310227
Depends on: 310318
This might have also caused bug 310318.
If you cannot fix bug 310318 soon, please back out this patch.
We cannot input Japanese text. This is very serious problem.
(Assignee)

Comment 31

12 years ago
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.

Updated

12 years ago
Attachment #196687 - Flags: approval1.8b5?

Comment 32

12 years ago
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?

Updated

12 years ago
Attachment #196687 - Flags: approval1.8b5?

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 33

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

12 years ago
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?
(Assignee)

Comment 35

12 years ago
Created attachment 198363 [details] [diff] [review]
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

12 years ago
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.)
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

12 years ago
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.

Updated

12 years ago
Attachment #198363 - Flags: review?(bzbarsky)
Attachment #198363 - Flags: approval1.8b5?

Comment 39

12 years ago
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. 
Attachment #198363 - Flags: review?(bzbarsky) → review+
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

12 years ago
we'll try for RC1.
Flags: blocking1.8rc1+
Flags: blocking1.8b5-
Flags: blocking1.8b5+

Comment 42

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

moving request to 1.8rc1 since 1.8b5 has shipped.
Attachment #198363 - Flags: approval1.8b5? → approval1.8rc1?
(Assignee)

Comment 43

12 years ago
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.

Updated

12 years ago
Attachment #198363 - Flags: approval1.8rc1? → approval1.8rc1+

Comment 44

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

Marking this closed.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Product: Core → Core Graveyard

Updated

8 years ago
Summary: Crash if I select the correct spell "hello" from context menu [ @ nsFontMetricsWin::ResolveForwards] → Crash if I select the correct spell "hello" from context menu [@ nsFontMetricsWin::ResolveForwards]
Crash Signature: [@ nsFontMetricsWin::ResolveForwards]
You need to log in before you can comment on or make changes to this bug.