I found this when looking at this talkback report: http://talkback-public.mozilla.org/search/start.jsp?search=2&type=iid&id=22769607 And indeed, I was also able to crash Mozilla on print preview at http://nytimes.com For some reason, I'm not able to crash it now. I've managed to create a minimised testcase, that crashes on print preview when switching to landscape or portrait. Talkback ID: TB22843329Y 0x00000000 nsIView::Destroy [mozilla\view\src\nsview.cpp, line 303] The testcase crashes on 1.8.0.x, 1.8.1 and current trunk builds. It doesn't crash in Mozilla1.7, so it seems like a regression. I can look for a regression window, if desired. (cc-ing Bernd, as there are tables involved with the testcase)

(In reply to comment #0) > I can look for a regression window, if desired. Sorry, I can't do that, print preview portrait/landscape was almost for a year severely busted.

It crashes older trunk (20060111), so nothing really new.

It triggers ###!!! ASSERTION: Allowed only one anonymous view between frames: 'ancestorView == view->GetParent()->GetParent()', file d:/moz_src/mozilla/layout/generic/nsCon tainerFrame.cpp, line 268

Robert looks like your turf

Finally!! I've been hunting this bug on several occasions over the past couple of years, but I could never put my finger on it. In most cases one would first see a view destroy one of its child views here: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/view/src/nsView.cpp&rev=3.233&root=/cvsroot&mark=211#203 and later a frame destroys the same view here: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsFrame.cpp&rev=3.669&root=/cvsroot&mark=647#618

Here's a frame to illustrate...

This fixes the crash for me.

Shouldn't we be checking other frame lists too?

In fact, I think you should just get rid of that "optimization".

I can reproduce this crash when closing print preview for following website: http://www.monunivers.com/scrabble/ods.htm (Bug 351555) TB23014820X Stack Signature 0x00000000 d1f07783 Product ID Firefox2 Build ID 2006090603 Trigger Time 2006-09-07 13:01:21.0 Platform Win32 Operating System Windows NT 5.1 build 2600 Module URL visited http://www.monunivers.com/scrabble/ods.htm User Comments Since Last Crash 86990 sec Total Uptime 86990 sec Trigger Reason Access violation Source File, Line No. N/A Stack Trace 0x00000000 nsIView::Destroy [mozilla/view/src/nsView.cpp, line 305] nsSplittableFrame::Destroy [mozilla/layout/generic/nsSplittableFrame.cpp, line 71] nsBlockFrame::Destroy [mozilla/layout/generic/nsBlockFrame.cpp, line 317] nsFrameList::DestroyFrames [mozilla/layout/generic/nsFrameList.cpp, line 138] nsLineBox::DeleteLineList [mozilla/layout/generic/nsLineBox.cpp, line 325] nsLineBox::DeleteLineList [mozilla/layout/generic/nsLineBox.cpp, line 325] nsFrameList::DestroyFrames [mozilla/layout/generic/nsFrameList.cpp, line 138] ViewportFrame::Destroy [mozilla/layout/generic/nsViewportFrame.cpp, line 67] nsFrameList::DestroyFrames [mozilla/layout/generic/nsFrameList.cpp, line 138] nsFrameList::DestroyFrames [mozilla/layout/generic/nsFrameList.cpp, line 138] nsHTMLScrollFrame::Destroy [mozilla/layout/generic/nsGfxScrollFrame.cpp, line 172] ViewportFrame::Destroy [mozilla/layout/generic/nsViewportFrame.cpp, line 67] DocumentViewerImpl::ReturnToGalleyPresentation [mozilla/layout/base/nsDocumentViewer.cpp, line 4021] DocumentViewerImpl::ExitPrintPreview [mozilla/layout/base/nsDocumentViewer.cpp, line 3754] XPCWrappedNative::CallMethod [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2169] XPC_WN_CallMethod [mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1449] js_Invoke [mozilla/js/src/jsinterp.c, line 1353] js_InternalInvoke [mozilla/js/src/jsinterp.c, line 1451] JS_CallFunctionValue [mozilla/js/src/jsapi.c, line 4419] XPC_NW_FunctionWrapper [mozilla/js/src/xpconnect/src/XPCNativeWrapper.cpp, line 387] js_Invoke [mozilla/js/src/jsinterp.c, line 1353] js_Interpret [mozilla/js/src/jsinterp.c, line 4052] js_Invoke [mozilla/js/src/jsinterp.c, line 1372] js_InternalInvoke [mozilla/js/src/jsinterp.c, line 1451] JS_CallFunctionValue [mozilla/js/src/jsapi.c, line 4419]

*** Bug 351555 has been marked as a duplicate of this bug. ***

(In reply to comment #12) > In fact, I think you should just get rid of that "optimization". I agree. After removing that block ReparentFrameView/ReparentFrameViewList can be rewritten to share code, but I didn't bother since I expect the view related code will be removed soon anyway? http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsHTMLContainerFrame.cpp&rev=3.218&root=/cvsroot&mark=416-419,430-470,485-530#410 There are several places where I think we could use ReparentFrameViewList instead of looping sibling lists calling ReparentFrameView, eg: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsContainerFrame.cpp&rev=1.262&root=/cvsroot&mark=996-998,1029-1031#978 It seems unnecessary to calculate the ancestor views on every iteration... (there are a few other places as well), but again, I expect this code will be removed...

Hopefully views will be removed, but I'm tied up on all kinds of stuff and I don't know when it will happen. If it's easy (it looks like it should be), you might want to go ahead and do those cleanups ... it might mitigate performance impact from this patch, if there is any.

Checked in to trunk at 2006-09-11 20:47 PDT I watched carefully the various T* performance metrics on all the Tinderboxes and I couldn't detect any change due to this checkin (that's not a guarantee there won't be a problem of course). I filed bug 352300 on the potential cleanup/optimisation. Even though there is a performance risk with this patch I still think we should consider it for branches. http://talkback-public.mozilla.org/ tells me there are 301 crashes with nsIView::Destroy on the stack since 2006-05-02, many of them mentions printing/print preview. -> FIXED

Would it have been valid instead to replace the check with a check of the HAS_CHILD_WITH_VIEW bit: // This code is called often and we need it to be as fast as possible, so // see if we can trivially detect that no work needs to be done - if (!aChildFrame->HasView()) { - // Child frame doesn't have a view. See if it has any child frames - if (!aChildFrame->GetFirstChild(nsnull)) { - return NS_OK; - } + if (!(aChildFrame->GetStateBits() & + (NS_FRAME_HAS_VIEW | NS_FRAME_HAS_CHILD_WITH_VIEW))) { + return NS_OK; } (I had this diff lying around in my tree, no idea why I put it there.)

Yes. But I'm just as happy with it removed.

(In reply to comment #18) > Would it have been valid instead to replace the check with a check of the > HAS_CHILD_WITH_VIEW bit Good point, I think you're right, assuming that we can trust that bit... I know it's set when not needed in many cases, but that shouldn't bother us here, but we have had bugs in the past due to it not being set correctly... BTW, if we do trust it then we can also use it in ReparentFrameViewTo() to prune the frame tree walk. I'm not sure how much of these optimisations are needed though (especially on trunk where views will go away hopefully). I did load tests on a few selected pages and my impression is that this method is rarely called and when it is called it is very few child frames that ReparentFrameViewTo would walk. In most cases it's the child frame is a Text, Image or an empty Inline frame, or possibly with a single Text frame grandchild. Startup about:blank # 8 calls, all optimised, 1.5 frame in each call. http://edition.cnn.com/ # no calls http://slashdot.org/ # no calls http://news.bbc.co.uk/sport3/worldcup2002/hi/history/newsid_1966000/1966379.stm # 1 call, optimised, 1 frame http://news.google.com/ # 70 calls, all optimised, 1.63 frames in each call. http://google.com/ search for "firefox" # no calls http://download.java.net/jdk6/docs/api/index.html # no calls http://www.msdn.com/ # no calls http://www.yahoo.com/ # no calls http://news.com.com/ # no calls http://www.aljazeera.net/ # no calls http://www.yomiuri.co.jp/ # 9 calls, all optimised, 1 frame in each call. http://uk.imdb.com/ # 4 calls, all optimised, 1 frame in each call. http://wired.com/ # no calls http://news.cn/ # 8 calls, all optimised, 1.75 frames/call. http://www.loot.com/ # no calls http://www.lesechos.fr/ # 14 calls, all optimised, 1.07 frames/call. http://www.spiegel.de/ # 351 calls, all optimised, 1.76 frames/call. http://www.intel.com/ # no calls http://news.bbc.co.uk/ # 2 calls, all optimised, 1 frame/call. All calls were pruned by your suggested optimisation. I also tested without the optimisation and measured the avg. number of ancestors we look at to find the view in ReparentFrameView(): 1.152 I also measured the avg. number of calls to ReparentFrameView() that actually invokes ReparentFrameViewTo(): 0 (due to the early return when "aOldParentFrame == aNewParentFrame". All of the above is in galley mode. I did a few tests in Print Preview also and the number of calls went up a bit (times 2 or 3) but nothing alarming. So, unless we find some odd dynamic content that have orders of magnitude more calls I'd say we need not worry about this.

Not a topcrasher, not a blocker ...

The crash is Verified FIXED with build 2006-09-16-08 of SeaMonkey trunk under Windows XP using: https://bugzilla.mozilla.org/attachment.cgi?id=236616 and https://bugzilla.mozilla.org/attachment.cgi?id=236619 as testcases.

Comment on attachment 237273 [details] [diff] [review] Patch rev. 2 approved for 1.8 branch, a=dveditz for drivers.

Checked in to MOZILLA_1_8_BRANCH at 2006-11-28 10:54 PST

verified fixed for 1.8.1.1 Tested with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.1pre) Gecko/20061130 BonEcho/2.0.0.1pre on Windows 2000 and XP