Closed Bug 526769 Opened 11 years ago Closed 11 years ago
Trunk Crash Report [@ do
_Query Frame::operator<ns IScrollable Frame> ns IScrollable Frame*() ]
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? → 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?
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.
Attachment #414066 - Flags: review?(roc) → review+
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
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.
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.
blocking1.9.1: ? → ---
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
Severity: normal → critical
Summary: Trunk Crash Report [ @do_QueryFrame::operator<nsIScrollableFrame> nsIScrollableFrame*() ] → Trunk Crash Report [@ do_QueryFrame::operator<nsIScrollableFrame> nsIScrollableFrame*() ]
Crash Signature: [@ do_QueryFrame::operator<nsIScrollableFrame> nsIScrollableFrame*() ]
You need to log in before you can comment on or make changes to this bug.