Last Comment Bug 338129 - Crash [@ nsQueryInterface::operator()] with designMode and window gets removed on mousedown/click of resizer
: Crash [@ nsQueryInterface::operator()] with designMode and window gets remove...
Status: RESOLVED FIXED
[sg:critical] dead pointer, sometimes...
: crash, testcase, verified1.8.0.5, verified1.8.1
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-16 05:46 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2007-08-21 18:28 PDT (History)
10 users (show)
chofmann: blocking1.9a1+
dbaron: blocking1.8.1+
dveditz: blocking1.8.0.5+
dveditz: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (717 bytes, text/html)
2006-05-16 05:47 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Use weak reference to nsEditor (49.43 KB, patch)
2006-05-22 07:08 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
daniel: review+
Details | Diff | Splinter Review
just make sure that event listeners are removed on time (14.19 KB, patch)
2006-06-08 11:26 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
daniel: review+
neil: superreview+
daniel: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review
v3 (15.47 KB, patch)
2006-06-13 02:05 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
1st aviary backport with regressions. (11.42 KB, patch)
2006-07-28 04:52 PDT, Alexander Sack
no flags Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-16 05:46:41 PDT
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 [:mwargers] (not working for Mozilla) 2006-05-16 05:47:42 PDT
Created attachment 222170 [details]
testcase
Comment 2 Bernd 2006-05-16 11:38:13 PDT
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] (way behind * queues, especially ni? queue) 2006-05-16 23:43:17 PDT
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] (way behind * queues, especially ni? queue) 2006-05-22 07:08:33 PDT
Created attachment 222858 [details] [diff] [review]
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] (way behind * queues, especially ni? queue) 2006-05-22 07:19:41 PDT
Or should I ask someone else to review, Daniel?
Comment 6 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2006-05-23 11:51:35 PDT
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 2006-06-02 09:48:03 PDT
any other editor reviewers?
Comment 8 Daniel Glazman (:glazou) 2006-06-07 08:46:53 PDT
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=daniel@glazman.org
Comment 9 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2006-06-07 08:52:59 PDT
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] (way behind * queues, especially ni? queue) 2006-06-08 11:26:14 PDT
Created attachment 224888 [details] [diff] [review]
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] 2006-06-09 14:44:27 PDT
Please land on trunk and 1.8 branch before requesting 1.8.0.5 approval on the patch. Thanks! 
Comment 12 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2006-06-13 02:05:39 PDT
Created attachment 225402 [details] [diff] [review]
v3

Got some comments from Daniel on IRC.
Does this look safer and better now?
Comment 13 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2006-06-13 11:46:44 PDT
Comment on attachment 225402 [details] [diff] [review]
v3

Neil, is this still ok to you?
Comment 14 Daniel Glazman (:glazou) 2006-06-14 04:44:27 PDT
Comment on attachment 224888 [details] [diff] [review]
just make sure that event listeners are removed on time

r=daniel@glazman.org
Comment 15 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2006-06-14 04:50:47 PDT
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 2006-06-14 09:58:10 PDT
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] (way behind * queues, especially ni? queue) 2006-06-15 00:32:30 PDT
Checked in to trunk
Comment 18 Daniel Veditz [:dveditz] 2006-06-15 14:24:57 PDT
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) 2006-06-16 01:04:42 PDT
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] 2006-06-16 14:14:11 PDT
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] 2006-06-26 16:21:10 PDT
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 [:mwargers] (not working for Mozilla) 2006-06-26 16:23:52 PDT
No, the iframe should go away with the testcase, so that's not a bug.
Comment 23 Alexander Sack 2006-07-28 04:52:32 PDT
Created attachment 231086 [details] [diff] [review]
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:] 2006-08-22 00:27:38 PDT
https://bugzilla.mozilla.org/attachment.cgi?id=222170
ff2b2 windows, linux no crash; macppc no resizer to click
ff2b2 debug windows, linux no crash

Note You need to log in before you can comment on or make changes to this bug.