338129 - Crash [@ nsQueryInterface::operator()] with designMode and window gets removed on mousedown/click of resizer

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

Filing this under core->editor, but it might also be Event handling, don't know.
Filing it as security sensitive just to be sure (although it seems to be a null dereference crash?).

Talkback ID: TB18747259H
0x00000000
nsQueryInterface::operator()   nsCOMPtr<nsIHTMLEditor>::nsCOMPtr<nsIHTMLEditor>   nsEventListenerManager::HandleEvent   nsEventTargetChainItem::HandleEvent   nsEventTargetChainItem::HandleEventTargetChain   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventDispatcher::Dispatch   PresShell::HandleEventInternal   PresShell::HandlePositionedEvent   PresShell::HandleEvent   nsViewManager::HandleEvent   nsViewManager::DispatchEvent   HandleEvent   nsWindow::DispatchEvent   nsWindow::DispatchMouseEvent  

Also crashes in Mozilla1.7.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Bernd]

this triggers the assert ###!!! ASSERTION: Unexpected ContentRemoved: '!mIsDocumentGone', file d:/moz_src/mozilla/layout/base/nsPresShell.cpp, line 5258

http://lxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#5263

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

And the crash happens in editor since there are few event listeners which have
a pointer to the editor and that pointer is not set to null when editor is deleted.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Use weak reference to nsEditor

nsEditor is deleted before some event listeners. So better to use weak reference and check whether the editor is still alive before trying
to use it.
The change to DeleteRefToAnonymousNode is needed to remove the 
assertions Bernd mentioned.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Or should I ask someone else to review, Daniel?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Btw, this is not a null dereference crash, but reference to a dead pointer.
Should this block 1.8.0.5 ?

Comment 7

[chris hofmann]

any other editor reviewers?

Comment 8

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

Comment on attachment 222858 [details] [diff] [review]
Use weak reference to nsEditor

yes, everything seems to be ok but unfortunately I have no time to test this patch.
I suggest you ask Neil for sr.
r=[e-mail]

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 222858 [details] [diff] [review]
Use weak reference to nsEditor

btw, I tried to keep the changes as simple as possible so that the functionality shouldn't change at all. That way this could be taken also to branches - I hope.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: just make sure that event listeners are removed on time

Sorry Daniel, but Neil complained something about nsEditor::RemoveEventListeners
and I started to re-debug this and noticed a simpler way to fix the crash.
Patch size 1/4 of the previous one :)
So the problem is just that mMouseListenerP is used for many things and it must
be removed from all event targets it is listening.

Comment 11

[Daniel Veditz [:dveditz]]

Please land on trunk and 1.8 branch before requesting 1.8.0.5 approval on the patch. Thanks! 

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

Got some comments from Daniel on IRC.
Does this look safer and better now?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 225402 [details] [diff] [review]
v3

Neil, is this still ok to you?

Comment 14

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

Comment on attachment 224888 [details] [diff] [review]
just make sure that event listeners are removed on time

r=[e-mail]

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 224888 [details] [diff] [review]
just make sure that event listeners are removed on time

so this should be ok, after all

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 224888 [details] [diff] [review]
just make sure that event listeners are removed on time

Actually, I think Daniel should approve this, as it's the module owner that does that.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Checked in to trunk

Comment 18

[Daniel Veditz [:dveditz]]

The existence of the apparently blessed "v3" patch different from the one glazman actually reviewed later is confusing us. Waiting for an explicit approval for 1.8.1 from glazman on which one we want for the 1.8 and 1.8.0 branches

Comment 19

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

Comment on attachment 224888 [details] [diff] [review]
just make sure that event listeners are removed on time

yeah, go on.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 224888 [details] [diff] [review]
just make sure that event listeners are removed on time

approved for 1.8.0 branch, a=dveditz for drivers

Comment 21

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US;
rv:1.8.0.5) Gecko/20060626 Firefox/1.5.0.5, no crash with testcase.

BUT the iframe disappears on click... is that acceptable behavior?  From the bug summary, it looks like that was half of this bug? Crash is gone, but iframe still goes away. :-(

Comment 22

[Martijn Wargers (dead)]

No, the iframe should go away with the testcase, so that's not a bug.

Comment 23

[Alexander Sack]

Attachment: 1st aviary backport with regressions.

This patch makes the crash go away. On the first click, it shows the same behaviour as described in bug: frame + resizer go away.

However, when reloading, the resizer is never drawn again. Instead there is some activity under the hood (looks like HideResizer and ShowResizer are called in an infinite loop).

I see the following assertions on console:

###!!! ASSERTION: Please remove this from the document properly: '!mDocument', file nsGenericElement.cpp, line 839
Break: at file nsGenericElement.cpp, line 839

Any ideas?

Comment 24

[Bob Clary [:bc] (inactive)]

https://bugzilla.mozilla.org/attachment.cgi?id=222170
ff2b2 windows, linux no crash; macppc no resizer to click
ff2b2 debug windows, linux no crash