490425 - Crash [@ nsViewManager::DispatchEvent]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

Attachment: 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

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

Hold reference so DoSetWindowDimensions() will not destroy us.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 375186 [details] [diff] [review]
patch

Can you please review this patch?

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

This is a RHT internal topcrash.

Comment 4

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

It would be better to hold the strong reference in HandleEvent --- having 'this' go away anywhere inside a method is bad news...

Comment 5

[Daniel Veditz [:dveditz]]

roc: Is comment 4 an "r-" ? That is, is the ball in your court as reviewer or back to the assignee?

Comment 6

[Daniel Veditz [:dveditz]]

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.

Comment 7

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

Comment on attachment 375186 [details] [diff] [review]
patch

The patch is almost right but not quite.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: updated one

Reference is moved to HandleEvent()

Comment 9

[Martijn Wargers (dead)]

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?

Comment 10

[Martin Stránský [:stransky] (ni? me)]

(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.

Comment 11

[Martijn Wargers (dead)]

Sorry, never mind, it turned out I 'm seeing a recent regression, I filed bug 491848 for it.

Comment 12

[Daniel Veditz [:dveditz]]

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?

Comment 13

[Daniel Veditz [:dveditz]]

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.

Comment 14

[Daniel Veditz [:dveditz]]

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

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 375580 [details] [diff] [review]
updated one

a191=beltzner

Comment 16

[Martijn Wargers (dead)]

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.

Comment 17

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 18

[Martin Stránský [:stransky] (ni? me)]

Seems to affect 1.8 too, the patch should apply fine.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

So we really have no way to cause this crash on demand, right?

Comment 20

[Samuel Sidler (old account; do not CC)]

No, but if Martin or caillon can get one of there users to verify it, that'd go a long way.

Comment 21

[Christopher Aillon (sabbatical, not receiving bugmail)]

https://bugzilla.redhat.com/show_bug.cgi?id=488570#c91 should help with verification.

Comment 22

[Daniel Veditz [:dveditz]]

Checked in
  http://hg.mozilla.org/mozilla-central/rev/c18316c01045
  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bfb9bfaa286d

Comment 23

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 24

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

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

Comment 27

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

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

Comment 29

[Al Billings [:abillings - ex-MoCo]]

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).

Comment 30

[Martin Stránský [:stransky] (ni? me)]

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);
}

Comment 31

[Martin Stránský [:stransky] (ni? me)]

Attachment: null pointer check at nsViewManager::DispatchEvent()

Comment 32

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

Comment on attachment 378026 [details] [diff] [review]
null pointer check at nsViewManager::DispatchEvent()

+              if(vm->mRootView) {

Space in "if("

Comment 33

[Martin Stránský [:stransky] (ni? me)]

Attachment: null pointer check at nsViewManager::DispatchEvent() with fixed formatting

Comment 34

[Martin Stránský [:stransky] (ni? me)]

Attachment: v3

Added another NULL check, we can crash in vm->mRootView->GetParent().

Comment 35

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 378810 [details] [diff] [review]
v3

Roc, can you look at this one please? There's another check in for cycle.

Comment 36

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 37

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 378810 [details] [diff] [review]
v3

Cool! Can we get this one checked in?

Comment 38

[Justin Wood (:Callek)]

(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.

Comment 39

[Justin Wood (:Callek)]

(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