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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Editor
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({crash, regression})

Trunk
mozilla1.9.3a5
x86
Windows XP
crash, regression
Points:
---

Firefox Tracking Flags

(blocking1.9.2 .7+, status1.9.2 .7-fixed, blocking1.9.1 .11+, status1.9.1 .11-fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

See bug 570321 comment 4.

bp-afc49e3f-9a06-465b-944e-e10192100605

stack:

0|0|xul.dll|nsTextServicesDocument::NodeHasOffsetEntry(nsTArray<OffsetEntry *> *,nsIDOMNode *,int *,int *)|hg:hg.mozilla.org/mozilla-central:editor/txtsvc/src/nsTextServicesDocument.cpp:b219912edfec|3919|0x2 0|1|xul.dll|nsTextServicesDocument::DeleteNode(nsIDOMNode *)|hg:hg.mozilla.org/mozilla-central:editor/txtsvc/src/nsTextServicesDocument.cpp:b219912edfec|1839|0x16 0|2|xul.dll|nsTSDNotifier::DidDeleteNode(nsIDOMNode *,unsigned int)|hg:hg.mozilla.org/mozilla-central:editor/txtsvc/src/nsTSDNotifier.cpp:b219912edfec|118|0x9 0|3|xul.dll|nsEditor::DeleteNode(nsIDOMNode *)|hg:hg.mozilla.org/mozilla-central:editor/libeditor/base/nsEditor.cpp:b219912edfec|1459|0x12 0|4|xul.dll|nsTextEditRules::DidDeleteSelection(nsISelection *,short,unsigned int)|||0x43 0|5|xul.dll|nsTextEditRules::DidDoAction(nsISelection *,nsRulesInfo *,unsigned int)|hg:hg.mozilla.org/mozilla-central:editor/libeditor/text/nsTextEditRules.cpp:b219912edfec|371|0x6 0|6|xul.dll|nsPlaintextEditor::DeleteSelection(short)|hg:hg.mozilla.org/mozilla-central:editor/libeditor/text/nsPlaintextEditor.cpp:b219912edfec|738|0x15 0|7|xul.dll|nsEditorEventListener::KeyPress(nsIDOMEvent *)|hg:hg.mozilla.org/mozilla-central:editor/libeditor/base/nsEditorEventListener.cpp:b219912edfec|407|0xa 0|8|xul.dll|nsEventListenerManager::HandleEventInternal(nsPresContext *,nsEvent *,nsIDOMEvent * *,nsPIDOMEventTarget *,unsigned int,nsEventStatus *,nsCxPusher *)|hg:hg.mozilla.org/mozilla-central:content/events/src/nsEventListenerManager.cpp:b219912edfec|1177|0x3c 0|9|xul.dll|nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor &,unsigned int,nsDispatchingCallback *,int,nsCxPusher *)|hg:hg.mozilla.org/mozilla-central:content/events/src/nsEventDispatcher.cpp:b219912edfec|341|0xe5 0|10|xul.dll|nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor &,unsigned int,nsDispatchingCallback *,int,nsCxPusher *)|hg:hg.mozilla.org/mozilla-central:content/events/src/nsEventDispatcher.cpp:b219912edfec|395|0x2c 0|11|xul.dll|nsEventDispatcher::Dispatch(nsISupports *,nsPresContext *,nsEvent *,nsIDOMEvent *,nsEventStatus *,nsDispatchingCallback *,nsCOMArray<nsPIDOMEventTarget> *)|hg:hg.mozilla.org/mozilla-central:content/events/src/nsEventDispatcher.cpp:b219912edfec|628|0x3e 0|12|xul.dll|PresShell::HandleEventInternal(nsEvent *,nsIView *,nsEventStatus *)|hg:hg.mozilla.org/mozilla-central:layout/base/nsPresShell.cpp:b219912edfec|6622|0x13 0|13|xul.dll|PresShell::HandleEvent(nsIView *,nsGUIEvent *,nsEventStatus *)|||0x78e 0|14|xul.dll|nsViewManager::HandleEvent(nsView *,nsPoint,nsGUIEvent *)|hg:hg.mozilla.org/mozilla-central:view/src/nsViewManager.cpp:b219912edfec|1073|0xd 0|15|xul.dll|nsViewManager::DispatchEvent(nsGUIEvent *,nsIView *,nsEventStatus *)|hg:hg.mozilla.org/mozilla-central:view/src/nsViewManager.cpp:b219912edfec|1052|0x11 0|16|xul.dll|HandleEvent|hg:hg.mozilla.org/mozilla-central:view/src/nsView.cpp:b219912edfec|160|0x12 0|17|xul.dll|nsWindow::DispatchEvent(nsGUIEvent *,nsEventStatus &)|hg:hg.mozilla.org/mozilla-central:widget/src/windows/nsWindow.cpp:b219912edfec|3226|0x2 0|18|xul.dll|nsWindow::DispatchWindowEvent(nsGUIEvent *)|hg:hg.mozilla.org/mozilla-central:widget/src/windows/nsWindow.cpp:b219912edfec|3249|0xf 0|19|xul.dll|nsWindow::DispatchKeyEvent(unsigned int,unsigned short,nsTArray<nsAlternativeCharCode> const *,unsigned int,tagMSG const *,nsModifierKeyState const &,unsigned int)|hg:hg.mozilla.org/mozilla-central:widget/src/windows/nsWindow.cpp:b219912edfec|3309|0xe 0|20|xul.dll|nsWindow::OnKeyDown(tagMSG const &,nsModifierKeyState &,int *,nsFakeCharMessage *)|hg:hg.mozilla.org/mozilla-central:widget/src/windows/nsWindow.cpp:b219912edfec|5978|0x1a 0|21|xul.dll|nsWindow::ProcessKeyDownMessage(tagMSG const &,int *)|hg:hg.mozilla.org/mozilla-central:widget/src/windows/nsWindow.cpp:b219912edfec|5074|0xe 0|22|xul.dll||||0x41990b 0|23|xul.dll|nsWindow::WindowProc(HWND__ *,unsigned int,unsigned int,long)|hg:hg.mozilla.org/mozilla-central:widget/src/windows/nsWindow.cpp:b219912edfec|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.

