344560 - Firefox crashes upon visiting site [@ nsTextInputSelectionImpl::ScrollSelectionIntoView]

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Tadej]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060707 Firefox/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060707 Firefox/2.0b1

It was working without problems on 1.5.0.4 and older.

Reproducible: Always

Steps to Reproduce:
1. Visit this site
2. http://www.gwshack.us/0f002
3. FF crash, when small icons should show

Actual Results:  
Firefox crashes everytime on site visit

Expected Results:  
It should show small icons and when you put your mouse on it should popup hoovering message, saying description of icon.

Comment 1

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Stack Signature	 0x040801f3 d74a28e0
Product ID	Firefox2
Build ID	2006071204
Trigger Time	2006-07-13 11:05:23.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	
URL visited	
User Comments	
Since Last Crash	48 sec
Total Uptime	1882 sec
Trigger Reason	Access violation
Source File, Line No.	N/A
Stack Trace 	
0x040801f3
nsTextInputSelectionImpl::ScrollSelectionIntoView  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/forms/nsTextControlFrame.cpp, line 708]
nsEditor::ScrollSelectionIntoView  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/editor/libeditor/base/nsEditor.cpp, line 2606]
nsEditor::EndPlaceHolderTransaction  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/editor/libeditor/base/nsEditor.cpp, line 939]
nsAutoPlaceHolderBatch::~nsAutoPlaceHolderBatch  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/editor/libeditor/base\nsEditorUtils.h, line 66]
nsTextControlFrame::SetValue  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/forms/nsTextControlFrame.cpp, line 3250]
nsTextControlFrame::SetProperty  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/forms/nsTextControlFrame.cpp, line 2416]
nsHTMLTextAreaElement::SetValue  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/html/content/src/nsHTMLTextAreaElement.cpp, line 435]
XPCWrappedNative::CallMethod  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2160]
XPC_WN_GetterSetter  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1474]
js_Invoke  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1349]
js_InternalInvoke  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1447]
js_InternalGetOrSet  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1507]
js_SetProperty  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsobj.c, line 3370]
js_Interpret  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 3835]
js_Execute  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1599]
obj_eval  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsobj.c, line 1346]
js_Invoke  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1349]
js_Interpret  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 4085]
js_Invoke  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1368]
nsXPCWrappedJSClass::CallMethod  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp, line 1415]
nsXPCWrappedJS::CallMethod  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp, line 468]
SharedStub  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 147]
nsXMLHttpRequest::ChangeState  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsXMLHttpRequest.cpp, line 1891]
nsXMLHttpRequest::RequestCompleted  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsXMLHttpRequest.cpp, line 1431]
nsXMLHttpRequest::OnStopRequest  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsXMLHttpRequest.cpp, line 1376]
nsStreamListenerTee::OnStopRequest  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/netwerk/base/src/nsStreamListenerTee.cpp, line 65]
nsInputStreamPump::OnStateStop  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/netwerk/base/src/nsInputStreamPump.cpp, line 564]

Comment 2

[Ria Klaassen (not reading all bugmail)]

No crash in 1.9a1_2006070314, crash in 1.9a1_2006070322.
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-07-03+13%3A00&maxdate=2006-07-03+23%3A00

Comment 3

[Adam Guthrie]

So Ria's regression range points to bug 339066. Do you have any idea what's going on here, roc?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

ON trunk, I don't see any icons and I don't crash. Is this branch only?

Comment 5

[Adam Guthrie]

Nope. Both Ria and I crash on trunk.

Comment 6

[bio_tube]

Just visited the site with Firefox 2.0b1. I think it might be a problem with the site's Javascript as it didn't crash until after I told NoScript to let it execute.

Comment 7

[Martijn Wargers (dead)]

Attachment: testcase (crashes on load)

Ok, I managed to minimise the crashing page to this.
But the testcase crashes in a different area, talkback ID: TB21249564H
0x72612050
nsQueryReferent::operator()   nsCOMPtr<nsIEditor>::nsCOMPtr<nsIEditor>   0x0012f074
While minimising, I also crashed with this talkback ID: TB21246646E
which has nsTextControlFrame::SetValue as top stack.
which is the same stacktrace as bug 344884, so that one might be related.
I've checked that the testcase has the same regression range as this bug.

