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

RESOLVED FIXED

Status

()

Core
Editor
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: smaug)

Tracking

(4 keywords)

Trunk
x86
Windows XP
crash, testcase, verified1.8.0.5, verified1.8.1
Points:
---
Bug Flags:
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] dead pointer, sometimes null, crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 222170 [details]
testcase

Comment 2

11 years ago
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
(Assignee)

Comment 3

11 years ago
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.
(Assignee)

Comment 4

11 years ago
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.
Assignee: mozeditor → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #222858 - Flags: review?(daniel)
(Assignee)

Comment 5

11 years ago
Or should I ask someone else to review, Daniel?
(Assignee)

Comment 6

11 years ago
Btw, this is not a null dereference crash, but reference to a dead pointer.
Should this block 1.8.0.5 ?
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?

Comment 7

11 years ago
any other editor reviewers?
Flags: blocking1.9a1? → blocking1.9a1+
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
Attachment #222858 - Flags: review?(daniel) → review+
(Assignee)

Comment 9

11 years ago
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.
Attachment #222858 - Flags: superreview?(neil)
(Assignee)

Comment 10

11 years ago
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.
Attachment #222858 - Attachment is obsolete: true
Attachment #224888 - Flags: superreview?(neil)
Attachment #224888 - Flags: review?(daniel)
Attachment #222858 - Flags: superreview?(neil)

Updated

11 years ago
Attachment #224888 - Flags: superreview?(neil) → superreview+
Please land on trunk and 1.8 branch before requesting 1.8.0.5 approval on the patch. Thanks! 
Flags: blocking1.8.0.5? → blocking1.8.0.5+
(Assignee)

Comment 12

11 years ago
Created attachment 225402 [details] [diff] [review]
v3

Got some comments from Daniel on IRC.
Does this look safer and better now?
Attachment #224888 - Attachment is obsolete: true
Attachment #225402 - Flags: review?(daniel)
Attachment #224888 - Flags: review?(daniel)
(Assignee)

Comment 13

11 years ago
Comment on attachment 225402 [details] [diff] [review]
v3

Neil, is this still ok to you?
Attachment #225402 - Flags: superreview?(neil)
Comment on attachment 224888 [details] [diff] [review]
just make sure that event listeners are removed on time

r=daniel@glazman.org
Attachment #224888 - Flags: review+
(Assignee)

Comment 15

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

so this should be ok, after all
Attachment #224888 - Attachment is obsolete: false
(Assignee)

Updated

11 years ago
Attachment #225402 - Flags: superreview?(neil)
Attachment #225402 - Flags: review?(daniel)
(Assignee)

Updated

11 years ago
Attachment #224888 - Flags: approval-branch-1.8.1?(bugmail)
(Assignee)

Updated

11 years ago
Attachment #224888 - Flags: approval1.8.0.5?
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.
Attachment #224888 - Flags: approval-branch-1.8.1?(bugmail) → approval-branch-1.8.1?(daniel)
Whiteboard: needs trunk landing, need 1.8.1 approval (glazman)
(Assignee)

Comment 17

11 years ago
Checked in to trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: needs trunk landing, need 1.8.1 approval (glazman) → needs 1.8.1 approval (glazman)
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
(Assignee)

Updated

11 years ago
Attachment #225402 - Attachment is obsolete: true
Comment on attachment 224888 [details] [diff] [review]
just make sure that event listeners are removed on time

yeah, go on.
Attachment #224888 - Flags: approval-branch-1.8.1?(daniel) → approval-branch-1.8.1+
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1
Whiteboard: needs 1.8.1 approval (glazman)
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
Attachment #224888 - Flags: approval1.8.0.5? → approval1.8.0.5+
(Assignee)

Updated

11 years ago
Flags: blocking1.8.1?
Keywords: fixed1.8.0.5

Comment 21

11 years ago
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. :-(
Keywords: fixed1.8.0.5 → verified1.8.0.5
(Reporter)

Comment 22

11 years ago
No, the iframe should go away with the testcase, so that's not a bug.
Flags: blocking1.8.1+
Whiteboard: [sg:critical] dead pointer, sometimes null

Comment 23

11 years ago
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

11 years ago
https://bugzilla.mozilla.org/attachment.cgi?id=222170
ff2b2 windows, linux no crash; macppc no resizer to click
ff2b2 debug windows, linux no crash
Keywords: fixed1.8.1 → verified1.8.1
Group: security
Flags: in-testsuite?
Crash Signature: [@ nsQueryInterface::operator()]
You need to log in before you can comment on or make changes to this bug.