Crash [@ nsViewManager::DispatchEvent]

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

({crash, fixed1.9.0.11, fixed1.9.1})

1.9.0 Branch
x86
Linux
Points:
---
Bug Flags:
blocking1.9.0.11 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] [needs 1.8 landing], crash signature)

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

10 years ago
Posted 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
(Assignee)

Comment 1

10 years ago
Posted patch patch (obsolete) — Splinter Review
Hold reference so DoSetWindowDimensions() will not destroy us.
(Assignee)

Updated

10 years ago
Keywords: crash
(Assignee)

Updated

10 years ago
Attachment #375186 - Flags: review?(roc)
(Assignee)

Comment 2

10 years ago
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-
(Assignee)

Comment 8

10 years ago
Posted patch updated oneSplinter 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?
(Assignee)

Comment 10

10 years ago
(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.
(Assignee)

Comment 17

10 years ago
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.
(Assignee)

Comment 18

10 years ago
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
Last Resolved: 10 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?
(Assignee)

Comment 24

10 years ago
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
The actual crash report will be at http://crash-stats.mozilla.com/report/index/2478c4d7-7f49-45eb-b9b7-18bb82090515?p=1 when it is no longer pending.
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).
(Assignee)

Comment 30

10 years ago
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);
}
(Assignee)

Updated

10 years ago
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+
(Assignee)

Comment 34

10 years ago
Posted patch v3Splinter Review
Added another NULL check, we can crash in vm->mRootView->GetParent().
Attachment #378045 - Attachment is obsolete: true
(Assignee)

Comment 35

10 years ago
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
(Assignee)

Comment 36

10 years ago
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+
(Assignee)

Comment 37

9 years ago
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.