Last Comment Bug 570350 - Crash [@ nsTextServicesDocument::NodeHasOffsetEntry(nsTArray<OffsetEntry*>*, nsIDOMNode*, int*, int*) ] when enabling/disabling the spell checker
: Crash [@ nsTextServicesDocument::NodeHasOffsetEntry(nsTArray<OffsetEntry*>*,...
[qa-ntd-192] [qa-ntd-191]
: crash, regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Windows XP
-- critical (vote)
: mozilla1.9.3a5
Assigned To: :Ehsan Akhgari
: Makoto Kato [:m_kato]
Depends on:
Blocks: 569504
  Show dependency treegraph
Reported: 2010-06-05 13:33 PDT by :Ehsan Akhgari
Modified: 2011-06-13 10:01 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (v1) (24.24 KB, patch)
2010-06-05 20:19 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review
Patch (v1.1) (24.21 KB, patch)
2010-06-07 21:25 PDT, :Ehsan Akhgari
roc: review+
christian: approval1.9.2.7+
christian: approval1.9.1.11+
Details | Diff | Splinter Review

Description User image :Ehsan Akhgari 2010-06-05 13:33:07 PDT
See bug 570321 comment 4.



0|0|xul.dll|nsTextServicesDocument::NodeHasOffsetEntry(nsTArray<OffsetEntry *> *,nsIDOMNode *,int *,int *)||3919|0x2 0|1|xul.dll|nsTextServicesDocument::DeleteNode(nsIDOMNode *)||1839|0x16 0|2|xul.dll|nsTSDNotifier::DidDeleteNode(nsIDOMNode *,unsigned int)||118|0x9 0|3|xul.dll|nsEditor::DeleteNode(nsIDOMNode *)||1459|0x12 0|4|xul.dll|nsTextEditRules::DidDeleteSelection(nsISelection *,short,unsigned int)|||0x43 0|5|xul.dll|nsTextEditRules::DidDoAction(nsISelection *,nsRulesInfo *,unsigned int)||371|0x6 0|6|xul.dll|nsPlaintextEditor::DeleteSelection(short)||738|0x15 0|7|xul.dll|nsEditorEventListener::KeyPress(nsIDOMEvent *)||407|0xa 0|8|xul.dll|nsEventListenerManager::HandleEventInternal(nsPresContext *,nsEvent *,nsIDOMEvent * *,nsPIDOMEventTarget *,unsigned int,nsEventStatus *,nsCxPusher *)||1177|0x3c 0|9|xul.dll|nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor &,unsigned int,nsDispatchingCallback *,int,nsCxPusher *)||341|0xe5 0|10|xul.dll|nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor &,unsigned int,nsDispatchingCallback *,int,nsCxPusher *)||395|0x2c 0|11|xul.dll|nsEventDispatcher::Dispatch(nsISupports *,nsPresContext *,nsEvent *,nsIDOMEvent *,nsEventStatus *,nsDispatchingCallback *,nsCOMArray<nsPIDOMEventTarget> *)||628|0x3e 0|12|xul.dll|PresShell::HandleEventInternal(nsEvent *,nsIView *,nsEventStatus *)||6622|0x13 0|13|xul.dll|PresShell::HandleEvent(nsIView *,nsGUIEvent *,nsEventStatus *)|||0x78e 0|14|xul.dll|nsViewManager::HandleEvent(nsView *,nsPoint,nsGUIEvent *)||1073|0xd 0|15|xul.dll|nsViewManager::DispatchEvent(nsGUIEvent *,nsIView *,nsEventStatus *)||1052|0x11 0|16|xul.dll|HandleEvent||160|0x12 0|17|xul.dll|nsWindow::DispatchEvent(nsGUIEvent *,nsEventStatus &)||3226|0x2 0|18|xul.dll|nsWindow::DispatchWindowEvent(nsGUIEvent *)||3249|0xf 0|19|xul.dll|nsWindow::DispatchKeyEvent(unsigned int,unsigned short,nsTArray<nsAlternativeCharCode> const *,unsigned int,tagMSG const *,nsModifierKeyState const &,unsigned int)||3309|0xe 0|20|xul.dll|nsWindow::OnKeyDown(tagMSG const &,nsModifierKeyState &,int *,nsFakeCharMessage *)||5978|0x1a 0|21|xul.dll|nsWindow::ProcessKeyDownMessage(tagMSG const &,int *)||5074|0xe 0|22|xul.dll||||0x41990b 0|23|xul.dll|nsWindow::WindowProc(HWND__ *,unsigned int,unsigned int,long)||3961|0x2a 0|24|user32.dll|InternalCallWinProc|||0x27 1|0|ntdll.dll|KiFastSystemCallRet|||0x0 1|1|ntdll.dll|ZwWaitForSingleObject|||0xb 1|2|kernel32.dll|WaitForSingleObjectEx|||0x8a 1|3|kernel32.dll|WaitForSingleObject|||0x11 1|4|DrvTrNTm.dll||||0x4eb1 1|5|kernel32.dll|BaseThreadStart|||0x36
Comment 1 User image :Ehsan Akhgari 2010-06-05 17:09:28 PDT
Could you please list the exact thing that you did which resulted in a crash?  Thanks!
Comment 2 User image (mostly gone) XtC4UaLL [:xtc4uall] 2010-06-05 18:01:08 PDT
i haven't found reliable STRs as of yet, but it's not about enabling/disabling the spellchecker, but rather deleting by spellchecker marked chars/words with the backspace key. maybe DEL key works too.

