526769 - Trunk Crash Report [@ do_QueryFrame::operator<nsIScrollableFrame> nsIScrollableFrame*() ]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[chris hofmann]

new crash on trunk and firefox 3.6 due to frame poisoning

spin off from bug 526587

reports at

http://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&query=do_QueryFrame%3A%3Aoperator&date=&range_value=1&range_unit=weeks&do_query=1&signature=do_QueryFrame%3A%3Aoperator%3CnsIScrollableFrame%3E%20nsIScrollableFrame*%28%29

example stack

0  	xul.dll  	do_QueryFrame::operator<nsIScrollableFrame> nsIScrollableFrame*  	 layout/generic/nsQueryFrame.h:276
1 		@0x37 	
2 	xul.dll 	nsFrame::HandleEvent 	layout/generic/nsFrame.cpp:1628
3 	xul.dll 	nsPresShellEventCB::HandleEvent 	layout/base/nsPresShell.cpp:1321
4 	xul.dll 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:362
5 	xul.dll 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:584
6 	xul.dll 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:6582
7 	xul.dll 	PresShell::HandlePositionedEvent 	layout/base/nsPresShell.cpp:6428
8 	xul.dll 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6292
9 	xul.dll 	nsViewManager::HandleEvent 	view/src/nsViewManager.cpp:1197
10 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:1176
11 	xul.dll 	HandleEvent 	view/src/nsView.cpp:167
12 	xul.dll 	nsWindow::DispatchEvent 	widget/src/windows/nsWindow.cpp:2937
13 	xul.dll 	nsWindow::DispatchWindowEvent 	widget/src/windows/nsWindow.cpp:2960
14 	xul.dll 	nsWindow::DispatchMouseEvent 	widget/src/windows/nsWindow.cpp:3335
15 	xul.dll 	ChildWindow::DispatchMouseEvent 	widget/src/windows/nsWindow.cpp:6959

Comment 2

[Mats Palmgren (inactive)]

I'm investigating this one...

Comment 3

[chris hofmann]

this remains at low volume (1-8) crashes per day even after b1, b2, and b3.

if it's hard to reproduce and/or not real easy to fix quickly (and with low risk) then it it can probably stay off the fx3.6 blocklist.

Comment 4

[Mats Palmgren (inactive)]

This is what I think happens.  We get a MOUSE_MOVE:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1622
then in nsFrame::HandleDrag we call in to nsFrameSelection code
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2221
which notifies selection listeners synchronously, which can destroy
the frame.  We crash on line 2232 with a destroyed 'this'.

In that case it should be easy and risk-free to fix using a nsWeakFrame(this).
Patch in a bit...

Comment 5

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

You may be able to write a test case that does that...

Comment 6

[Mats Palmgren (inactive)]

Yeah, I think so too, but I haven't been able to make it crash so far...

Fwiw, I found a comment on nsFrameSelection::HandleClick() being destructive:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2015
and nsFrameSelection::HandleDrag() calls HandleClick() at the end:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#1771
I tracked down the comment to bug 368760.  I don't see HandleDrag in
bug 368760 comment 1, so HandleDrag() -> HandleClick() is probably a
more recent change.

I don't know much about "static analysis", but can it help us with stuff
like this?

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Simple and risk-free fix.
(BTW, both HandleTableSelection and HandleDrag are marked /*unsafe*/ in
nsFrameSelection.h)

Comment 8

[Mats Palmgren (inactive)]

Do we have a special sg: flag for frame poisoning crashes like this bug --
which is sg:dos with fp and sg:critical without, right?

Comment 9

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

We don't.

(In reply to comment #6)
> I don't know much about "static analysis", but can it help us with stuff
> like this?

Yes, I've been asking Taras to implement it for a while now.

Comment 10

[Daniel Veditz [:dveditz]]

Until we have no supported branches without frame poisoning I'd lean toward "[sg:critical mitigated by frame poisoning]" but I wouldn't be averse to "[sg:dos (sg:critical w/out frame poisoning)]". The latter should not over-count trunk criticals because we should be querying for "[sg:critical" with the leading square bracket, but will make branch criticals stand out for triage.

Comment 11

[Daniel Veditz [:dveditz]]

We could also make a distinction based on whether we've seen it crash on the older branches or not. If we have a testcase that causes an unsafe crash on the old branches then "critical mitigated by fp", and for the ones we're only seeing because of frame poisoning "dos (possibly critical w/out fp)".

A lot of these we've filed recently due to crash-stats reports might all come down to a small handful of actual flaws, just showing up at different places based on context and timing. They might also be regressions that wouldn't ever happen on the old branches. Hard to say without a testcase or by looking at the eventual fix.

The patch here looks like it would apply to the 1.9.0 and 1.9.1 branches, although the code outside the patch context has changed a bit. It does look like the old code checked whether the frame was still alive, but only as a debugging warning. Those checks were removed with bug 503943, http://hg.mozilla.org/mozilla-central/rev/eda2433181c9#l11.97

Comment 12

[Mats Palmgren (inactive)]

The only user comment in crash reports, so far:
"I was highlighting something on lernu.net"

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 414066 [details] [diff] [review]
Patch rev. 1

Need approval1.9.2 to land on trunk.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 414066 [details] [diff] [review]
Patch rev. 1

a1.9.2=dbaron

Comment 15

[chris hofmann]

this is #9 fp topcrash in the list from today
https://bug526587.bugzilla.mozilla.org/attachment.cgi?id=414317&t=ytKa49fedH

so landing it for the planned beta to confirm the fix in 3.6b4 would be really good.

Comment 16

[Mats Palmgren (inactive)]

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

Comment 17

[Mats Palmgren (inactive)]

This patch isn't needed for 1.9.2, 1.9.1 or 1.9.0 branches.
The |weakFrame| check in nsFrame::HandleDrag() looks the same on all three:
http://mxr.mozilla.org/mozilla1.9.2/source/layout/generic/nsFrame.cpp#2206
and should be safe. (So the fix in this bug is for the trunk regression
from bug 503943)

Querying crash stats (past 4 weeks) I found one non-trunk crash, in 3.6b1:
http://crash-stats.mozilla.com/report/index/f8b3ef8c-a802-4915-a9b3-5f7eb2091101
and the stack leading up to [@do_QueryFrame::operator<nsIScrollableFrame> nsIScrollableFrame*() ] here is completely different.
I filed bug 531069 about this crash.