Comment 8

[Martijn Wargers (dead)]

(I just hope the testcase crash is basically the same as seen on http://www.gwshack.us/0f002 )

Comment 9

[Brett Wilson]

I narrowed this down to the GetRangeForWord call in extensions/spellcheck/src/mozInlineSpellCheck.cpp, mozInlineSpellChecker::SpellCheckAfterEditorChange

The first time, the QueryReferent works, the second time it doesn't. I could not figure out why.


  printf("after edit weak pointer 1.3 = %p\n", (void*)(nsIWeakReference*)mEditor);
  nsCOMPtr<nsIEditor> ep13 (do_QueryReferent(mEditor)); // ERASEME
  printf("Editor = %p\n", (void*)(nsIEditor*)ep13);

  nsCOMPtr<nsIDOMRange> wordRange;
  rv = wordUtil.GetRangeForWord(anchorNode, anchorOffset,
                                getter_AddRefs(wordRange));
   NS_ENSURE_SUCCESS(rv, rv);
#ifdef DEBUG_INLINESPELL
  nsString wordRangeText;
  wordRange->ToString(wordRangeText);
  printf("->Editor change, current word is \"%s\"\n",
         NS_ConvertUTF16toUTF8(wordRangeText).get());
#endif

  printf("after edit weak pointer 1.6 = %p\n", (void*)(nsIWeakReference*)mEditor);
  nsCOMPtr<nsIEditor> ep16 (do_QueryReferent(mEditor)); // ERASEME
  printf("Editor = %p\n", (void*)(nsIEditor*)ep16);

Comment 10

[Brian Ryner (not reading)]

Purify log of the crash:
[E] FMR: Free memory read in mozInlineSpellChecker::SkipSpellCheckForNode(nsIDOMNode *,int *) {1 occurrence}
        Reading 4 bytes from 0x077ebbf8 (4 bytes at 0x077ebbf8 illegal)
        Address 0x077ebbf8 is 24 bytes into a 64 byte block at 0x077ebbe0
        Address 0x077ebbf8 points to a C++ new block in heap 0x01cc0000
        Thread ID: 0xdd8
        Error location
            mozInlineSpellChecker::SkipSpellCheckForNode(nsIDOMNode *,int *) [c:\builds\mozilla\ff2\mozilla\extensions\spellcheck\src\mozinlinespellchecker.cpp:808]
            mozInlineSpellChecker::DoSpellCheck(mozInlineSpellWordUtil&,nsIDOMRange *,nsIDOMRange *,nsIDOMRange *,nsISelection *) [c:\builds\mozilla\ff2\mozilla\extensions\spellcheck\src\mozinlinespellchecker.cpp:960]
            mozInlineSpellChecker::SpellCheckAfterEditorChange(int,nsISelection *,nsIDOMNode *,int,nsIDOMNode *,int,nsIDOMNode *,int) [c:\builds\mozilla\ff2\mozilla\extensions\spellcheck\src\mozinlinespellchecker.cpp:384]
            nsEditor::HandleInlineSpellCheck(int,nsISelection *,nsIDOMNode *,int,nsIDOMNode *,int,nsIDOMNode *,int) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\base\nseditor.cpp:5452]
            nsTextEditRules::AfterEdit(int,short) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\text\nstexteditrules.cpp:239]
            nsPlaintextEditor::EndOperation(void) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\text\nsplaintexteditor.cpp:1664]
            nsAutoRules::~nsAutoRules(void) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\base\nseditorutils.h:123]
            nsPlaintextEditor::InsertText(nsAString_internal const&) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\text\nsplaintexteditor.cpp:743]
            nsTextControlFrame::SetValue(nsAString_internal const&) [c:\builds\mozilla\ff2\mozilla\layout\forms\nstextcontrolframe.cpp:3254]
            nsTextControlFrame::SetProperty(nsPresContext *,nsIAtom *,nsAString_internal const&) [c:\builds\mozilla\ff2\mozilla\layout\forms\nstextcontrolframe.cpp:2419]
        Allocation location
            new(UINT)      [f:\vs70builds\3077\vc\crtbld\crt\src\newop.cpp:10]
            mozInlineSpellCheckerConstructor [c:\builds\mozilla\ff2\mozilla\extensions\spellcheck\src\mozspellcheckerfactory.cpp:90]
            nsGenericFactory::CreateInstance(nsISupports *,nsID const&,void * *) [c:\builds\mozilla\ff2\obj\fx-opt\xpcom\build\nsgenericfactory.cpp:79]
            nsComponentManagerImpl::CreateInstanceByContractID(char const*,nsISupports *,nsID const&,void * *) [c:\builds\mozilla\ff2\mozilla\xpcom\components\nscomponentmanager.cpp:1981]
            CallCreateInstance(char const*,nsISupports *,nsID const&,void * *) [c:\builds\mozilla\ff2\obj\fx-opt\xpcom\build\nscomponentmanagerutils.cpp:170]
            nsCreateInstanceByContractID::()(nsID const&,void * *)const [c:\builds\mozilla\ff2\obj\fx-opt\xpcom\build\nscomponentmanagerutils.cpp:210]
            nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&,nsID const&) [c:\builds\mozilla\ff2\obj\fx-opt\xpcom\build\nscomptr.cpp:150]
            nsCOMPtr<nsIInlineSpellChecker>::=(nsCOMPtr_helper const&) [c:\builds\mozilla\ff2\obj\fx-opt\dist\include\xpcom\nscomptr.h:780]
            nsEditor::GetInlineSpellCheckerOptionally(int,nsIInlineSpellChecker * *) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\base\nseditor.cpp:1329]
            nsTextControlFrame::SetEnableRealTimeSpell(int) [c:\builds\mozilla\ff2\mozilla\layout\forms\nstextcontrolframe.cpp:1702]
        Free location
            _purecall      [f:\vs70builds\3077\vc\crtbld\crt\src\newop.cpp]
            mozInlineSpellChecker::`vector deleting destructor'(UINT) [C:\builds\mozilla\ff2\obj\fx-opt\dist\bin\components\spellchk.dll]
            mozInlineSpellChecker::Release(void) [c:\builds\mozilla\ff2\mozilla\extensions\spellcheck\src\mozinlinespellchecker.cpp:98]
            nsCOMPtr_base::assign_assuming_AddRef(nsISupports *) [c:\builds\mozilla\ff2\mozilla\xpcom\glue\nscomptr.h:531]
            nsCOMPtr_base::assign_with_AddRef(nsISupports *) [c:\builds\mozilla\ff2\obj\fx-opt\xpcom\build\nscomptr.cpp:89]
            nsEditor::PreDestroy(void) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\base\nseditor.cpp:483]
            nsTextControlFrame::PreDestroy(nsPresContext *) [c:\builds\mozilla\ff2\mozilla\layout\forms\nstextcontrolframe.cpp:1375]
            DoDeletingFrameSubtree [c:\builds\mozilla\ff2\mozilla\layout\base\nscssframeconstructor.cpp:9654]
            DoDeletingFrameSubtree [c:\builds\mozilla\ff2\mozilla\layout\base\nscssframeconstructor.cpp:9688]
            DeletingFrameSubtree [c:\builds\mozilla\ff2\mozilla\layout\base\nscssframeconstructor.cpp:9737]

[E] IPR: Invalid pointer read in nsQueryReferent::()(nsID const&,void * *)const {1 occurrence}
        Reading 4 bytes from 0xaeaeaeae (4 bytes at 0xaeaeaeae illegal)
        Address 0xaeaeaeae points into invalid memory 
        Thread ID: 0xdd8
        Error location
            nsQueryReferent::()(nsID const&,void * *)const [c:\builds\mozilla\ff2\obj\fx-opt\xpcom\build\nsweakreference.cpp:52]
            nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&,nsID const&) [c:\builds\mozilla\ff2\obj\fx-opt\xpcom\build\nscomptr.cpp:150]
            nsCOMPtr<nsIEditor>::nsCOMPtr<nsIEditor>(nsCOMPtr_helper const&) [c:\builds\mozilla\ff2\obj\fx-opt\dist\include\xpcom\nscomptr.h:694]
            mozInlineSpellChecker::SkipSpellCheckForNode(nsIDOMNode *,int *) [c:\builds\mozilla\ff2\mozilla\extensions\spellcheck\src\mozinlinespellchecker.cpp:808]
            mozInlineSpellChecker::DoSpellCheck(mozInlineSpellWordUtil&,nsIDOMRange *,nsIDOMRange *,nsIDOMRange *,nsISelection *) [c:\builds\mozilla\ff2\mozilla\extensions\spellcheck\src\mozinlinespellchecker.cpp:960]
            mozInlineSpellChecker::SpellCheckAfterEditorChange(int,nsISelection *,nsIDOMNode *,int,nsIDOMNode *,int,nsIDOMNode *,int) [c:\builds\mozilla\ff2\mozilla\extensions\spellcheck\src\mozinlinespellchecker.cpp:384]
            nsEditor::HandleInlineSpellCheck(int,nsISelection *,nsIDOMNode *,int,nsIDOMNode *,int,nsIDOMNode *,int) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\base\nseditor.cpp:5452]
            nsTextEditRules::AfterEdit(int,short) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\text\nstexteditrules.cpp:239]
            nsPlaintextEditor::EndOperation(void) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\text\nsplaintexteditor.cpp:1664]
            nsAutoRules::~nsAutoRules(void) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\base\nseditorutils.h:123]

[E] EXU: Unhandled exception in nsQueryReferent::()(nsID const&,void * *)const {1 occurrence}
        Exception code: 0xc0000005 [Error: access violation reading from freed memory]
        Exception address: nsQueryReferent::()(nsID const&,void * *)const [c:\builds\mozilla\ff2\obj\fx-opt\xpcom\build\nsweakreference.cpp:52]
        Filter: WinMainCRTStartup [f:\vs70builds\3077\vc\crtbld\crt\src\crtexe.c:409]
        Exception location
            nsQueryReferent::()(nsID const&,void * *)const [c:\builds\mozilla\ff2\obj\fx-opt\xpcom\build\nsweakreference.cpp:52]
            nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&,nsID const&) [c:\builds\mozilla\ff2\obj\fx-opt\xpcom\build\nscomptr.cpp:150]
            nsCOMPtr<nsIEditor>::nsCOMPtr<nsIEditor>(nsCOMPtr_helper const&) [c:\builds\mozilla\ff2\obj\fx-opt\dist\include\xpcom\nscomptr.h:694]
            mozInlineSpellChecker::SkipSpellCheckForNode(nsIDOMNode *,int *) [c:\builds\mozilla\ff2\mozilla\extensions\spellcheck\src\mozinlinespellchecker.cpp:808]
            mozInlineSpellChecker::DoSpellCheck(mozInlineSpellWordUtil&,nsIDOMRange *,nsIDOMRange *,nsIDOMRange *,nsISelection *) [c:\builds\mozilla\ff2\mozilla\extensions\spellcheck\src\mozinlinespellchecker.cpp:960]
            mozInlineSpellChecker::SpellCheckAfterEditorChange(int,nsISelection *,nsIDOMNode *,int,nsIDOMNode *,int,nsIDOMNode *,int) [c:\builds\mozilla\ff2\mozilla\extensions\spellcheck\src\mozinlinespellchecker.cpp:384]
            nsEditor::HandleInlineSpellCheck(int,nsISelection *,nsIDOMNode *,int,nsIDOMNode *,int,nsIDOMNode *,int) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\base\nseditor.cpp:5452]
            nsTextEditRules::AfterEdit(int,short) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\text\nstexteditrules.cpp:239]
            nsPlaintextEditor::EndOperation(void) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\text\nsplaintexteditor.cpp:1664]
            nsAutoRules::~nsAutoRules(void) [c:\builds\mozilla\ff2\mozilla\editor\libeditor\base\nseditorutils.h:123]

Comment 11

[Brian Ryner (not reading)]

To actually understand why it crashes, you need a longer stack, like this one.  Calling nsComputedDOMStyle::GetPropertyValue() can actually cause the spellchecker object to be synchronously released as the editor is torn down.

>	spellchk.dll!mozInlineSpellChecker::~mozInlineSpellChecker()  Line 113	C++
 	spellchk.dll!mozInlineSpellChecker::`scalar deleting destructor'()  + 0xf	C++
 	spellchk.dll!mozInlineSpellChecker::Release()  Line 98 + 0xcd	C++
 	editor.dll!nsCOMPtr<nsIInlineSpellChecker>::assign_assuming_AddRef(nsIInlineSpellChecker * newPtr=0x00000000)  Line 569	C++
 	editor.dll!nsCOMPtr<nsIInlineSpellChecker>::assign_with_AddRef(nsISupports * rawPtr=0x00000000)  Line 1225	C++
 	editor.dll!nsCOMPtr<nsIInlineSpellChecker>::operator=(nsIInlineSpellChecker * rhs=0x00000000)  Line 714	C++
 	editor.dll!nsEditor::PreDestroy()  Line 487	C++
 	gklayout.dll!nsTextControlFrame::PreDestroy(nsPresContext * aPresContext=0x055b75a8)  Line 1380	C++
 	gklayout.dll!nsTextControlFrame::RemovedAsPrimaryFrame(nsPresContext * aPresContext=0x055b75a8)  Line 1466	C++
 	gklayout.dll!DoDeletingFrameSubtree(nsPresContext * aPresContext=0x055b75a8, nsFrameManager * aFrameManager=0x046afa0c, nsVoidArray & aDestroyQueue={...}, nsIFrame * aRemovedFrame=0x0463aa78, nsIFrame * aFrame=0x0463ac6c)  Line 9655	C++
 	gklayout.dll!DoDeletingFrameSubtree(nsPresContext * aPresContext=0x055b75a8, nsFrameManager * aFrameManager=0x046afa0c, nsVoidArray & aDestroyQueue={...}, nsIFrame * aRemovedFrame=0x0463aa78, nsIFrame * aFrame=0x0463aa78)  Line 9667 + 0x19	C++
 	gklayout.dll!DeletingFrameSubtree(nsPresContext * aPresContext=0x055b75a8, nsFrameManager * aFrameManager=0x046afa0c, nsIFrame * aFrame=0x0463aa78)  Line 9737 + 0x19	C++
 	gklayout.dll!nsCSSFrameConstructor::ContentRemoved(nsIContent * aContainer=0x047b3598, nsIContent * aChild=0x055b7db0, int aIndexInContainer=1, int aInReinsertContent=0)  Line 9959 + 0x11	C++
 	gklayout.dll!nsCSSFrameConstructor::RecreateFramesForContent(nsIContent * aContent=0x055b7db0)  Line 11970 + 0x1b	C++
 	gklayout.dll!nsCSSFrameConstructor::RestyleElement(nsIContent * aContent=0x055b7db0, nsIFrame * aPrimaryFrame=0x0463aa78, nsChangeHint aMinHint=0)  Line 10505	C++
 	gklayout.dll!nsCSSFrameConstructor::ProcessOneRestyle(nsIContent * aContent=0x055b7db0, nsReStyleHint aRestyleHint=eReStyle_Self, nsChangeHint aChangeHint=0)  Line 13948	C++
 	gklayout.dll!nsCSSFrameConstructor::ProcessPendingRestyles()  Line 13997	C++
 	gklayout.dll!PresShell::FlushPendingNotifications(mozFlushType aType=Flush_Frames)  Line 5344	C++
 	gklayout.dll!nsDocument::FlushPendingNotifications(mozFlushType aType=Flush_Frames)  Line 4293	C++
 	gklayout.dll!nsHTMLDocument::FlushPendingNotifications(mozFlushType aType=Flush_Frames)  Line 1272	C++
 	gklayout.dll!nsComputedDOMStyle::GetPropertyCSSValue(const nsAString_internal & aPropertyName={...}, nsIDOMCSSValue * * aReturn=0x0012dfa4)  Line 307	C++
 	gklayout.dll!nsComputedDOMStyle::GetPropertyValue(const nsAString_internal & aPropertyName={...}, nsAString_internal & aReturn={...})  Line 280 + 0x28	C++
 	spellchk.dll!IsBreakElement(nsIDOMViewCSS * aDocView=0x0448271c, nsIDOMNode * aNode=0x045bd06c)  Line 500 + 0x3d	C++
 	spellchk.dll!mozInlineSpellWordUtil::BuildSoftText()  Line 556 + 0x1b	C++
 	spellchk.dll!mozInlineSpellWordUtil::EnsureWords()  Line 263	C++
 	spellchk.dll!mozInlineSpellWordUtil::GetRangeForWord(nsIDOMNode * aWordNode=0x04778044, int aWordOffset=5, nsIDOMRange * * aRange=0x0012e2f4)  Line 289	C++
 	spellchk.dll!mozInlineSpellChecker::SpellCheckAfterEditorChange(int aAction=1001, nsISelection * aSelection=0x045bd0f0, nsIDOMNode * aPreviousSelectedNode=0x045bd06c, int aPreviousSelectedOffset=0, nsIDOMNode * aStartNode=0x00000000, int aStartOffset=0, nsIDOMNode * aEndNode=0x00000000, int aEndOffset=0)  Line 326 + 0x30	C++
 	editor.dll!nsEditor::HandleInlineSpellCheck(int action=1001, nsISelection * aSelection=0x045bd0f0, nsIDOMNode * previousSelectedNode=0x045bd06c, int previousSelectedOffset=0, nsIDOMNode * aStartNode=0x00000000, int aStartOffset=0, nsIDOMNode * aEndNode=0x00000000, int aEndOffset=0)  Line 5452 + 0x49	C++

