274516 - Crash when printing directions with maps (mapquest.com) [@ nsIFrame::HasView]

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

(spawned off from bug 274424)

Using the "Steps to reproduce" from bug 274424:

Steps to Reproduce:
1. Create a long route (~10 turns) 
2. On the resulting page, click the 'map' link next to each turn, so you can see
the 'turn-by-turn' directions.
3. After expaninding, print.

results in a crash using both Print and Print Preview (scroll down a bit)
using Mozilla 2004-12-10-06.  Mozilla 1.7.3 does not crash.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Stack

Comment 2

[Mats Palmgren (inactive)]

Regression window 2004-11-25-06 -- 2004-11-26-06:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-11-25+06%3A00&maxdate=2004-11-26+06%3A00&cvsroot=%2Fcvsroot

(Maybe bug 209694 or bug 270804?)

Comment 3

[Martijn Wargers (dead)]

Attachment: Almost minimal testcase

Testcase derived from the site that still 100% reproducable crashes on print
preview in my 20041213 trunk build.

Comment 4

[Martijn Wargers (dead)]

The testcase also crashes with builds older than 2004-11-25.

The url testcase works with:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/2004-11-25-05
But it crashes on print preview with:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/2004-11-25
Firefox/0.9.1+ 10:19am

So I think that the fix for bug 209694 somehow now triggers this crash, but the
crash potential was always there on that site on print preview (see testcase).

Comment 5

[Asa Dotzler [:asa]]

roc's still out, I think. Who else can look into this? dbaron or bz, can either
of you take a look for 1.8a6?

Comment 6

[Asa Dotzler [:asa]]

too late for 1.8a6. blocking-

Comment 7

[Todd]

Hi All,

    I came across a customer with the same this problem yesterday.  She was
able to print mapquest directions to her HP LJ4100 PCL printer but not to
her Xerox 432st PCL5e printer.  Same symptom as described here.  I fussed with
her Xerox print driver, changed graphics from "raster" to "vector" and she
started printing from her Xerox (no crashing either).

Comment 8

[Bernd]

I see 
WARNING: data loss - complete row needed more height than available, on top of p
age, file e:/moz_src/mozilla/layout/tables/nsTableRowGroupFrame.cpp, line 1075
WARNING: data loss - complete row needed more height than available, on top of p
age, file e:/moz_src/mozilla/layout/tables/nsTableRowGroupFrame.cpp, line 1075
###!!! ASSERTION: data loss - incomplete row needed more height than available,
on top of page: 'rowMetrics.height <= rowReflowState.availableHeight', file e:/m
oz_src/mozilla/layout/tables/nsTableRowGroupFrame.cpp, line 1043

but no crash

Comment 9

[Bernd]

Martijn, could you please reevaluate this bug, with winxp 20050202 I don't
crash(url and testcase are OK).
I fixed bug 269648 on 2004-12-01
        bug 272830 on 2005-01-01
        bug 277062 on 2005-02-01. 
One of them might have fixed it.

Comment 10

[Martijn Wargers (dead)]

I evaluated with win2000 20050202 trunk Firefox build.
I don't crash on print preview anymore, with the url.
With the testcase, Firefox hangs/freezes on print preview. But that is probably
because of bug 245300, I guess.
So I think this bug is fixed.

Comment 11

[Bernd]

It seems that the assert is triggered by a misbehaviour of the outer table
frame, during the height constrained resize reflow the outer table frame does
not reduce the available height for the inner table frame
 tblO 03645200 r=2 a=9048,14568 c=UC,UC cnt=363
  tbl 0364530C r=2 a=9048,14568 c=UC,UC cnt=364

but does take the margin into account on exit

   tbl 0364530C d=1884,14568 me=756 o=(0,0) 1884 x 24000
  tblO 03645200 d=1884,14628 me=756 o=(0,0) 1884 x 24060

removing the style="margin-top:5px;" as consequence hides the assertion

Comment 12

[Bernd]

Attachment: patch

