371137 - nsEditorSpellCheck::GetNextMisspelledWord() triggers assertions and sometimes crashes when used on certain pages

Status: RESOLVED WORKSFORME

(Core :: Spelling checker, defect)

Description

[Sean Murphy]

Attachment: FindNext_Assert.html

Using nsEditorSpellCheck::GetNextMisspelledWord() on the focused editor of certain web pages triggers an assertion and in some cases even crashes the entire application, leaving misspelled word iteration via the Core spell checking modules completely broken at the moment.

The best way to demonstrate this bug is by using the test cases I have attached.  The interesting aspect of this bug is that it seems to depend on how the HTML has been laid out in the source file (regarding the use of whitespace and blank lines, for example).  All three test cases are, HTML code and presentation wise, EXACTLY the same web page.  The minor differences between them apply only to the HTML source code's formatting and use of whitespace/line-breaks.

I have a feeling that in some situations the differences in code structure negatively affect the nsITextServicesDocument which is responsible for flattening down the page into pure text and then allowing the spell checker to progress over certain blocks and perform checks on them.

Each differently-formatted case results in varying degrees of correct/incorrect/crashing behavior from nsEditorSpellCheck::GetNextMisspelledWord()/mozSpellChecker::NextMisspelledWord().

STR:

1. Focus the <input> text field on each page and place the caret somewhere before the incorrectly spelled word.
2. Obtain the editor's spell checker and call GetNextMisspelledWord() on it.
3. While bug 370186 remains open, you'll need to perform GetNextMisspelledWord() a second time to actually kick start its behavior.

Results exhibited by each test case upon calling GetNextMisspelledWord():

1. FindNext_Assert.html:

###!!! ASSERTION: Failed to position iterator!: 'NS_SUCCEEDED(rv)', file mozilla/content/base/src/nsContentIterator.cpp, line 980

This condition is asserted when PositionAt(mFirst) is called from within nsContentIterator::First().  The partial stack trace:

>#0  nsContentIterator::First (this=0x21f894a0) at mozilla/content/base/src/nsContentIterator.cpp:980
>#1  0x199e7d94 in nsFilteredContentIterator::First (this=0x21f89440) at mozilla/editor/txtsvc/src/nsFilteredContentIterator.cpp:156
>#2  0x199e98dd in nsTextServicesDocument::FirstTextNode (aIterator=0x21f89440, aIteratorStatus=0x21f891e4) at mozilla/editor/txtsvc/src/nsTextServicesDocument.cpp:4116
>#3  0x199eb4a4 in nsTextServicesDocument::FirstBlock (this=0x21f891d0) at mozilla/editor/txtsvc/src/nsTextServicesDocument.cpp:601
>#4  0x1feed47c in mozSpellChecker::SetupDoc (this=0x21f89860, outBlockOffset=0xbffff0ec) at mozilla/extensions/spellcheck/src/mozSpellChecker.cpp:395
>#5  0x1feed524 in mozSpellChecker::NextMisspelledWord (this=0x21f89860, aWord=@0xbffff1a4, aSuggestions=0x21f839d0) at mozilla/extensions/spellcheck/src/mozSpellChecker.cpp:94
>#6  0x18c31c31 in nsEditorSpellCheck::GetNextMisspelledWord (this=0x21f839c0, aNextMisspelledWord=0xbffff278) at mozilla/editor/composer/src/nsEditorSpellCheck.cpp:257
>#7  0x00021646 in -[BrowserWindowController checkSpelling:] (self=0x21a3ec30, _cmd=0x90a9b378, sender=0x2a54690) at CaminoTrunk/camino/src/browser/BrowserWindowController.mm:4496

2. FindNext_Crash.html:

This one attempts to dereference a NULL pointer and results in crash at:

