Closed Bug 490425 Opened 14 years ago Closed 14 years ago

Crash [@ nsViewManager::DispatchEvent]

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
1.9.0 Branch
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: stransky, Assigned: stransky)

Details

(Keywords: crash, fixed1.9.0.11, fixed1.9.1, Whiteboard: [sg:critical?] [needs 1.8 landing])

Crash Data

Attachments

(3 files, 3 obsolete files)

crash backtrace
8.63 KB, text/plain
Details
updated one
476 bytes, patch
roc
: review+
beltzner
: approval1.9.1+
dveditz
: approval1.9.0.11+
Details | Diff | Splinter Review
v3
2.37 KB, patch
roc
: review+
Details | Diff | Splinter Review
Attached file crash backtrace — 
Downstream bug - https://bugzilla.redhat.com/show_bug.cgi?id=488570

in nsViewManager::DispatchEvent() there's a code:

1130                  vm->DoSetWindowDimensions(vm->mDelayedResize.width,
1131                                            vm->mDelayedResize.height);
1132                  vm->mDelayedResize.SizeTo(NSCOORD_NONE, NSCOORD_NONE);
1133
1134                  // Paint later.
1135                  vm->UpdateView(vm->mRootView, NS_VMREFRESH_NO_SYNC);

but vm (nsViewManager *) can be destroyed in DoSetWindowDimensions() so 

vm->UpdateView(vm->mRootView, NS_VMREFRESH_NO_SYNC);

uses already freed object. (vm is a normal pointer here). In this particular case ViewManager is destroyed because of frame style change:

#0  DocumentViewerImpl::Hide (this=0xae8eccc0) at nsDocumentViewer.cpp:1997
#1  0x0699561c in nsDocShell::SetVisibility (this=0xaffa6e60, aVisibility=0)
at nsDocShell.cpp:3970
#2  0x012c63a0 in nsSubDocumentFrame::Destroy (this=0xae4723d4) at
nsFrameFrame.cpp:775
#3  0x01291058 in nsBlockFrame::DoRemoveFrame (this=0xafef269c,
aDeletedFrame=0xae4723d4,
    aDestroyFrames=1, aRemoveOnlyFluidContinuations=0) at
nsBlockFrame.cpp:5415
#4  0x012916fe in nsBlockFrame::RemoveFrame (this=0xafef269c, aListName=0x0,
aOldFrame=0xae4723d4)
    at nsBlockFrame.cpp:5013
#5  0x0124ac94 in nsFrameManager::RemoveFrame (this=0xb2e8681c,
aParentFrame=0xafef269c,
    aListName=0x0, aOldFrame=0xae4723d4) at nsFrameManager.cpp:694
#6  0x012066ba in nsCSSFrameConstructor::ContentRemoved (this=0xaea11120,
aContainer=0xae6f7060,
    aChild=0xaeac4460, aIndexInContainer=0, aDidReconstruct=0xbfe46a8c)
    at nsCSSFrameConstructor.cpp:9644
#7  0x01203ee0 in nsCSSFrameConstructor::RecreateFramesForContent
(this=0xaea11120,
    aContent=0xaeac4460) at nsCSSFrameConstructor.cpp:11278
#8  0x012042f8 in nsCSSFrameConstructor::ProcessRestyledFrames
(this=0xaea11120,
    aChangeList=@0xbfe46b70) at nsCSSFrameConstructor.cpp:10018
#9  0x012047d0 in nsCSSFrameConstructor::RestyleElement (this=0xaea11120,
aContent=0xaf9d70e0,
    aPrimaryFrame=0xafad1d98, aMinHint=0) at nsCSSFrameConstructor.cpp:10087
#10 0x012049c5 in nsCSSFrameConstructor::ProcessOneRestyle (this=0xaea11120,
aContent=0xaf9d70e0,
    aRestyleHint=eReStyle_Self, aChangeHint=0) at
nsCSSFrameConstructor.cpp:13422
#11 0x01204bfc in nsCSSFrameConstructor::ProcessPendingRestyles
(this=0xaea11120)
    at nsCSSFrameConstructor.cpp:13518
#12 0x012743c6 in PresShell::DoFlushPendingNotifications (this=0xb2e86800,
aType=Flush_Layout,
    aInterruptibleReflow=1) at nsPresShell.cpp:4540
#13 0x01274609 in PresShell::WillPaint (this=0xb2e86800) at
nsPresShell.cpp:6005
#14 0x01740328 in nsViewManager::FlushPendingInvalidates (this=0xafdc2860)
    at nsViewManager.cpp:2285
#15 0x0174052d in nsViewManager::EnableRefresh (this=0xafdc2860,
aUpdateFlags=0)
    at nsViewManager.cpp:1998
#16 0x0173ca1a in nsViewManager::EndUpdateViewBatch (this=0xafdc2860,
aUpdateFlags=0)
    at nsViewManager.cpp:2041
