278677 - crash If I try to use editor observer (nsIEditorObserver)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a6) Gecko/20050111
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a6) Gecko/20050111

I get nsIEditor interface for editor and register editor observer. When I edit
document then mozilla is crashed.


Reproducible: Always

Steps to Reproduce:

var editor=document.getElementById('editor');
editor.makeEditable(editor.editortype, false);
var editorobserver={
	EditAction: function(){}
};
editor.getEditor(editor.contentWindow).addEditorObserver(editorobserver);

Comment 1

[alexander :surkov (:asurkov)]

Attachment: test case

run with "chrome" protocol

Comment 2

[Martijn Wargers (dead)]

Attachment: Testcase2

Same testcase as reporter, but with enablePrivilege. Just press two times
"Allow" and then follow the steps from the reporter.

Two Talkback ID's:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB3113732W


nsWyciwygChannel::IsPending 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/html/document/src/nsWyciwygChannel.cpp,
line 97]
nsAutoPlaceHolderBatch::~nsAutoPlaceHolderBatch 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/editor/libeditor/base\nsEditorUtils.h,
line 66]
nsHTMLEditor::HandleKeyPress 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/editor/libeditor/html/nsHTMLEditor.cpp,
line 1363]
nsTextEditorKeyListener::KeyPress 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/editor/libeditor/text/nsEditorEventListeners.cpp,
line 250]
DispatchToInterface 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp,
line 136]
nsEventListenerManager::HandleEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1604]
nsDocument::HandleDOMEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/base/src/nsDocument.cpp,
line 3830]
nsGenericElement::HandleDOMEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericElement.cpp,
line 2028]
PresShell::HandleEventInternal 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp,
line 5942]
PresShell::HandleEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp,
line 5773]
nsViewManager::HandleEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2360]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2133]
HandleEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp,
line 174]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1103]
nsWindow::DispatchKeyEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 3043]
nsWindow::OnChar 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 3249]
nsWindow::ProcessMessage 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 3933]
nsWindow::WindowProc 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1389]
USER32.dll + 0x1ef0 (0x77e11ef0)
USER32.dll + 0x204c (0x77e1204c)
USER32.dll + 0x21af (0x77e121af)
nsAppStartup::Run 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,
line 147]
main 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/browser/app/nsBrowserApp.cpp,
line 60]
KERNEL32.DLL + 0x2893d (0x7c59893d)

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB3113720G


0x00000000
nsEditor::NotifyEditorObservers 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/editor/libeditor/base/nsEditor.cpp,
line 1643]
nsAutoPlaceHolderBatch::~nsAutoPlaceHolderBatch 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/editor/libeditor/base\nsEditorUtils.h,
line 66]
nsHTMLEditor::HandleKeyPress 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/editor/libeditor/html/nsHTMLEditor.cpp,
line 1363]
nsTextEditorKeyListener::KeyPress 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/editor/libeditor/text/nsEditorEventListeners.cpp,
line 250]
DispatchToInterface 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp,
line 136]
nsEventListenerManager::HandleEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1604]
nsDocument::HandleDOMEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/base/src/nsDocument.cpp,
line 3830]
nsGenericElement::HandleDOMEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericElement.cpp,
line 2028]
PresShell::HandleEventInternal 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp,
line 5942]
PresShell::HandleEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp,
line 5773]
nsViewManager::HandleEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2360]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2133]
HandleEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp,
line 174]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1103]
nsWindow::DispatchKeyEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 3043]
nsWindow::OnChar 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 3249]
nsWindow::ProcessMessage 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 3933]
nsWindow::WindowProc 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1389]
USER32.dll + 0x1ef0 (0x77e11ef0)
USER32.dll + 0x204c (0x77e1204c)
USER32.dll + 0x21af (0x77e121af)
nsAppStartup::Run 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,
line 147]
main 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/browser/app/nsBrowserApp.cpp,
line 60]
KERNEL32.DLL + 0x2893d (0x7c59893d)

Comment 3

[Martijn Wargers (dead)]

Attachment: Backtrace rom debug build of 2004-12-08

Comment 4

[Martijn Wargers (dead)]

No obvious duplicates -> confirming

Comment 5

[Wladimir Palant]