Comment 12

[Brett Wilson]

This is really two bugs. The stack traces that Brian posted are not a crash at ScrollSelectionIntoView. They show a different crash in editor cleanup.

I filed bug 346748 for the spellchecker problem.

With a patch that fixes that bug, this bug still appears, which seems to be independent of spellchecking. It happens even when I don't create any spellcheck objects.

Comment 13

[Brett Wilson]

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

Comment 14

[Brett Wilson]

The problem is that the spellchecker accesses style information in the textbox to determine whether blocks should be considered word breaks or not. This cases DOM notifications to be flushed, which could conceivably delete the editor and the textbox.

Bug 346748 addresses the problem for the spellchecker, but the problem is that other people calling the editor, such as setting text in the textbox, can cause the textbox to be deleted by doing that call. Since they aren't expecting it, they crash.

Comment 15

[Brett Wilson]

There are several possibilities.

1. Hack the style system so we can get some possibly out-of-date style without flushing notifications. It is very unlikely that the notifications will change the block-iness of the element, so this should be acceptable. The hack in the style system might be ugly.

b. Somehow only process style events and not process deletion events on style notification flushes so nothing can get deleted.

iii. Make every caller of any operation that could trigger a spellcheck (and hence deletion) either tolerant of that or flush before doing the operation.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should SetValueInternal flush pending notifications before getting and calling into the frame so that anything that would destroy the frame happens in the before that call rather in the middle of it?  Or does the style change happen within that stuff?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Another possibility is that IsBreakElement could be checking the HTML concept of block rather than the CSS concept of block.