>#0  0x27fdfb04 in nsCOMPtr<nsINodeInfo>::operator-> (this=0x4) at ../../../dist/include/xpcom/nsCOMPtr.h:847
>#1  0x27ff13f0 in nsIContent::Tag (this=0x0) at ../../../dist/include/content/nsIContent.h:201
>#2  0x27f77248 in nsTextServicesDocument::IsBlockNode (aContent=0x0) at mozilla/editor/txtsvc/src/nsTextServicesDocument.cpp:3091
>#3  0x27f7bea8 in nsTextServicesDocument::CreateOffsetTable (aOffsetTable=0x1f4f1a64, aIterator=0x1f4f1b60, aIteratorStatus=0x1f4f1a54, aIterRange=0x0, aStr=0x0) at mozilla/editor/txtsvc/src/nsTextServicesDocument.cpp:4467
>#4  0x27f71e3c in nsTextServicesDocument::LastSelectedBlock (this=0x1f4f1a40, aSelStatus=0xbfffddb4, aSelOffset=0xbfffddb8, aSelLength=0xbfffddbc) at mozilla/editor/txtsvc/src/nsTextServicesDocument.cpp:1278
>#5  0x18bbf72c in mozSpellChecker::SetupDoc (this=0x1f4f1cc0, outBlockOffset=0xbfffde10) at mozilla/extensions/spellcheck/src/mozSpellChecker.cpp:367
>#6  0x18bbdf9c in mozSpellChecker::NextMisspelledWord (this=0x1f4f1cc0, aWord=@0xbfffdf10, aSuggestions=0x1f4f13c0) at mozilla/extensions/spellcheck/src/mozSpellChecker.cpp:94
>#7  0x188e76b8 in nsEditorSpellCheck::GetNextMisspelledWord (this=0x1f4f13b0, aNextMisspelledWord=0xbfffe0e0) at mozilla/editor/composer/src/nsEditorSpellCheck.cpp:256
>#8  0x00028258 in -[BrowserWindowController checkSpelling:] (self=0x2ce2120, _cmd=0x90a88cac, sender=0x1ced8df0) at CaminoTrunk/camino/src/browser/BrowserWindowController.mm:4410

3. FindNext_Works.html:

While all three have been identically displaying pages, this is the only case which results in correct behavior.

4. For one additional test case, consider http://google.com:

Performing the STR on Google.com indicate even more incorrect/inconsistent behavior.  Calling GetNextMisspelledWord() on the editor does not proceed to spell check the text in that editor, but rather obtains text found earlier ("Personalized Home | Sign in") and spell checks that.  This happens because mozSpellChecker::SetupDoc() encounters difficulty when calling mTsDoc->LastSelectedBlock().  The nsITextServicesDocument object returns eBlockNotFound and leaves mozSpellChecker with no choice but to just blindly start checking from the beginning of the entire DOM document.

The reason mTsDoc->LastSelectedBlock() fails with eBlockNotFound again stems from the content iterator failing to position itself:

http://mxr.mozilla.org/mozilla/source/content/base/src/nsContentIterator.cpp#1101

nsresult 
nsContentIterator::PositionAt(nsIContent* aCurNode)

Fails here:

if (!firstNode || !lastNode ||
    !ContentIsInTraversalRange(mCurNode, mPre, mFirst, firstOffset,
                               mLast, lastOffset))
{
  mIsDone = PR_TRUE;
  return NS_ERROR_FAILURE;
}

Specifically, the ContentInTraversalRange() helper function returns "no."

Useful bits from the stack trace:

>#0  nsContentIterator::PositionAt (this=0x1f4aafc0, aCurNode=0x1ce9c660) at mozilla/content/base/src/nsContentIterator.cpp:1049
>#1  0x27f6d40c in nsFilteredContentIterator::PositionAt (this=0x1f4aacf0, aCurNode=0x1ce9c660) at mozilla/editor/txtsvc/src/nsFilteredContentIterator.cpp:450
>#2  0x27f71d0c in nsTextServicesDocument::LastSelectedBlock (this=0x1f4aaca0, aSelStatus=0xbfffddb4, aSelOffset=0xbfffddb8, aSelLength=0xbfffddbc) at mozilla/editor/txtsvc/src/nsTextServicesDocument.cpp:1260
>#3  0x18bbf72c in mozSpellChecker::SetupDoc (this=0x1f4aabb0, outBlockOffset=0xbfffde10) at mozilla/extensions/spellcheck/src/mozSpellChecker.cpp:367
>#4  0x18bbdf9c in mozSpellChecker::NextMisspelledWord (this=0x1f4aabb0, aWord=@0xbfffdf10, aSuggestions=0x1f4aa770) at mozilla/extensions/spellcheck/src/mozSpellChecker.cpp:94
>#5  0x188e76b8 in nsEditorSpellCheck::GetNextMisspelledWord (this=0x1f4aa760, aNextMisspelledWord=0xbfffe0e0) at mozilla/editor/composer/src/nsEditorSpellCheck.cpp:256
>#6  0x00028258 in -[BrowserWindowController checkSpelling:] (self=0x2ce2120, _cmd=0x90a88cac, sender=0x1ced8df0) at CaminoTrunk/camino/src/browser/BrowserWindowController.mm:4410

Finally, the behavior which I have detailed in this report seems fairly widespread and concerns more than just Google and the test cases.  I visited some popular sites to examine behavior and several of them, including Digg.com and MySpace.com, run into the same trouble and trigger identical assertions when spell checking an editor.

Comment 1

