230417 - M17b crash [@ 0x00000000 - nsIView::Destroy] - Print preview crashes at http://www.linuxworld.com/story/32629.htm

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Dom Incollingo]

1) Navigate to http://www.linuxworld.com/story/32629.htm
2) Choose File => Print Preview
3) Mozilla crashes

Sorry I don't have a talkback Id for this crash.  The Linux build for 2004010707
doesn't seem to have talkback.

Comment 1

[Ronald Tilby]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040106
This crash also occurs on Windows 2000 with Mozilla 2004010608 Nightly.

Comment 2

[James Napolitano]

WFM, Mozilla/5.0 (Windows; U; Win98; en-US; rv:) Gecko/20040309

I browsed through the html source quickly, and this page contains long nested
tables, some with the align attribute set.  This bug looks like it may be a dupe
of bug 185357.  There was a patch checked in from that bug on 2004-02-03 (See
comment 52).  Can someone on Linux please see if this bug still crashes with a
build past that date?  If not, we should resolve this as a dupe of bug 185357.

Comment 3

[Tuukka Tolvanen (sp3000)]

linux.
worksforme 2004-03-06-10-trunk seamonkey gtk1
worksforme 2004-03-05-20 cvs firefox gtk2
worksforme 2004-01-19-09-trunk seamonkey gtk1

Comment 4

[Jay Patel [:jay]]

Looks like this crash is still around.  I was able to reproduce this crash using
the steps in the original post with Mozilla 1.7 beta on Windows XP:

Incident ID: 12019
Stack Signature	0x00000000 ed8b9339
Email Address	[e-mail]
Product ID	Mozilla1.7
Build ID	2004031615
Trigger Time	2004-04-04 15:08:04.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	null
URL visited	http://www.linuxworld.com/story/32629.htm
User Comments	http://www.linuxworld.com/story/32629.htm
Since Last Crash	null sec
Total Uptime	null sec
Trigger Reason	Access violation
Source File Name	
Trigger Line No.	
Stack Trace 	
0x00000000
nsIView::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp,
line 253]
nsFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsFrame.cpp,
line 646]

Not yet a true topcrash, but seeing that it is easily reproducible, we might
want to look into this for 1.7.

Comment 5

[James Napolitano]

Hmm... this worked for me before (see comment 2), but now it crashes on print
preview. 
Mozilla 1.7b, Win98se.
My Talkback ID is TB12103Y in case you need it.

Comment 6

[chris hofmann]

Stack Signature	 0x002c006c 66c9c1b7
Email Address	[e-mail]
Product ID	Mozilla1.7
Build ID	2004031615
Trigger Time	2004-04-04 17:56:38.0
Platform	Win32
Operating System	Windows 98 4.10 build 67766446
Module	null
URL visited	http://www.linuxworld.com/story/32629.htm
User Comments	bug 230417. print preview on above URL.
Since Last Crash	null sec
Total Uptime	null sec
Trigger Reason	Access violation
Source File Name	
Trigger Line No.	
Stack Trace 	
0x002c006c
nsIView::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp,
line 253]
nsFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsFrame.cpp,
line 646]
nsSubDocumentFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/document/src/nsFrameFrame.cpp,
line 565]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsBlockFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 300]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsTableFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/table/src/nsTableFrame.cpp,
line 311]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsTableOuterFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/table/src/nsTableOuterFrame.cpp,
line 83]
nsLineBox::DeleteLineList
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsLineBox.cpp,
line 303]
nsBlockFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 303]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsTableFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/table/src/nsTableFrame.cpp,
line 311]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsTableOuterFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/table/src/nsTableOuterFrame.cpp,
line 83]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsBlockFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 300]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsTableFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/table/src/nsTableFrame.cpp,
line 311]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsTableOuterFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/table/src/nsTableOuterFrame.cpp,
line 83]
nsLineBox::DeleteLineList
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsLineBox.cpp,
line 303]
nsBlockFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 303]
nsLineBox::DeleteLineList
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsLineBox.cpp,
line 303]
nsBlockFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 303]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsTableFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/table/src/nsTableFrame.cpp,
line 311]
nsFrameList::DestroyFrames
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/base/src/nsFrameList.cpp,
line 130]
nsContainerFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 137]
nsTableOuterFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/table/src/nsTableOuterFrame.cpp,
line 83]
nsLineBox::DeleteLineList
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsLineBox.cpp,
line 303]
nsBlockFrame::Destroy
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 303]

Comment 7

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

This direly needs a minimized testcase

Comment 8

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

This could be the problem in bug 185357 comment 50 (the problems I cite there
are those documented in the XXX comments in attachment 144685 [details] [diff] [review]).