#17 0x0120d770 in nsIViewManager::UpdateViewBatch::EndUpdateViewBatch
(this=0xbfe47458,
    aUpdateFlags=0) at ../../../dist/include/view/nsIViewManager.h:379
#18 0x012711ce in PresShell::ResizeReflow (this=0xae469400, aWidth=18000,
aHeight=28080)
    at nsPresShell.cpp:2579
#19 0x012608d8 in PresShell::ResizeReflow (this=0xae469400, aView=0xaea544c0,
aWidth=18000,
    aHeight=28080) at nsPresShell.cpp:5977
#20 0x01743b38 in nsViewManager::DoSetWindowDimensions (this=0xaea54580,
aWidth=18000,
    aHeight=28080) at nsViewManager.h:325
#21 0x017410c8 in nsViewManager::DispatchEvent (this=0xaea54580,
aEvent=0xbfe47748,
    aStatus=0xbfe47660) at nsViewManager.cpp:1131
#22 0x01736a33 in HandleEvent (aEvent=0xbfe47748) at nsView.cpp:168
#23 0x06c17fed in nsCommonWidget::DispatchEvent (this=0xaeb0d280,
aEvent=0xbfe47748,
    aStatus=@0xbfe47794) at nsCommonWidget.cpp:158
#24 0x06c0a91a in nsWindow::OnExposeEvent (this=0xaeb0d280,
aWidget=0xb7b5b428, aEvent=0xbfe47e04)
    at nsWindow.cpp:1776

(gdb) f 0
#0  DocumentViewerImpl::Hide (this=0xae8eccc0) at nsDocumentViewer.cpp:1997
1997      GetDocumentSelection(getter_AddRefs(selection));
(gdb) p mViewManager
$8 = {mRawPtr = 0xaea54580}

(gdb) f 21
#21 0x017410c8 in nsViewManager::DispatchEvent (this=0xaea54580,
aEvent=0xbfe47748,
    aStatus=0xbfe47660) at nsViewManager.cpp:1131
(gdb) p vm
$6 = (nsViewManager *) 0xaea54580
Attached patch patch (obsolete) — — Splinter Review 
Hold reference so DoSetWindowDimensions() will not destroy us.
Keywords: crash
Attachment #375186 - Flags: review?(roc)
Comment on attachment 375186 [details] [diff] [review]
patch

Can you please review this patch?
This is a RHT internal topcrash.
Flags: blocking1.9.0.11?
Whiteboard: [sg:critical?]
It would be better to hold the strong reference in HandleEvent --- having 'this' go away anywhere inside a method is bad news...
roc: Is comment 4 an "r-" ? That is, is the ball in your court as reviewer or back to the assignee?
Since we're not seeing this (only 7 crash reports in 3.0.10, out of thousands) and there's no testcase to verify we're uncomfortable rushing this into 1.9.0.11 (code freeze less than a week away). But we'll take it if roc's happy with the patch.
Assignee: nobody → stransky
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.11?
Flags: blocking1.9.0.11-
Comment on attachment 375186 [details] [diff] [review]
patch