[Sean Murphy]

Attachment: Test Case: FindNext_Crash.html

Comment 2

[Sean Murphy]

Attachment: Test Case: FindNext_Works.html

Comment 3

[Sean Murphy]

After examining this some more, I believe the root cause of the problematic behavior can be traced back to mozSpellChecker::SetupDoc(), specifically when it calls nsTextServicesDocument::LastSelectedBlock().

To demonstrate, use http://google.com/ as the example again: 
Enter some text into the search field, move the caret to the beginning of the editor (finding does not wrap) and then call nsEditorSpellCheck::GetNextMisspelledWord() twice (the first call is irrelevant; see bug 370186).

GetNextMisspelledWord() hands off its work to mozSpellChecker::NextMisspelledWord().  NextMisspelledWord() then calls its own SetupDoc() method to prepare the nsITextServicesDocument it will use to provide text and allow for word iteration.  It is here inside SetupDoc() where the first wrong step seems to be taken and from which all further faulty behavior originates.  The failing line inside SetupDoc() is when it calls nsTextServicesDocument::LastSelectedBlock() on line 365:
<http://mxr.mozilla.org/mozilla/source/extensions/spellcheck/src/mozSpellChecker.cpp#365>

Stepping into nsTextServicesDocument::LastSelectedBlock(), it fails on line 1260:
> result = mIterator->PositionAt(content); // This fails; NS_FAILED(result) is true.
<http://mxr.mozilla.org/mozilla/source/editor/txtsvc/src/nsTextServicesDocument.cpp#1260>

On web pages which permit the proper functionality of GetNextMisspelledWord(), mozSpellChecker's mTsDoc->LastSelectedBlock() works correctly and sets the text services document's current block as it should.  

On pages that break GetNextMisspelledWord(), mTsDoc->LastSelectedBlock() fails and mozSpellChecker is then forced to fall back and use the first block in the document (mTsDoc->FirstBlock()).  The first assertions are then actually triggered during FirstBlock() and more follow later on when mozSpellChecker attempts to progress and reference more text blocks with nsTextServicesDocument::NextBlock().  It seems as though the entire problem stems from the prior failure of the text services document during LastSelectedBlock().

So, determining why nsTextServicesDocument::LastSelected() fails on some pages could be the answer to solving this bug.

Comment 4

[Jesse Ruderman]

Sean, I'm confused about the steps to reproduce.  How can I "Obtain the editor's spell checker and call GetNextMisspelledWord() on it"?

Comment 5

[Jesse Ruderman]

Hmm, no response from Sean :(

Masayuki / Alexander, can you make sense of this bug report?

Comment 6

[alexander :surkov (:asurkov)]

Unfortunately I'm not spellchecker developer. Probably it's possible to call it in DOM Inspector like input.editor.getInlineSpellChecker(false).spellChecker. GetNextMisspelledWord() but I met security restrictions.

Comment 7

[Sean Murphy]

Jesse,

Sorry I didn't reply sooner.  Gosh, it has been a while since I last dealt with these objects, but I'll try to elaborate about how the issues are reproduced.

Basically, the bug exists in the implementation of nsIEditorSpellCheck.  To get at this interface, you'll need to first grab a reference to the currently focused text field and obtain its nsIInlineSpellChecker.  Through this interface you can then call nsIInlineSpellChecker::GetSpellChecker() to fetch the nsIEditorSpellCheck object we're really after.

I was accessing the objects through C++, so I'm not sure I can help much using the JS route...

Comment 8

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

Unfortunately, I'm not a spellchecker developer too... And I'm busy in other works, I cannot check this bug now, sorry.

Comment 9

[Wayne Mery (:wsmwk)]

no signature from past 3 months contain  nsEditorSpellCheck::GetNextMisspelledWord

there are these however
* nsCOMPtr_base::~nsCOMPtr_base() | nsEditorSpellCheck::~nsEditorSpellCheck()
  eg. bp-6e44a275-78eb-4b2a-8f38-5aa962091207  
* nsRefPtr<nsIDOMEventListener>::~nsRefPtr<nsIDOMEventListener>() | nsEditorSpellCheck::InitSpellChecker(nsIEditor*, int)
  eg. bp-686f07da-3988-406f-bbde-842112091124

Comment 10

[Wayne Mery (:wsmwk)]

Only two crashes in past 3 months. one is bp-bb2e7974-a2cc-49e6-bc22-64b2f2161018
So I think this can be closed.

Comment 11

[Wayne Mery (:wsmwk)]

I forgot there is a testcase here

Comment 12

[Andrei Purice]

Marking this as Resolved > Worksforme since there are no more crashes with this signature in the past 6 months.