Comment 9

[James Napolitano]

I'll make the testcase.

Comment 10

[James Napolitano]

Attachment: testcase1

Here is my testcase.  This one was a real pain to make.  It turns out the
culprit is a TABLE containing another TABLE (with align="right") containing an
IFRAME.  This by itself won't crash print preview; it has to be positioned just
over the page break.  (There seem to be a few print crashers involving page
breaks).  The weird part is that the IFRAME will disappear (in print preview)
when this happens.  See my comments in the testcase for the details.

The tricky part is reproducing the crash.  Sometimes Mozilla will crash right
as soon as you hit Print Preview.  Sometimes you have to jump back and forth
between Portrait and Landscape modes a few times.  Sometimes it crashes when
you hit Close, and the rest of the time when you hit Reload.

(In case anyone wants to compare stack traces with the original webpage, my
talkback ID for this testcase is TB12500Q.)

Comment 11

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

James, that rocks!! Thank you.

This smells a bit like 156982 that is a horrible bug I was never able to track
down. But your testcase is much nicer than what I was able to get to...

Comment 12

[Patrick Smits]

I can confirm this bug. I changed the height in the testcase to 820px and now 
it crashes everytime on a print-preview. 
 
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040322 Firefox/0.8 

Comment 13

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

Attachment: more reduced testcase

This testcase is reduced to just a floating IFRAME at the right vertical
position. It doesn't crash, but you can see in Print Preview that the IFRAME
border has moved to page 2 but the IFRAME itself is stuck on page 1!!!

Comment 14

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

Gah. I see that this doesn't work on GTK1 and GTK2 because those widget
implementations don't handle SetParent. The crash is about something else.

Comment 15

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

Attachment: genuine reduced testcase

You can get the crash by removing the inner table and just making the IFRAME
"float:left".

I can't seem to get the crash if I replace the outer table with a DIV or SPAN.
I wonder if there's any non-TABLE container that will give the crash.

I can get the crash by replacing the IFRAME with an overflow:scroll DIV. That
simplifies things a bit, I think

Comment 16

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

It sure looks like dbaron's comment was correct --- the float placeholder is
deep inside the table, so DrainOverflowLines doesn't see it and reparent the out
of flow frame. But I've tried doing the obvious traversal of the overflow frames
to find nested placeholders and it doesn't work...

Comment 17

[James Napolitano]

Oh, this probably isn't related, but I noticed while making my testcase that, in
print preview, you can scroll the Iframe with your mouse (and the Iframe doesn't
have a scrollbar)!  When you do, the contents of the Iframe become blurred by
horizontal bars and are garbled.  I opened a separate bug for this: bug 239795. 

Oh, roc's comment 15 reminds me of bug 234062.  There, an image with float:left
positioned over a page break would cause a crash on print preview.

(also, in the 'genuine reduced testcase', there is a </div> where there should
be a </table>).

Comment 18

[Hiro]

Attachment: crash log of Firefox for Mac OS X

With the Mac-OS-X version, Print Preview worked normally.
But tt crashed, just as it pushed the "Close" button of Print Preview.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040404
Firefox/0.8.0+

Comment 19

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

It appears that when the problematic DrainOverflowLines is called on my latest
testcase, the overflow-list contains the table but the blockframe in the table
cell is empty! That's very weird. How could that happen? It should contain at
least a textframe with whitespace and the placeholder for the float.

Comment 20

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

