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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[(no longer active)]

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

Comment 1

[(no longer active)]

Could you please list the exact thing that you did which resulted in a crash?  Thanks!

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

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.

Comment 3

[(no longer active)]

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

[(no longer active)]

Attachment: 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

[(no longer active)]

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).

Comment 6

[(no longer active)]

Attachment: 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 7

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/8a37644190b7

Comment 8

[LegNeato]

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.

Comment 9

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/acd85daa3bb3
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3fb73b2265ba

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Were STR discovered for this? QA needs something to use to verify for branch releases.

Comment 11

[(no longer active)]

(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

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 13

[(no longer active)]

(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.

Comment 14

[Gabriela [:gaby2300]]

I can't reproduce on 3.6.7