so it would be:
* enter some garbage what would be marked by spellchecker in a form (or use the wrong dictionary language as in my case)
* enable spellchecker
* put the cursor at the end
* hit backspace/keep the key pushed

repeat untill crash happens.

the crashing started with build 20100604040314.
Comment 3 User image :Ehsan Akhgari 2010-06-05 19:29:18 PDT
The problem is that nsTSDNotifier holds a strong reference to nsTextServicesDocument (as a C++ pointer!), and does not participate in cycle collection.  Moreover, it always creates a cycle (the TSD object holding a pointer to its notifier, and the notifier holding a pointer back to it), so the cycle collector deletes the TSD object too soon.
Comment 4 User image :Ehsan Akhgari 2010-06-05 20:19:04 PDT
Created attachment 449495 [details] [diff] [review]
Patch (v1)

The only purpose behind the nsTSDNotifier class was to implement IEditActionListener.  That made no sense!  So I went ahead and removed that class, and implemented its functionality inside nsTextServicesDocument itself.  This will automatically solve the cycle collection problem, by removing a node on the cycle graph!
Comment 5 User image :Ehsan Akhgari 2010-06-05 20:20:37 PDT and should both have this patch, to make sure that we don't introduce this crash to our users in those releases (which already have the patch for bug 569504).
Comment 6 User image :Ehsan Akhgari 2010-06-07 21:25:17 PDT
Created attachment 449791 [details] [diff] [review]
Patch (v1.1)

As most things with the editor, this was not as easy as it appeared on the surface.

I discovered a leak, which I tracked down to be a confusion in the cycle collector.  What was happening was that nsTextServicesDocument::Release was called again as the object was deleted, using a stack like this:

1827 ^G###!!! ASSERTION: Serial number requested for unrecognized pointer!  Are you memmoving a refcounted object?: 'serialno != 0', file /Users/ehsanakhgari/moz/src/xpcom/base/nsTraceRefcntImpl.cpp, line 1052
1828 S_LogRelease_P (nsTraceRefcntImpl.cpp:1053, in libxpcom_core.dylib)
1829 nsTextServicesDocument::Release() (in libgklayout.dylib) + 233
1830 sCOMArray_base::RemoveObject(nsISupports*) (nsCOMArray.cpp:129, in libxpcom_core.dylib)
1831 nsCOMArray<nsIEditActionListener>::RemoveObject(nsIEditActionListener*) (in libgklayout.dylib) + 24
1832 nsEditor::RemoveEditActionListener(nsIEditActionListener*) (in libgklayout.dylib) + 73
1833 nsTextServicesDocument::~nsTextServicesDocument() (in libgklayout.dylib) + 184
1834 nsTextServicesDocument::Release() (in libgklayout.dylib) + 284
1835 ReleaseObjects(void*, void*) (in libxpcom_core.dylib) (nsCOMArray.cpp:151)
1836 nsVoidArray::EnumerateForwards(int (*)(void*, void*), void*) (in libxpcom_core.dylib) (nsVoidArray.cpp:724)
1837 sCOMArray_base::Clear() (nsCOMArray.cpp:159, in libxpcom_core.dylib)
1838 nsCOMArray<nsIEditActionListener>::Clear() (in libgklayout.dylib) + 17
1839 nsEditor::PreDestroy(int) (in libgklayout.dylib) + 306

As can be seen, what was happening here was that removing the listener caused the refcount to drop to zero and the object to be deleted, but nsTextServicesDocument's destructor would try to remove the listener from the editor once again, which would "succeed" because as far as the editor was concerned, the reference was not yet removed from the document.

The reason behind this all was the embarrassing mistake of trying to unregister the listener inside nsTextServicesDocument's destructor.  This is like asking someone to unregister us from something long after all references to us (including the "something" above) have been released!  This patch removes that code from the destructor.
Comment 8 User image christian 2010-06-11 15:22:49 PDT
Comment on attachment 449791 [details] [diff] [review]
Patch (v1.1)

a=LegNeato for Please land this on mozilla-1.9.2 default.
Comment 10 User image Al Billings [:abillings] 2010-06-21 17:53:57 PDT
Were STR discovered for this? QA needs something to use to verify for branch releases.
Comment 11 User image :Ehsan Akhgari 2010-06-21 20:45:28 PDT
(In reply to comment #10)
> Were STR discovered for this? QA needs something to use to verify for branch
> releases.

Yes, see comment 2 please.  I also found disabling and re-enabling the Check Spelling item from the text box's context menu in the middle of deletions helped in reproducing the crash.
Comment 12 User image Al Billings [:abillings] 2010-06-22 16:52:32 PDT
When was this bug introduced? I can't reproduce with I'm going to try with Are we sure that it shows itself outside of 1.9.3?
Comment 13 User image :Ehsan Akhgari 2010-06-22 21:31:43 PDT
(In reply to comment #12)
> When was this bug introduced? I can't reproduce with I'm going to try
> with Are we sure that it shows itself outside of 1.9.3?

This was a regression from bug 569504 on trunk.  In order to reproduce the crash, you need a build including the patch for that bug but not this one.  I personally only reproduced the crash on trunk, but the cause of the crash is the same across all our branches.
Comment 14 User image Gabriela [:gaby2300] 2010-07-09 20:17:56 PDT
I can't reproduce on 3.6.7

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