The reason for the problem is that as far as I can see nsEditor doesn't addref
the observers as well as action listeners. Document state listeners use an
nsISupportsArray and are addrefed. This difference is not documented and I can't
see the reasoning behind it. I think that all arrays should be changed to use
the nsCOMArray type.

Blame shows to jfrancis but I don't think he is still around. CCing glazman
instead to comment on this issue.

Comment 6

[lekma]

confirming with xulrunner 1.8 and xulrunner 1.9a1

Comment 7

[timeless]

unless i'm missing something, this list doesn't reference count at all. which seems like a very good way to die.

Comment 8

[timeless]

Attachment: compiling patch (seems to regress some stuff)

anyone is free to take this bug from me. i'm one offing patches w/ no particular interest in owning bugs.

Comment 9

[timeless]

Comment on attachment 206806 [details] [diff] [review]
compiling patch (seems to regress some stuff)

i believe that i've broken the transaction notification stuff.

Comment 10

[Ronald Tilby]

The testcases WFM using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060602 Minefield/3.0a1 ID:2006060204 [cairo]
Should this bug be closed?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> The testcases WFM using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
> rv:1.9a1) Gecko/20060602 Minefield/3.0a1 ID:2006060204 [cairo]
> Should this bug be closed?
> 

'testcase2' still leads to crash on Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060602 SeaMonkey/1.5a

Comment 12

[Martijn Wargers (dead)]

*** Bug 345335 has been marked as a duplicate of this bug. ***

Comment 13

[Andreas Wuest]

The strange thing is that this only happens from XPConnect, since observer->EditAction() is used by various internal classes (e.g. in textboxes, etc., which all use the nsEditor component), and they are experiencing no problems.

Furthermore, the related code (addEditorObserver, removeEditorObserver, NotifyEditorObservers, ...) hasn't changed for ages (OTOH, since Mozilla itself does not use it from XPConnect anywhere in its code base, it might also be the case that this never worked correctly, so technically isn't a regression). I _think_ me might get an invalid pointer from XPConnect already at addEditorObserver() time, but I can't prove it.

As a workaround, you can use a nsIEditActionListener and place the code you had wanted to execute upon a nsIEditorObserver event inside all of the Did*() methods, but this is ugly.

Comment 14

[timeless]

someone should probably try to fix this or figure out why my fix doesn't work or something :(

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

addEditorObserver doesn't addref the pointer it gets, which probably explains the crashes from JS. The C++ code probably keeps them alive by some other mean, while the JS ones presumably get garbage collected at some point.

So I don't think this is a strange thing :)

Comment 16

[[e-mail]]

Attachment: alternate compiling patch (also untested)

Comment 17

[Andrew Schultz]

Attachment: valgrind log with attachment 234233 applied

valgrind sees a boatload of errors before crashing, but this is the first

Comment 18

[Andrew Schultz]

Attachment: just the first bit

(oops) the previous attachment was the whole log

Comment 19

[[e-mail]]

Comment on attachment 234233 [details] [diff] [review]
alternate compiling patch (also untested)

Thanks for debugging this for me :-)

> NS_IMETHODIMP
> nsEditor::RemoveEditActionListener(nsIEditActionListener *aListener)
> {
>-  if (!aListener || !mActionListeners)
>+  if (!aListener)
>     return NS_ERROR_FAILURE;
> 
>-  if (!mActionListeners->RemoveElement((void *)aListener))
>+  if (!mActionListeners.RemoveObject(aListener))
>     return NS_ERROR_FAILURE;
> 
>   NS_IF_RELEASE(aListener);
Oops, double-release :-[

Comment 20

[[e-mail]]

Attachment: Updated for bitrot

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 241545 [details] [diff] [review]
Updated for bitrot

I could r+ this too, if you want.

Comment 22

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 241545 [details] [diff] [review]
Updated for bitrot

r=me

Comment 23

[[e-mail]]

Fix checked in.

Comment 24

[Boris Zbarsky [:bzbarsky]]

How about adding comments to the API (nsIEditor) documenting the ownership model too?

I assume you looked at existing editor observer consumers to make sure they don't leak, right?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Oh, and this patch needs to rev the IID of nsIEditor, since you're making a pretty fundamental change in how it behaves.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Filed bug 356466 on comment 24 and 25.

Comment 27

[Mike Cowperthwaite]

This bug's patch has been suggested as the cause of (crash) bug 357861.