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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

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)

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

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

Comment 3

[Bernd]

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

Comment 4

[Bernd]

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

Comment 5

[Bernd]

Attachment: testcase without table tags

Comment 6

[Bernd]

Attachment: more reduced

Comment 7

[Bernd]

Robert looks like your turf

Comment 8

[Mats Palmgren (inactive)]

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

Comment 9

[Mats Palmgren (inactive)]

Attachment: Frame dump

Here's a frame to illustrate...

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This fixes the crash for me.

Comment 11

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

Shouldn't we be checking other frame lists too?

Comment 12

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

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

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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]

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

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

Comment 16

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

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.

Comment 17

[Mats Palmgren (inactive)]

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

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 19

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

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

Comment 20

[Mats Palmgren (inactive)]

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

Comment 21

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

Not a topcrasher, not a blocker ...

Comment 22

[Stephen Donner [:stephend] Not actively reading bugmail]

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 23

[Daniel Veditz [:dveditz]]

Comment on attachment 237273 [details] [diff] [review]
Patch rev. 2

approved for 1.8 branch, a=dveditz for drivers.

Comment 24

[Mats Palmgren (inactive)]

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

Comment 25

[Carsten Book [:Tomcat]]

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