by http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.7a5pre&query_search=signature&query_type=startswith&query=nsTextServicesDocument%3A%3ANodeHasOffsetEntry&date=06%2F05%2F2010%2017%3A53%3A05&range_value=1&range_unit=weeks&hang_type=any&process_type=any&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=nsTextServicesDocument%3A%3ANodeHasOffsetEntry%28nsTArray%3COffsetEntry*%3E*%2C%20nsIDOMNode*%2C%20int*%2C%20int*%29
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
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!
Attachment #449495 - Flags: review?(roc)
1.9.2.5 and 1.9.1.11 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: --- → ?
Attachment #449495 - Flags: review?(roc) → review+
blocking1.9.1: ? → .11+
blocking1.9.2: ? → .5+
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.
Attachment #449495 - Attachment is obsolete: true
Attachment #449791 - Flags: review?(roc)
Attachment #449791 - Flags: review?(roc) → review+
http://hg.mozilla.org/mozilla-central/rev/8a37644190b7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Attachment #449791 - Flags: approval1.9.2.5?
Attachment #449791 - Flags: approval1.9.1.11?

Comment 8

7 years ago
Comment on attachment 449791 [details] [diff] [review]
Patch (v1.1)

a=LegNeato for 1.9.2.6. 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+
status1.9.1: --- → wanted
status1.9.2: --- → wanted
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/acd85daa3bb3
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3fb73b2265ba
status1.9.1: wanted → .11-fixed
status1.9.2: wanted → .6-fixed
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 1.9.1.9. I'm going to try with 1.9.1.10. 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 1.9.1.9. I'm going to try
> with 1.9.1.10. 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.