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

VERIFIED FIXED

Status

()

Core
Layout
--
critical
VERIFIED FIXED
12 years ago
7 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: mats)

Tracking

(4 keywords)

Trunk
crash, regression, testcase, verified1.8.1.1
Points:
---
Bug Flags:
blocking1.8.1 -
blocking1.8.1.1 +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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)
(Reporter)

Comment 1

12 years ago
Created attachment 236616 [details]
testcase
(Reporter)

Comment 2

12 years ago
(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

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

Comment 4

12 years ago
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

12 years ago
Created attachment 236618 [details]
testcase without table tags

Comment 6

12 years ago
Created attachment 236619 [details]
more reduced
Attachment #236618 - Attachment is obsolete: true

Comment 7

12 years ago
Robert looks like your turf
(Assignee)

Comment 8

12 years ago
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
Assignee: nobody → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 9

12 years ago
Created attachment 236660 [details]
Frame dump

Here's a frame to illustrate...
(Assignee)

Comment 10

12 years ago
Created attachment 236662 [details] [diff] [review]
Patch rev. 1

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:
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. ***
(Assignee)

Comment 15

12 years ago
Created attachment 237273 [details] [diff] [review]
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...
Attachment #237273 - Flags: superreview?(roc)
Attachment #237273 - Flags: review?(roc)
(Assignee)

Updated

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

Comment 17

12 years ago
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
Status: NEW → RESOLVED
Last Resolved: 12 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() &
+        (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.
(Assignee)

Comment 20

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

Updated

12 years ago
Flags: blocking1.8.1?
Not a topcrasher, not a blocker ...
Flags: blocking1.8.1? → blocking1.8.1-
(Assignee)

Updated

12 years ago
Flags: blocking1.8.1.1?
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.
Status: RESOLVED → VERIFIED
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)]
(Assignee)

Comment 24

11 years ago
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 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
Keywords: fixed1.8.1.1 → verified1.8.1.1
Crash Signature: [@ nsIView::Destroy ]
You need to log in before you can comment on or make changes to this bug.