Comment 18

[Brett Wilson]

Attachment: Ginormous patch

This patch makes all important spellchecking things happen on an event. This fixes the crashes and also improves responsiveness. I also cleaned up the header files.

Comment 19

[Brett Wilson]

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

Comment 20

[Brian Ryner (not reading)]

Comment on attachment 231964 [details] [diff] [review]
Ginormous patch 

>+nsresult
>+mozInlineSpellStatus::GetDocumentRange(nsIDOMDocumentRange** aDocRange)
>+{
>+  nsresult rv;
>+  if (! mSpellChecker->mEditor)
>+    return NS_ERROR_UNEXPECTED;
>+
>+  nsCOMPtr<nsIEditor> editor = do_QueryReferent(mSpellChecker->mEditor, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIDOMDocument> domDoc;
>+  rv = editor->GetDocument(getter_AddRefs(domDoc));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIDOMDocumentRange> docRange = do_QueryInterface(domDoc, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  docRange.swap(*aDocRange);

You might want to explicitly set *aDocRange = nsnull before the swap, just in case someone decides to do this, which is legal XPCOM:

nsIDOMDocumentRange* range;   // uninitialized
GetDocumentRange(&range);   // will likely crash when docRange goes out of scope

Same thing below for PositionToCollapsedRange.

> // mozInlineSpellChecker::SpellCheckAfterEditorChange
>+//
>+//    Called by the editor when nearly anything happens to change the content
>+//    of the content.

The content of the content?  :^)

>+//
>+//    The start and end positions specify a range for the thing that happened,
>+//    but these are usually NULL, even when you'd think they would be useful
>+//    because you want the range (for example, pasting).
> 

So what do we do instead when they're null?
>+//    WE IGNORE THE INPUT PARAMETER. This function definition is presserved
>+//    for compatability, but we will always check the spellcheck selection.

nit: "compatibility", and might be clearer to say "function signature", or just "parameter".

>+nsresult
>+mozInlineSpellChecker::DoSpellCheckSelection(mozInlineSpellWordUtil& aWordUtil,
>+                                             nsISelection* aSpellCheckSelection,
>+                                             mozInlineSpellStatus* aStatus)
> {
>   nsresult rv;
> 
>-  // the spell check selection includes all misspelled words
>-  nsCOMPtr<nsISelection> spellCheckSelection;
>-  rv = GetSpellCheckSelection(getter_AddRefs(spellCheckSelection));
>+  // clear out mNumWordsInSpellSelection since we'll be rebuilding the ranges.
>+  mNumWordsInSpellSelection = 0;
>+
>+  // Since we could be modifying the ranges for the spellCheckSelection while
>+  // looping on the spell check selection, keep a separate array of range
>+  // elements inside the selection
>+  nsCOMPtr <nsIMutableArray> ranges =
>+      do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);

Do you really need an nsI[Mutable]Array here?  Can't you just use a nsCOMArray and avoid all the QueryElementAt overhead?

>@@ -1094,22 +1405,36 @@ mozInlineSpellChecker::ResumeCheck(mozIn
> nsresult
> mozInlineSpellChecker::HandleNavigationEvent(nsIDOMEvent* aEvent,
>                                              PRBool aForceWordSpellCheck,
>                                              PRInt32 aNewPositionOffset)
> {
>-  // get the current selection and compare it to the new selection.
>   nsresult rv;
> 
>+  // If we already handled the navigation event and there is no possibility
>+  // anything has changed since then, we don't have to do anything. This
>+  // optimization makes a noticable different when you hold down a navigation

"difference"?

Code-wise this looks good otherwise, I'm going to take another look at a higher level before marking +.

Comment 21

[Brett Wilson]

Attachment: Final trunk patch

This is the final trunk patch. It turns out SpellCheckSelection is not a public function like I thought, so I just removed it (it's not used internally). This addresses the other comments.

For the record, I used a mutable array because that was copied from the old version. I trivially updated it to use a COM array.

Comment 22

[Brett Wilson]

Fixed on trunk, leaving open for branch. Note that the branch patch requires bug 345112 which introduces async checking in the first place. Leaving open for branch.

Comment 23

[Brett Wilson]

Attachment: Branch patch

Even though this has only baked on trunk for one night, I strongly recommend getting it on branch ASAP so it can get more testing before beta2. This patch also includes the async functionality in bug 345112.

Comment 24

[Mike Schroepfer]

Comment on attachment 232138 [details] [diff] [review]
Branch patch

a=schrep for drivers.  Thanks brett.

Comment 25

[Brett Wilson]

Fixed on branch.

Comment 26

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060805 BonEcho/2.0b1 and the testcase in comment 7.