671319 - Crash [@ nsFrame::ExpandSelectionByMouseMove] part 2

Status: RESOLVED WORKSFORME

(Core :: DOM: Selection, defect)

Description

[Martijn Wargers (dead)]

Attachment: zipped up testcase (uses enhanced privileges)

I mentioned in bug 670058, comment 16, that I was still seeing this particular crash with a certain testcase.
I managed to minized that testcase considerably, see zipped attachment.
Open the filed named "parentframe - Copy (2).htm".
Normally, it crashes within 10 seconds or so.

Prior to the crash I'm seeing these assertions in a debug build:
###!!! ASSERTION: aFrameSelection must be handling current drag for selection: '
draggingFrameSelection && draggingFrameSelection == GetConstFrameSelection()', f
ile c:/mozilla-build/firefox_trunk_debug/layout/generic/nsFrame.cpp, line 2576
WARNING: NS_ENSURE_TRUE(contentRoot) failed: file c:/mozilla-build/firefox_trunk
_debug/layout/generic/nsSelection.cpp, line 923
###!!! ASSERTION: aFrame must be in dragging nsFrameSelection: '!draggingFrameSe
lection || draggingFrameSelection == aFrame->GetConstFrameSelection()', file c:/
mozilla-build/firefox_trunk_debug/layout/generic/nsFrame.cpp, line 2118


https://crash-stats.mozilla.com/report/index/bp-397f7a53-c124-4490-a693-7b21a2110713
0 	xul.dll 	nsFrame::ExpandSelectionByMouseMove 	layout/generic/nsFrame.cpp:2622
1 	xul.dll 	nsFrame::HandleDrag 	
2 	xul.dll 	nsFrame::HandleEvent 	layout/generic/nsFrame.cpp:1836
3 	xul.dll 	nsPresShellEventCB::HandleEvent 	layout/base/nsPresShell.cpp:1503
4 	xul.dll 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:389
5 	xul.dll 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:672
6 	xul.dll 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:7093
7 	xul.dll 	PresShell::HandlePositionedEvent 	layout/base/nsPresShell.cpp:6923
8 	xul.dll 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6758
9 	xul.dll 	PresShell::HandleEvent 	
10 	xul.dll 	nsViewManager::HandleEvent 	view/src/nsViewManager.cpp:1053
11 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:1031
12 	xul.dll 	AttachedHandleEvent 	view/src/nsView.cpp:192
13 	xul.dll 	nsWindow::DispatchEvent 	widget/src/windows/nsWindow.cpp:3549
14 	xul.dll 	nsDOMWindowUtils::SendMouseEventCommon 	dom/base/nsDOMWindowUtils.cpp:470
15 	xul.dll 	nsDOMWindowUtils::SendMouseEvent 	dom/base/nsDOMWindowUtils.cpp:371
16 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
17 	xul.dll 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1592
18 		@0x8874b78 
etc..

Comment 1

[Thomas Ahlblom]

Attachment 545682 [details] crashes Linux as well:
bp-eb4d9ce1-4ff1-4bd2-bc4f-f32622110713

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

> WARNING: NS_ENSURE_TRUE(contentRoot) failed: file c:/mozilla-build/firefox_trunk
_debug/layout/generic/nsSelection.cpp, line 923

Hmm, this looks bad. I guess that when a document is destroying, it doesn't cancel to dragging.

Comment 3

[Thomas Ahlblom]

Regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58101c64c8&tochange=b2622d5c857a

Comment 4

[Martijn Wargers (dead)]

Thanks, then this is a regression from bug 552707, also.

Comment 5

[bogas04]

https://crash-stats.mozilla.com/report/index/5e008c48-da4b-4f06-9f40-d2b132110713

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Hmm, the testcase tests very strange case. Looks like it doesn't dispatch mouseup events but dispatches mousedown and mousemove event on another frameselection content during another dragging transaction. My patch didn't assume such case, so, it could be the cause.

Can you reproduce this bug on actual web pages?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

> but dispatches mousedown and mousemove event on another frameselection content during another dragging transaction.

Um, this may be wrong since nsIDOMWindowUtils::SendMouseEvent() dispatches the events via widget. So, the contents to receive the events should be corrected by mouse capture...

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Okay, I see the cause. If mousedown event is dispatched (not matter the button), constructor of nsAutoHandlingUserInputStatePusher releases the capturing forcibly. By aborting dragging transaction in nsIPresShell::SetCapturingContent(), I succeeded to fix this bug.

I'm trying to write more possible testcase.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Attachment: Patch v1.0

When nsIPresShell::SetCapture() is called, we should abort the dragging transaction.

Even if capturing is canceled by some reasons, current trunk build doesn't cancel the dragging transaction at same time. Therefore, there can be a strange situation that nobody is capturing mouse events but drag for selection isn't canceled.

In most cases, any elements doesn't recapture mouse events. However, if someone which has independent selection and is in another document captures mouse events again, we see this crash.

Comment 10

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

Comment on attachment 546751 [details] [diff] [review]
Patch v1.0

Have you went through all the cases when SetCapturingContent is called to
make sure this is ok?

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

I think so, the method is used for two ways. One is releasing capture. At this time, we cannot keep the dragging transaction. Therefore, we must abort it. The other is capturing content for something. It's typically called by mousedown event handling. So, dragging shouldn't be doing at that time. Even if such case actually happened, it means the user uses two or more mice at same time. In such case, we don't need to continue the transaction for preventing unexpected behavior.

And also even if web pages called setCapture(), it's refused because setCapture() does nothing if another element has been already capturing.

Comment 12

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

Comment on attachment 546751 [details] [diff] [review]
Patch v1.0


> nsIPresShell::SetCapturingContent(nsIContent* aContent, PRUint8 aFlags)
> {
>+  // If SetCapturingContent() is called during dragging mouse for selection,
>+  // we should abort current transaction.
>+  nsFrameSelection* fs = nsFrameSelection::GetMouseDownFrameSelection();
nsRefPtr, since AbortDragForSelection may end up deleting fs via some selection
listener.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

http://hg.mozilla.org/mozilla-central/rev/be5295604fd0

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Temporarily, backed out for risk management of mozilla8, see bug 675865.