802995 - crash in nsTextServicesDocument::DeleteSelection while using spell checker

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Ludovic Hirlimann [:Usul]]

This bug was filed from the Socorro interface and is 
report bp-bba5f0ee-9249-4daa-b23f-58e7d2121016 .
============================================================= 
0 	xul.dll 	nsTextServicesDocument::DeleteSelection 	editor/txtsvc/src/nsTextServicesDocument.cpp:1387
1 	xul.dll 	nsTextServicesDocument::InsertText 	editor/txtsvc/src/nsTextServicesDocument.cpp:1737
2 	xul.dll 	mozSpellChecker::Replace 	extensions/spellcheck/src/mozSpellChecker.cpp:222
3 	xul.dll 	nsEditorSpellCheck::ReplaceWord 	editor/composer/src/nsEditorSpellCheck.cpp:394
4 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
5 	xul.dll 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2418
6 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1474
7 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:344
8 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2442
9 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:293
10 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:355
11 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:387
12 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5604
13 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:1892
14 	kernel32.dll 	SystemTimeToFileTime 	
15 		@0xe1ea05f

Comments found in Crash-stats :
This has now happened 4 times, every time I start the spell checker. I have not tried rebooting yet.

I was spell checking a reply to an e-mail; then it just disapeared

Comment 1

[Wayne Mery (:wsmwk)]

another regression

vincent, roland, does this correlate to gsfn issue reported by vincent in tb-support-crew?  can you hook up the topic URL/tag to this bug

also exists in TB17 bp-8165cfcf-acf3-4731-b889-6610e2121017

Comment 2

[Vincent (caméléon)]

I haven't heard of any crash related to spell check at this time...

Comment 3

[Alex Keybl [:akeybl]]

Ehsan - can you help the TB guys out and take a look a this? I know you've done Editor work in the past. Thanks :)

Comment 4

[(no longer active)]

Sure, I can help.  Aryeh, can you please take a look and see if you find out anything?

Thanks!

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The line of the crash was last changed by bug 763283 part 2, which uses ->AsContent() without a null check.  Without digging to see how exactly this could happen, that seems like the obvious reason for the crash.  The regression should be in 16 in that case, which seems to be what happened.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Add null check

The original code before bug 763283 was

      if (mIteratorStatus != nsTextServicesDocument::eIsDone)
      {
        // The old iterator is still pointing to something valid,
        // so get its current node so we can restore it after we
        // create the new iterator!

        curContent = do_QueryInterface(mIterator->GetCurrentNode());
      }

so I changed it to better match that.  No test, because I have no way to know how to reproduce.  Try: https://tbpl.mozilla.org/?tree=Try&rev=6c0b12df1d34

Comment 7

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a9ae8ce3b8d

Comment 8

[(no longer active)]

Comment on attachment 675087 [details] [diff] [review]
Add null check

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 763283
User impact if declined: Thunderbird topcrash
Testing completed (on m-c, etc.): We can't reproduce the crash, but the fix is straightforward
Risk to taking this patch (and alternatives if risky): Minimal
String or UUID changes made by this patch: none.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a9ae8ce3b8d

Should this have a test?

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Yes, if anyone can create one.  We only know about the crash from crash reports, and don't have steps to reproduce, so we don't know how to write a test.  It would probably take an excessive amount of work to test this.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 675087 [details] [diff] [review]
Add null check

Risk is low enough, let's see if this takes down the volume on branches, approving.

Comment 12

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/005b6a9a807e
https://hg.mozilla.org/releases/mozilla-beta/rev/a2cc1ba86817

Comment 13

[Wayne Mery (:wsmwk)]

will need to check crash-stats after TB17 release

Comment 14

[Wayne Mery (:wsmwk)]

Thanks for the fix.

v.fixed - no crash sigs in TB17.
But there some other sigs - bug 834487, and bug  835314