Hmm. It's non-deterministic. When I start Moz -> load testcase -> print preview,
the table cell's block never has any children. When I start Moz -> load testcase
-> print preview -> open new window -> load slightly modified testcase in new
window -> (removing 'overflow:scroll' -> print preview -> close print preview ->
load original testcase in new window -> print preview, I see that the table
cells' block does have children. I suspect some uninitialized memory...

Comment 21

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

valgrind doesn't show anything :-(

Comment 22

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

Okay. So what happens when this testcase goes wrong is that the placeholder for
the floating overflow:scroll DIV is pushed to the overflow list for the table
cell's block, and the table itself is pushed to the overflow list for the page.
So, even when I add code to traverse the normal frame descendants of the table
in DrainOverflowLines for the second page, we don't see the float's placeholder.

I made my recursive traversal placeholder search search the overflow list as
well as the normal child list, but then ran into confusion about exactly how the
float should be reparented. The problem is that, as far as I can tell, the float
is actually not in the frame tree at all. It says its parent is the table cell
block, but the table cell block says it has no floater children. So out of flow
frames whose placeholders are in an overflowList aren't in the frame tree at
all? That makes it a bit hard to reparent views correctly.

Comment 23

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

Okay. Now I see that floats are not added as children of their parent's
floatList until BuildFloatList is called late in Reflow(). And floats whose
placeholders didn't fit into any lines aren't added at all. Bummer. I think this
is a mistake; I think these floats should be reachable somewhere in the frame
tree, even if they need their own special child list. It's rather hard to say
what invariants should apply to them while they're sitting out in the void
reachable only via the placeholder.

We could, for example, just stuff them into the overflow list next to their
placeholder. Then at least I know what their parent view should be. And then I
think the call to nsHTMLContainerFrame::ReparentFrameView in DrainOverflowLines
will take care of this testcase --- when we call that on the table, it will
crawl down to the float, finding it in the overflowList of the table cell's
block, and reparent its view to the new page.

Comment 24

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

This change will be a bit tricky, in poorly understood code, so I think this is
definitely something for 1.8a.

Comment 25

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

Here's a plan:

-- Make BuildFloatList also scan the overflowList; for each float placeholder
found, append the float to a new child list, overflowOutOfFlowList. This list
will be iterated through by GetAdditionalChildListName so we satisfy the
requirement that the floats be reachable through the frame tree.
-- Make DrainOverflowLines also pull frames from overflowOutOfFlowList and
reparent them to the new block.

I think this will take care of the float problems. I also need to look to see if
absolute frames need the same treatment.

Comment 26

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

I think the design of the float breaking code is fundamentally wrong to begin
with -- to push part of a float to the next page we shouldn't need to push part
of its parent block -- we should only need to push part of the block with the
space manager (if it's not already there).  Pushing part of its parent block
could, I think, lead to things getting pushed to later pages that shouldn't be.

Comment 27

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

I'm not even thinking about float breaking yet. I'll need to do some more
digging to fully understand how float breaking currently works. All I know right
now is that for float continuations we create an "overflow placeholder" to
manage the float continuation.

I think we should implement column layout to force us to deal with all these
issues :-).

Comment 28

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

I've implemented this. It works well. I think absolutely positioned out-of-flows
have the same issues and I'm going to put them on the same child list. I also
need to clean up the patch a bit, then I'll submit it.

Comment 29

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

Absolute frames currently don't follow their placeholders across pages, and
fixing that is going to be some work. So for now I'm leaving absolute frames off
the new child list (which I'm calling the "overflow out-of-flows list").

(Eventually absolute frames whose placeholders land in the overflow-line-list
will be reparented from their containing block to the block parent of their
placeholders and placed in the overflow-out-of-flows-list. Then when the
overflow lists are drained either back into the current block or into the
continuation block, absolute frames in the overflow-out-of-flows-list will be
reparented to their new containing block.)

Comment 30

[chris hofmann]

let's be thinking about this for 1.7 if we can...

Comment 31

[Asa Dotzler [:asa]]

roc, are you working that patch in another bug or here? any chance it'll be
landed on the trunk in time to test out and get onto the branch for 1.7? 

Comment 32

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

I'm working on it here. I had a home network outage this week, that's why I
haven't attached the patch yet. I'll do it right now.

Comment 33

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

Attachment: fix

As promised.

I don't claim this is particularly low risk, though. This overflow-lines code
is a bit fragile. If we can get it on the trunk quickly and pound on it a bit,
that's probably the way to go.

Comment 34

[Asa Dotzler [:asa]]

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

Comment 35

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

Comment on attachment 146900 [details] [diff] [review]
fix

>+// Destructor function for the overflowPlaceholders frame property
>+static void
>+DestroyOverflowOOFs(nsIPresContext* aPresContext,
>+                    nsIFrame*       aFrame,
>+                    nsIAtom*        aPropertyName,
>+                    void*           aPropertyValue)
>+{
>+  delete NS_STATIC_CAST(nsFrameList*, aPropertyValue);
>+}

It seems like this should never be called.  Maybe add NS_NOTREACHED if so?


> nsBlockFrame::BuildFloatList()
>-  for (line_iterator line = begin_lines(), line_end = end_lines();
>+  for (nsBlockFrame::line_iterator line = mLines.begin(), line_end = mLines.end();

why?

>+  if (overflowLines) {
>+    head = nsnull;
>+    current = nsnull;
>+
>+    nsIFrame* frame = overflowLines->front()->mFirstChild;
>+    while (frame) {
>+      if (nsLayoutAtoms::placeholderFrame == frame->GetType()) {
>+        nsIFrame *outOfFlowFrame = NS_STATIC_CAST(nsPlaceholderFrame*, frame)->GetOutOfFlowFrame();
>+        if (outOfFlowFrame &&
>+            !outOfFlowFrame->GetStyleDisplay()->IsAbsolutelyPositioned()) {
>+          // It's not an absolute or fixed positioned frame, so it
>+          // must be a float!
>+          // XXX This is a lame-o way of detecting a float, but it's the only way
>+          // apparently
>+          if (!head) {
>+            head = current = outOfFlowFrame;
>+          } else {
>+            current->SetNextSibling(outOfFlowFrame);
>+            current = outOfFlowFrame;
>+          }
>+        }
>+      }
>+      frame = frame->GetNextSibling();
>+    }
>+    
>+    if (current) {
>+      current->SetNextSibling(nsnull);
>+      nsFrameList* frameList = new nsFrameList(head);
>+      if (frameList) {
>+        SetOverflowOutOfFlows(frameList);
>+      }
>+    }

This seems like it has the same problem as the old code -- it doesn't find
placeholders within inlines.  Given that, I'm not really sure how it fixes the
bug.  What's going on here?

Comment 36

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

> It seems like this should never be called.  Maybe add NS_NOTREACHED if so?

Right. Sure.

> why?

Oh, I originally had that code in a static helper function, then I moved it out.
I'll change it back.

> This seems like it has the same problem as the old code -- it doesn't find
> placeholders within inlines.

The bug here is when the float placeholder is in the table cell block's
overflowLines, and the table itself is in its containing block's overflowLines.
When we pull the table to the next page, we call ReparentFrameView on the table,
which searches all the table's descendant frames looking for views to reparent.
It doesn't find the float's view because the float is simply not on any child
list of the table cell block. Later when we reflow the table cell, it pulls back
its overflow frames (at the end of DrainOverflowLines) but doesn't reparent them
(because they're already its children). So the float's view's parent stays wrong.

With this patch applied, when the table is pulled to the next page, we call
ReparentFrameView on the table as before and this time it finds the float on our
new child list and reparents its view to the new page. Everything else just works.

There may be bugs related to placeholders nested inside inlines, but this isn't
one. A fix for such bugs should be orthogonal to what we're doing here.

Comment 37

[Jay Patel [:jay]]

Has anyone reproduced this with Mozilla 1.7 rc1?  I was able to reproduce with
beta, but can't seem to get a crash with rc1.

Comment 38

[Michael Taeuber]

(In reply to comment #37)
> Has anyone reproduced this with Mozilla 1.7 rc1?  I was able to reproduce with
> beta, but can't seem to get a crash with rc1.

I've checked it with the current 1.7 nightly (Mozilla/5.0 (Windows; U; Windows
NT 5.1; de-AT; rv:1.7) Gecko/20040503). The output of the print preview is
showing up but if I want to close the print preview window, Mozilla crashes.
Talkback ID TB37620H
Mozilla 1.6 crashes directly after clicking on 'Print preview'

Comment 39

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

Comment on attachment 146900 [details] [diff] [review]
fix

OK, then r+sr=dbaron given the changes in comment 35 / comment 36 and the
re-adding in your new code in BuildFloatList of my XXXldb comment that you
removed from DrainOverflowLines.

Comment 40

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

Will do. Thanks.

Comment 41

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

checked in.

Comment 42

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

marking fixed

Comment 43

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

whoops. Forgot about the branch.

Comment 44

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

Comment on attachment 146900 [details] [diff] [review]
fix

I think we should check this in on the branch after it has been on the trunk
for a few days.

Comment 45

[Asa Dotzler [:asa]]

Time is running out on RC2. If this is going to make it into the second
candidate, it'll need to land pretty quickly. Any indication yet how this is
faring on the trunk?

Comment 46

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

Well, no regressions have been traced back to it yet :-)

I can land it on the branch tonight if you like.

Comment 47

[chris hofmann]

Comment on attachment 146900 [details] [diff] [review]
fix

a=chofmann for 1.7

Comment 48

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

checked into branch.

Comment 49

[Jay Patel [:jay]]

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

Comment 50

[Jay Patel [:jay]]

Loading print preview for the linuxworld page doesn't crash anymore, but I do
crash when closing the print preview.  I'm not going to reopen this, hopefully
we can figure out the closing print preview crash in bug 185357.

Comment 51

[Jay Patel [:jay]]

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

Comment 52

[Karen Huang]

Verified as fix on latest 1.7 branch Win 06-24,Mac 06-30 & Linux 06-29 builds.
But found crash occurs on something else for Linux while verifying this bug...
(see bug 249486)
Changing keywords from fixed1.7 to verified1.7.
Leave this bug status "as is" until this bug be verified on trunk again...