Martijn, could you test whether the patch fixes the hang (I don't see a hang)

Comment 13

[Martijn Wargers (dead)]

No, the patch doesn't fix the hang, I'm afraid. I just had time to test it.

Comment 14

[Bernd]

the reminder seems to be a Linux problem (I dont have a linux build)

Comment 15

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

but Martjin is seeing the hang on Windows, so it can't be just a Linux problem.

Comment 16

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

bernd, take a look at
http://lxr.mozilla.org/mozilla/source/layout/tables/nsTableFrame.cpp#3258
We call PushChildren to push nextRowGroupFrame *and its following siblings* to
the next page. The problem is, nextRowGroupFrame's next sibling is the TBODY
that we already put on the current page (because the row group sorting put the
TBODY first in rowGroups).

The problem is that PushChildren pushes nextRowGroupFrame and its following
siblings, but we really want to push nextRowGroupFrame and anything after it in
the rowGroup list.

Comment 17

[Bernd]

Hmm, but it seems that Martijn is the only one who sees the hang? This bug is
wfm win2005020204 winxp seamonkey and win20050206 ff1.0+. Martijn could you
produce a reflow log with the patch? A couple of pages 4-5 is enough.

Comment 18

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b) Gecko/20050201

I don´t hang or crash, but in Print Preview I get only three pages, showing the
first 8 maps of 25 total expanded on the first two pages, and the overview map
on the third page. 

Comment 19

[Hermann Schwab]

installed current nightlies and retested 2005020106
wfm in preview seeing all 25 expanded maps on 7 pages
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.6) Gecko/20050206
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b) Gecko/20050206
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b) Gecko/20050201 (now working)	

Comment 20

[Martijn Wargers (dead)]

Attachment: logfile

This is the logfile asked by Bernd in comment 17 with a Mozilla debug build
with the patch applied. I hope this is what is needed here.
It is the reflow log of the moment when I do a print preview. I've also added
here the warnings and assertions I get on print preview on the testcase just
before it begins with hanging. Maybe also useful.

WARNING: data loss - complete row needed more height than available, on top of
p
age, file c:/mozilla/mozilla/layout/tables/nsTableRowGroupFrame.cpp, line 1075
WARNING: data loss - complete row needed more height than available, on top of
p
age, file c:/mozilla/mozilla/layout/tables/nsTableRowGroupFrame.cpp, line 1075
###!!! ASSERTION: aContent1 must not be null: 'aContent1', file
c:/mozilla/mozil
la/layout/base/nsLayoutUtils.cpp, line 242
###!!! ASSERTION: aContent2 must not be null: 'aContent2', file
c:/mozilla/mozil
la/layout/base/nsLayoutUtils.cpp, line 243
WARNING: data loss - complete row needed more height than available, on top of
p
age, file c:/mozilla/mozilla/layout/tables/nsTableRowGroupFrame.cpp, line 1075
###!!! ASSERTION: bad prev sibling: 'aPrevSibling->GetNextSibling() ==
aFromChil
d', file c:/mozilla/mozilla/layout/tables/nsTableFrame.cpp, line 2209
###!!! ASSERTION: loop in frame list.  This will probably hang soon.: 'Error',
f
ile c:/mozilla/mozilla/layout/generic/nsFrameList.cpp, line 636
Break: at file c:/mozilla/mozilla/layout/generic/nsFrameList.cpp, line 636

Comment 21

[Bernd]

Martijn, does removing the float from the div make the picture differ? If so it
would indicate that this bug is really dependent on bug 245300.

Comment 22

[Martijn Wargers (dead)]

Attachment: Log of patched debug build with testcase without float

Removing the float style from the testcase still makes the patched build and
the normal build hang on print preview.
The assertions and warnings of the patched debug build seems different, though.

Comment 23

[Bernd]

I don't see the hang on my machine, its somehow intermittent. I will focus my
attention on bug 279316, which seems similiar and might finally help with this. 
I would recommend to minus this bug as it is partially a WFM and I don't see any
chance that this will make it till the deadline. It should however block ff 1.1.

Comment 24

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

I still hang in this testcase. I'll try fixing what I think is the obvious bug...

Comment 25

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

I still hang in this testcase. I'll try fixing what I think is the obvious bug...

Comment 26

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

Attachment: fix

This patch fixes the hang in this bug and also the crash testcases #1 and #2 in
bug 279316. I can even print preview the Mapquest URL given at the top of this
bug.

Basically it's just what I said ... currently PushChildren is being called as
if it will push the aFromChild frame and all the following frames from the
rowGroups array. But actually it pushes aFromChild and its following siblings,
which is a different set of frames. (E.g., in this testcase it pushes the
footer and the body, but it should only push the footer.) So I just changed
PushChildren to really push the frames from the current point in the rowGroups
array.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