The patch is almost right but not quite.
Attachment #375186 - Flags: review?(roc) → review-
Attached patch updated one — — Splinter Review 
Reference is moved to HandleEvent()
Attachment #375186 - Attachment is obsolete: true
Attachment #375580 - Flags: review?(roc)
I'm having this crash:
http://crash-stats.mozilla.com/report/index/6e1ecfb4-d3ef-40d9-82b5-54a612090506?p=1
I guess it will be fixed by this patch?
(In reply to comment #9)
> I'm having this crash:
> http://crash-stats.mozilla.com/report/index/6e1ecfb4-d3ef-40d9-82b5-54a612090506?p=1
> I guess it will be fixed by this patch?

It depends what happened in frame 0 which is missing there. Could be this one or something completely different.
Sorry, never mind, it turned out I 'm seeing a recent regression, I filed bug 491848 for it.
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.11-
Flags: blocking1.9.0.11+
Comment on attachment 375580 [details] [diff] [review]
updated one

Approved for 1.9.0.11, a=dveditz for release-drivers

Do we want this on 1.9.1 and trunk as well?
Attachment #375580 - Flags: approval1.9.1?
Attachment #375580 - Flags: approval1.9.0.11+
Some STR at https://bugzilla.redhat.com/show_bug.cgi?id=488570#c66 but requires a "Clearspace" installation. We need something more reliable for verification, a testcase in the bug or even better one we could check in as a regression test.
Keywords: testcase-wanted
Checked into the 1.9.0 branch. According to the RedHat bug it looks like this crashes 3.5 too so I'll leave the bug open until I get 1.9.1 approval
Keywords: fixed1.9.0.11
Attachment #375580 - Flags: approval1.9.1? → approval1.9.1+
What is Clearspace? Is it something that uses a Java applet. Without a site or without a way to get "Clearspace" there is no way to get a testcase.
Clearspace (http://en.wikipedia.org/wiki/Clearspace) is ugly java blob, the crash occurs when web based document editor is loaded on a web page. (Generally when you select "Edit document" in Clearpace) I'll try to find some reproducer.
Seems to affect 1.8 too, the patch should apply fine.
Flags: wanted1.8.1.x+
So we really have no way to cause this crash on demand, right?
No, but if Martin or caillon can get one of there users to verify it, that'd go a long way.
Checked in
  http://hg.mozilla.org/mozilla-central/rev/c18316c01045
  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bfb9bfaa286d
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
I made an account on wiki.jboss.org, which uses Clearspace, and used the editor with Firefox 3.0.10. I couldn't get a crash when I did so. Does anyone have any more specific steps for triggering this?
Some steps are at:

https://bugzilla.redhat.com/show_bug.cgi?id=488570#c66
https://bugzilla.redhat.com/show_bug.cgi?id=488570#c67

And some people reported the crash occurs only when flash plug-in is installed.
All right. I've been able to reproduce the crash using the steps at https://bugzilla.redhat.com/show_bug.cgi?id=488570#c66. Unfortunately, using last night's 3.0.11pre build, it still crashes using these same steps.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11pre) Gecko/2009051504 GranParadiso/3.0.11pre

So this bug does not appear to be fixed.
Crash reporter data:

Add-ons: {972ce4c6-7e08-4474-a285-3208198ce6fd}:3.0.11pre
BuildID: 2009051504
CrashTime: 1242411881
Email: 
InstallTime: 1242411732
ProductName: Firefox
SecondsSinceLastCrash: 105
StartupTime: 1242411796
Theme: classic/1.0
URL: http://www.jboss.org/community/wiki/Test1234567/edit?containerType=14&container=2005
Vendor: Mozilla
Version: 3.0.11pre
Crash with released 1.9.0.10 for comparison, same profile on same machine: http://crash-stats.mozilla.com/report/index/a0923bd3-b901-4c8a-a02b-7bdb42090515?p=1
Flags: wanted1.9.0.x+
No crash in last night's 1.9.1 build though (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090515 Shiretoko/3.5b5pre).
Huh, finally I manage to get the crash. It's here:

nsViewManager::DispatchEvent() at nsViewManager.cpp:

1078	              // Paint later.
1079	              vm->UpdateView(vm->mRootView, NS_VMREFRESH_NO_SYNC);
1080	              didResize = PR_TRUE;

because vm->mRootView is NULL now so we crash at:

NS_IMETHODIMP nsViewManager::UpdateView(nsIView *aView, PRUint32 aUpdateFlags)
{
  // Mark the entire view as damaged
  nsView* view = static_cast<nsView*>(aView); 

  nsRect bounds = view->GetBounds();
  view->ConvertFromParentCoords(&bounds.x, &bounds.y);
  return UpdateView(view, bounds, aUpdateFlags);
}
Attachment #378026 - Flags: review?(roc)
Comment on attachment 378026 [details] [diff] [review]
null pointer check at nsViewManager::DispatchEvent()

+              if(vm->mRootView) {

Space in "if("
Attachment #378026 - Flags: review?(roc) → review+
Attached patch v3 — — Splinter Review 
Added another NULL check, we can crash in vm->mRootView->GetParent().
Attachment #378045 - Attachment is obsolete: true
Comment on attachment 378810 [details] [diff] [review]
v3

Roc, can you look at this one please? There's another check in for cycle.
Attachment #378810 - Flags: review?(roc)
Flags: blocking1.8.1.next?
Whiteboard: [sg:critical?] → [sg:critical?] need r=roc for 1.8 patch
Comment on attachment 378810 [details] [diff] [review]
v3

To be clear here, we need this patch in all branches. 

Recently, vm->mRootView can be NULL and whole browser crashes then.
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Comment on attachment 378810 [details] [diff] [review]
v3

Cool! Can we get this one checked in?
Keywords: checkin-needed
Whiteboard: [sg:critical?] need r=roc for 1.8 patch → [sg:critical?] [needs 1.8 landing]
Crash Signature: [@ nsViewManager::DispatchEvent]
(In reply to Martin Stránský from comment #37)
> Comment on attachment 378810 [details] [diff] [review]
> v3
> 
> Cool! Can we get this one checked in?

Ok, what/where do we still need a checkin here, this bug is resolved, the only active branches are our rapid-releases and 1.9.2... Was hoping to take care of this checkin-needed but not sure what needs doing.
(In reply to Justin Wood (:Callek) from comment #38)
> (In reply to Martin Stránský from comment #37)
> > Comment on attachment 378810 [details] [diff] [review]
> > v3
> > 
> > Cool! Can we get this one checked in?
> 
> Ok, what/where do we still need a checkin here, this bug is resolved, the
> only active branches are our rapid-releases and 1.9.2... Was hoping to take
> care of this checkin-needed but not sure what needs doing.

No reply, removing checkin-needed
Keywords: checkin-needed
Group: core-security
You need to log in before you can comment on or make changes to this bug.