Last Comment Bug 802995 - crash in nsTextServicesDocument::DeleteSelection while using spell checker
: crash in nsTextServicesDocument::DeleteSelection while using spell checker
Status: VERIFIED FIXED
[tbird topcrash][TB16 regression][gs]
: crash, regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 16 Branch
: x86 Windows NT
: -- critical (vote)
: mozilla19
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks: 763283
  Show dependency treegraph
 
Reported: 2012-10-18 00:51 PDT by Ludovic Hirlimann [:Usul]
Modified: 2013-01-28 05:40 PST (History)
7 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
Add null check (1.46 KB, patch)
2012-10-25 05:38 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Ludovic Hirlimann [:Usul] 2012-10-18 00:51:33 PDT
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, NI for questions) 2012-10-18 02:22:09 PDT
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) 2012-10-18 02:43:06 PDT
I haven't heard of any crash related to spell check at this time...
Comment 3 Alex Keybl [:akeybl] 2012-10-19 15:19:53 PDT
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 :Ehsan Akhgari (away Aug 1-5) 2012-10-19 15:47:51 PDT
Sure, I can help.  Aryeh, can you please take a look and see if you find out anything?

Thanks!
Comment 5 :Aryeh Gregor (away until August 15) 2012-10-25 05:35:59 PDT
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 (away until August 15) 2012-10-25 05:38:51 PDT
Created attachment 675087 [details] [diff] [review]
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 :Ehsan Akhgari (away Aug 1-5) 2012-10-25 08:17:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a9ae8ce3b8d
Comment 8 :Ehsan Akhgari (away Aug 1-5) 2012-10-25 08:18:22 PDT
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] 2012-10-25 18:30:27 PDT
https://hg.mozilla.org/mozilla-central/rev/9a9ae8ce3b8d

Should this have a test?
Comment 10 :Aryeh Gregor (away until August 15) 2012-10-26 05:26:03 PDT
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 2012-10-26 11:12:12 PDT
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 13 Wayne Mery (:wsmwk, NI for questions) 2012-11-18 07:02:07 PST
will need to check crash-stats after TB17 release
Comment 14 Wayne Mery (:wsmwk, NI for questions) 2013-01-28 05:40:22 PST
Thanks for the fix.

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

Note You need to log in before you can comment on or make changes to this bug.