Closed Bug 351225 Opened 15 years ago Closed 15 years ago

Crash [@ nsIView::Destroy ] on print preview with The New York Times


(Core :: Layout, defect)

Not set





(Reporter: martijn.martijn, Assigned: mats)




(4 keywords)

Crash Data


(4 files, 2 obsolete files)

I found this when looking at this talkback report:

And indeed, I was also able to crash Mozilla on print preview at
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
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)
Attached file 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
Attached file testcase without table tags (obsolete) —
Attached file more reduced
Attachment #236618 - Attachment is obsolete: true
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:

and later a frame destroys the same view here:
Assignee: nobody → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
Attached file Frame dump
Here's a frame to illustrate...
Attached patch Patch rev. 1 (obsolete) — Splinter Review
This fixes the crash for me.
Attachment #236662 - Flags: review?(roc)
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: (Bug 351555)


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
URL visited
User Comments	
Since Last Crash	86990 sec
Total Uptime	86990 sec
Trigger Reason	Access violation
Source File, Line No.	N/A
Stack Trace 	
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. ***
Attached patch Patch rev. 2Splinter Review
(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?,430-470,485-530#410

There are several places where I think we could use ReparentFrameViewList
instead of looping sibling lists calling ReparentFrameView, eg:,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...
Attachment #237273 - Flags: superreview?(roc)
Attachment #237273 - Flags: review?(roc)
Attachment #236662 - Attachment is obsolete: true
Attachment #236662 - Flags: review?(roc)
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.
Attachment #237273 - Flags: superreview?(roc)
Attachment #237273 - Flags: superreview+
Attachment #237273 - Flags: review?(roc)
Attachment #237273 - Flags: review+
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. tells me there are 301 crashes with
nsIView::Destroy on the stack since 2006-05-02, many of them mentions
printing/print preview.

Closed: 15 years ago
Resolution: --- → 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() &
+    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

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.  # no calls     # no calls
                         # 1 call, optimised, 1 frame  # 70 calls, all optimised, 1.63 frames in each call. search for "firefox"  # no calls  # no calls       # no calls      # no calls       # no calls  # no calls  # 9 calls, all optimised, 1 frame in each call.        # 4 calls, all optimised, 1 frame in each call.          # no calls            # 8 calls, all optimised, 1.75 frames/call.       # no calls    # 14 calls, all optimised, 1.07 frames/call.     # 351 calls, all optimised, 1.76 frames/call.      # no calls     # 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

So, unless we find some odd dynamic content that have orders of magnitude
more calls I'd say we need not worry about this.
Flags: blocking1.8.1?
Not a topcrasher, not a blocker ...
Flags: blocking1.8.1? → blocking1.8.1-
Flags: blocking1.8.1.1?
The crash is Verified FIXED with build 2006-09-16-08 of SeaMonkey trunk under Windows XP using: and as testcases.
Comment on attachment 237273 [details] [diff] [review]
Patch rev. 2

approved for 1.8 branch, a=dveditz for drivers.
Attachment #237273 - Flags: approval1.8.1.1+
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Whiteboard: [checkin needed (1.8 branch)]
Checked in to MOZILLA_1_8_BRANCH at 2006-11-28 10:54 PST
Keywords: fixed1.8.1.1
Whiteboard: [checkin needed (1.8 branch)]
verified fixed for
Tested with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv: Gecko/20061130 BonEcho/ on Windows 2000 and XP
Crash Signature: [@ nsIView::Destroy ]
You need to log in before you can comment on or make changes to this bug.