Closed Bug 570350 Opened 10 years ago Closed 10 years ago

Crash [@ nsTextServicesDocument::NodeHasOffsetEntry(nsTArray<OffsetEntry*>*, nsIDOMNode*, int*, int*) ] when enabling/disabling the spell checker


(Core :: DOM: Editor, defect, critical)

Windows XP
Not set



Tracking Status
blocking1.9.2 --- .7+
status1.9.2 --- .7-fixed
blocking1.9.1 --- .11+
status1.9.1 --- .11-fixed


(Reporter: ehsan, Assigned: ehsan)



(Keywords: crash, regression, Whiteboard: [qa-ntd-192] [qa-ntd-191])

Crash Data


(1 file, 1 obsolete file)

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
Could you please list the exact thing that you did which resulted in a crash?  Thanks!
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.
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.
Blocks: 569504
Keywords: regression
Attached patch Patch (v1) (obsolete) — Splinter Review
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!
Attachment #449495 - Flags: review?(roc) 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).
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking1.9.1: ? → .11+
blocking1.9.2: ? → .5+
Attached patch Patch (v1.1)Splinter Review
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.
Attachment #449495 - Attachment is obsolete: true
Attachment #449791 - Flags: review?(roc)
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Attachment #449791 - Flags: approval1.9.2.5?
Attachment #449791 - Flags: approval1.9.1.11?
Comment on attachment 449791 [details] [diff] [review]
Patch (v1.1)

a=LegNeato for Please land this on mozilla-1.9.2 default.
Attachment #449791 - Flags: approval1.9.2.6+
Attachment #449791 - Flags: approval1.9.2.5?
Attachment #449791 - Flags: approval1.9.1.11?
Attachment #449791 - Flags: approval1.9.1.11+
blocking1.9.2: .5+ → .6+
Were STR discovered for this? QA needs something to use to verify for branch releases.
Whiteboard: [qa-needs-STR] [qa-examined-192] [qa-examined-191]
(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.
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?
(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.
I can't reproduce on 3.6.7
Whiteboard: [qa-needs-STR] [qa-examined-192] [qa-examined-191] → [qa-ntd-192] [qa-ntd-191]
Crash Signature: [@ nsTextServicesDocument::NodeHasOffsetEntry(nsTArray<OffsetEntry*>*, nsIDOMNode*, int*, int*) ]
You need to log in before you can comment on or make changes to this bug.