Closed Bug 526769 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Layout, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: chofmann, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [sg:dos (critical w/out frame-poisoning)] trunk regression)

Crash Data

Attachments

(1 file)

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
Flags: blocking1.9.2?
Keywords: testcase-wanted
Whiteboard: [sg:investigate]
Flags: blocking1.9.2? → blocking1.9.2-
I'm investigating this one...
Assignee: nobody → matspal
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.
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...
You may be able to write a test case that does that...
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?
Attached patch Patch rev. 1Splinter Review
Simple and risk-free fix.
(BTW, both HandleTableSelection and HandleDrag are marked /*unsafe*/ in
nsFrameSelection.h)
Attachment #414066 - Flags: review?(roc)
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?
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.
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.
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
blocking1.9.1: --- → ?
status1.9.1: --- → ?
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.17?
Whiteboard: [sg:investigate] → [sg:dos (critical w/out frame-poisoning)]
The only user comment in crash reports, so far:
"I was highlighting something on lernu.net"
Comment on attachment 414066 [details] [diff] [review]
Patch rev. 1

Need approval1.9.2 to land on trunk.
Attachment #414066 - Flags: approval1.9.2?
Comment on attachment 414066 [details] [diff] [review]
Patch rev. 1

a1.9.2=dbaron
Attachment #414066 - Flags: approval1.9.2? → approval1.9.2+
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.
http://hg.mozilla.org/mozilla-central/rev/d10f2f103f43
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Blocks: 503943
blocking1.9.1: ? → ---
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.17?
Summary: Firefox 3.6b1 Crash Report [ @do_QueryFrame::operator<nsIScrollableFrame> nsIScrollableFrame*() ] → Trunk Crash Report [ @do_QueryFrame::operator<nsIScrollableFrame> nsIScrollableFrame*() ]
Whiteboard: [sg:dos (critical w/out frame-poisoning)] → [sg:dos (critical w/out frame-poisoning)] trunk regression
Flags: wanted1.9.0.x-
Severity: normal → critical
Summary: Trunk Crash Report [ @do_QueryFrame::operator<nsIScrollableFrame> nsIScrollableFrame*() ] → Trunk Crash Report [@ do_QueryFrame::operator<nsIScrollableFrame> nsIScrollableFrame*() ]
Group: core-security
Crash Signature: [@ do_QueryFrame::operator<nsIScrollableFrame> nsIScrollableFrame*() ]
You need to log in before you can comment on or make changes to this bug.