>Index: layout/tables/nsTableFrame.cpp
>+    // hint that f's prev-sibling is probably lastFrame
>+    mFrames.RemoveFrame(f, lastFrame);

Is there an nsFrameList part of this patch that's missing?  It doesn't have a
RemoveFrame with that signature that I see...

For that matter, would you mind using an nsFrameList for the frames to push? 
You could still keep track of the last frame and use nsFrameList::InsertFrame
to keep this from being O(N^2), and if we actually get to the point where frame
lists can get at the last frame fast we can simplify the code to make use of
that.

Also, did you mean to break out of the loop if you hit aFooterFrame?  You don't
seem to do that.

>Index: layout/tables/nsTableFrame.h

>+  void PushChildren(const nsAutoVoidArray& aFrames, PRInt32 aPushFrom,
>+                    nsTableRowGroupFrame* aFooterFrame);

Document what the function does and what the args mean?

Comment 28

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

Attachment: fix #2

Okay, as requested. Actually it turns out we don't need the footer parameter.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174391 [details] [diff] [review]
fix #2

>Index: layout/tables/nsTableFrame.cpp
>+    nsIFrame* f = NS_STATIC_CAST(nsIFrame*, aFrames.ElementAt(childX));

Can probably use FastElementAt.

>+    // hint that f's prev-sibling is probably lastFrame
>+    mFrames.RemoveFrame(f, lastFrame);
>+    frames.InsertFrame(nsnull, lastFrame, f);

This is actually a problem, possibly... InsertFrame will set the NextSibling of
'f' to the nestsibling of lastFrame (that is, to null).  then the next time
through the loop, the hint to RemoveFrame is useless....

Perhaps we want to be iterating over the frames from the end to the front, and
passing the previous element of the array as the hint?

I'm not sure why footers are not an issue.  Shouldn't we avoid pushing them? Or
are they not in the array to start with?

Comment 30

[Bernd]

they are the first ones in the array due to
http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#10985

Comment 31

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

(In reply to comment #30)
> they are the first ones in the array due to
>
http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#10985

No, actually they're last in the array thanks to OrderRowGroups. And
construction of the first table frame can put them anywhere in the child list,
as far as I can tell.

Pushing the footer isn't the right thing to do for repeated footers, but it is
the right thing to do when the footer is not repeated. I don't really understand
how the repeating stuff works yet. Should I just exclude the footer from the
list of children to push if it returns PR_TRUE to "IsRepeatable()"?

> This is actually a problem, possibly... InsertFrame will set the NextSibling
> of 'f' to the nestsibling of lastFrame (that is, to null).  then the next time
> through the loop, the hint to RemoveFrame is useless....
>
> Perhaps we want to be iterating over the frames from the end to the front, and
> passing the previous element of the array as the hint?

No, that was already wrong. The lastFrame hint should just be constant
throughout the loop. I'll make a new patch.

Comment 32

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

Attachment: fix #3

Okay. This skips pushing repeating footers. And I think it gives a good hint
most of the time.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174444 [details] [diff] [review]
fix #3

>Index: layout/tables/nsTableFrame.h
>+   * Push all our child frames from the aFrames array, in order, starting from the
>+   * frame at aPushFrom to the end of the array. The frames are put on our overflow
>+   * list or moved directly to our next-in-flow if one exists.
>+   */

Document that this doesn't push repeatable rowgroups, and sr=bzbarsky

Comment 34

[Asa Dotzler [:asa]]

bernd, can you get us a review on this quickly so we can get it into beta? 

Comment 35

[Bernd]

Comment on attachment 174444 [details] [diff] [review]
fix #3

Robert,I did not the promised printing regression test, I will have time only
over the weekend for it.
But it looks OK to me. The decision whether a frame is repeatable or not is
based on the result of the first height unconstrained reflow. During this we
calculate the height if it is less than a quarter of the page height than it is
repeatable.

Comment 36

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

Comment on attachment 174444 [details] [diff] [review]
fix #3

requesting approval for this b1 blocker

Comment 37

[chris hofmann]

Comment on attachment 174444 [details] [diff] [review]
fix #3

a=chofmann for 1.8b

Comment 38

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

fix checked in

Comment 39

[Bernd]

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