302911 - crash when attempting to print the page [@ nsCachedStyleData::GetStyleData]

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[David Swegen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.7.10) Gecko/20050717 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.7.10) Gecko/20050717 Firefox/1.0.6

Trying to print the page in the URL field causes firefox to crash. 

Reproducible: Always

Steps to Reproduce:
1. Go to the page in the URL field.
2. Print using the dialog in File->Print
3. Watch firefox crash

Actual Results:  
Print progress dialog box comes up briefly, then firefox crashes.

Expected Results:  
Page should be printed, and functionality return to normal.

The interesting thing is that from this [1] list of geocaches, only the one in
the URL field crashes. All the others print quite happily.

[1] http://www.geocaching.com/seek/nearest.aspx?lat=55.8815&lon=14.2328

Comment 1

[Adam Guthrie]

Please provide a talkback ID for the crash. http://kb.mozillazine.org/Talkback

Comment 2

[Brian]

Also crashing while printing this page.

http://wiki.jboss.org/wiki/Wiki.jsp?page=IWantToConnectToXYZShouldIUseJCA

Comment 3

[Andreas]

I have encountered the same problem with many of the longer articles on Wikipedia.
Last time it happened it was 

http://de.wikipedia.org/wiki/T%C3%BCrkei (German page on Turkey)

but it happens frequently and I will add more URLs as they come.


Kind regards


zapyon

Comment 4

[Andreas]

Sorry, I forgot. I am using Firefox 

Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.7.10) Gecko/20050825 Firefox/1.0.4
(Debian package 1.0.4-2sarge3)

or Mozilla

Mozilla/5.0 (X11; U; Linux i686; de-AT; rv:1.7.8) Gecko/20050831
Debian/1.7.8-1sarge2

on Debian Linux.


Kind regards


Andreas

Comment 5

[Adam Guthrie]

Stack traces please! See comment 1 or
http://www.mozilla.org/unix/debugging-faq.html.

Comment 6

[Jesse Moorman]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051025 Firefox/1.5

I confirm such a crash on
http://travel.state.gov/visa/reciprocity/Country%20Folder/I/Iran.htm

After two crashes, I saved the page, and printing that document also crashed Firefox.

Comment 7

[Robert Mayberry]

http://www.ariba.com/suppliers/suppliers_FAQ.cfm

THe above link also crashes firefox (the program hangs in the "preparing to print" step). Quality feedback agent does not pop up.

Comment 8

[Adam Guthrie]

Robert, what build were you using? You guys need to get stacktraces or talkback ID's for the crashes. See comment 1 and comment 5.

Comment 9

[Kyle A]

I discovered this problem a while back. After getting input from others, I've come to learn that there's something in the CSS code that is causing the crash. Honestly, I don't know what that problem is.

It is, though, because of CSS. Try disabling CSS before printing again.

Comment 10

[Jim Worthington]

I have observed this problem when trying to print to a network printer with both postscript and non-postscript drivers.  It doesn't crash all the time, but enough to be annoying. Crash is repeatable for a given web page that is crashing.  Problem has been present with all pre-release versions of Firefox 1.5.  Firefox 1.x.x didn't have this problem. 

Also could not successfully roll back to version 1.0.7. to work around this printing problem. There is a large blank grey area at bottom of the window after reinstalling 1.0.7.  Had to revert to IE to print.

Comment 11

[Lucius Chiaraviglio]

Additional web page on which to reproduce:

http://news.yahoo.com/comics/uclickcomics/20051218/cx_db_uc/db20051218;_ylt=ApCFIiEg2aY2sBTWKpEgJgPV.i8C;_ylu=X3oDMTBiMW04NW9mBHNlYwMlJVRPUCUl

I did this when attempting to print the first sheet of this page from within Print Preview, with the page being one of multiple tabs, with the printer being a color printer.  (Not sure if any of this makes a difference, but just in case.)  Worked on 1st attempt; hung with no crash on 2nd attempt; crashed on next 3 attempts, of which one (maybe 2) managed to get something to the printer.

I can't find a Talkback ID for any of this right now because the server module that looks for these is not working (keeps giving an internal server error).

Comment 12

[Adam Guthrie]

Attachment: stacktrace

Comment 13

[Bernd]

we (I) need a reduced testcase here.

Comment 14

[Bernd]

Attachment: testcase

not really reduced, but reproducible, so it seems we have this since FF1.0?

Comment 15

[Bernd]

             tblO 034CBF34 d=5970,14421 me=5040 status=0x11
             block 034DE7B0 d=5970,14421 me=5040 status=0x13
            block 034CBB94 d=5970,284 me=1111 status=0x3 <<< where is all the height gone?
           cell 034CBB34 d=6000,300 me=1155 status=0x3

Comment 16

[Bernd]

Bug 292124 will make the crash with the testcase go away. However the underlaying problem does not vanish. The thing here seems to me like:

The table gets a constrained availHeight. It then does not fit but split and creates nextinflows. We might hit the end of
ReflowBlockFrame

      else {
          // Push the line that didn't fit and any lines that follow it
          // to our next-in-flow.
          UndoSplitPlaceholders(aState, lastPlaceholder);
          PushLines(aState, aLine.prev());
          aState.mReflowStatus = NS_FRAME_NOT_COMPLETE;
        }
      }

If this happens the height gets reduced as shown in the reflow above, My feeling is that we do not completely rewind the nextinflow stuff that the inner frames already did and on the next page we pull the wrong overflow frames and crash.

I am not certain that fixing the table splitting is good enough.

This raises for me the key question:
1. if a frame can split somehow, but will not fit anyway, should it split or not?

What I have in mind is atable rowgroup, where the first cell (first row) has a large nonsplittable content, so it can't be splitted, but the rows after it should go of course on the next page.

2. if 1. is answered with: do not split, shouldn't we assert.
Boris, Robert any insight on this?

Comment 17

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

(In reply to comment #16)
>       else {
>           // Push the line that didn't fit and any lines that follow it
>           // to our next-in-flow.
>           UndoSplitPlaceholders(aState, lastPlaceholder);
>           PushLines(aState, aLine.prev());
>           aState.mReflowStatus = NS_FRAME_NOT_COMPLETE;
>         }
>       }
> 
> If this happens the height gets reduced as shown in the reflow above, My
> feeling is that we do not completely rewind the nextinflow stuff that the
> inner frames already did and on the next page we pull the wrong overflow
> frames and crash.

I kinda got lost here. So you're saying there's a block problem here independent of tables?

> This raises for me the key question:
> 1. if a frame can split somehow, but will not fit anyway, should it split or
> not?
> 
> What I have in mind is atable rowgroup, where the first cell (first row) has a
> large nonsplittable content, so it can't be splitted, but the rows after it
> should go of course on the next page.

In general it should split. The first part, which doesn't fit, will be placed and truncated. The rest will move to the next column or page.

Comment 18

[Bernd]

>I kinda got lost here. So you're saying there's a block problem here
independent of tables?

I am not sure that its a block problem, but more a structural problem (aka my lack of understanding):

assume we have

<A>
 <B>
 <C>
 <D>
</A>

after lets assume that we need to split  <C> as H_b + H_c >  h_a_available 

<P1>
 <A>
  <B>
  <C1>  (C did create a next inflow C2, who has the reminder)
  at this place h_B + h_C1 are still larger than the available height for h_a.
  so it would be sensable to break before <C1> as h_b < h_a_available
  so we need to push <C1> too, we should then undo the splitting and push not              
  not only <C1>  but <C> and remove <C2> from the overflow list.

so at the end we have

<P>
 <A>
  <B>
 </A>
</P1>
<P2>
 <A2>
  <C1>
 </A2>
</P2>
<P3>
 <A3>
  <C2>
  <D>
 </A3>
</P3>

And my feeling is that the unwinding described above might have issues.

Comment 19

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

(In reply to comment #18)
That's exactly what our code is supposed to do. Except for one thing: we don't undo the split immediately. 'A' will just push both C1 and C2 to its overflow list; when C1 eventually gets reflowed and fits, then it will suck content from its next-in-flow(s) including C2 as long as that content fits*. If all that content fits, then C1 returns status 'complete' and its parent will delete its empty next in flow frames.

* E.g. for blocks:
http://lxr.mozilla.org/mozilla/source/layout/generic/nsBlockFrame.cpp#2411

Comment 20

[Bernd]

I am not sure that learning pagination with a crash bug is the way to do it, but here it goes.

Starting again from the reflow log  
>              tblO 034CBF34 d=5970,14421 me=5040 status=0x11
>             block 034DE7B0 d=5970,14421 me=5040 status=0x13
>            block 034CBB94 d=5970,284 me=1111 status=0x3 <<< where is all the height gone?
>           cell 034CBB34 d=6000,300 me=1155 status=0x3

The table has created a continuation of the row group (lets name it  C2) and keeps C1 as before. The block sees that it could probably layout the table on a next page, and pushes it to the next page. 

On the next page  we reflow the table and it has C1 and pulls C2 from the overflow list. Now it reflows C1 which will fit as it did fit allready on the first page only the addition of the caption made the outer table frame exceeding the available height. At the end of the rowgroup reflow, the row group will be marked complete (this might be allready the error) and the nextinflow will be deleted. This deletes C2, the table has reference to C2 in the rowgroups array and will try to reflow it at the next step, reflowing deleted frames dies as usually in the style system.

Comment 21

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

I think what should happen is that C1's reflow should pull all rows out of C2, leaving C2 empty. Then C1 returns saying it's complete. Its parent (nsTableFrame in this case) should then delete C2, removing C2 from its child list and not try to reflow C2 again.

Comment 22

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

So I think the problem is simply that when nsTableFrame::ReflowChildren calls nsContainerFrame::ReflowChild on C1, which empties out C2 and then deletes it
http://lxr.mozilla.org/mozilla/source/layout/generic/nsContainerFrame.cpp#890
we also need to remove C2 from nsTableFrame::ReflowChildren's rowgroup array.

Comment 23

[Bernd]

>nsTableFrame::ReflowChildren calls nsContainerFrame::ReflowChild on C1, which empties out C2

This is not exactly what is happening. Troy allready in 1999 found that this is to much bloat and implemented http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/tables&command=DIFF_FRAMESET&file=nsTableRowGroupFrame.cpp&rev2=3.134&rev1=3.133

in bug 82401 http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsTableRowGroupFrame.cpp&branch=3.349&root=/cvsroot&subdir=mozilla/layout/tables&command=DIFF_FRAMESET&rev1=3.251&rev2=3.252
Chris removed that pulling at all.

I am very  tempted to follow troys path 

Index: nsTableRowGroupFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/tables/nsTableRowGroupFrame.cpp,v
retrieving revision 3.349
diff -u -8 -p -r3.349 nsTableRowGroupFrame.cpp
--- nsTableRowGroupFrame.cpp	20 Nov 2005 22:05:15 -0000	3.349
+++ nsTableRowGroupFrame.cpp	13 Jan 2006 18:46:06 -0000
@@ -1269,16 +1271,19 @@ nsTableRowGroupFrame::Reflow(nsPresConte
       // Nope, find a place to split the row group 
       PRBool specialReflow = (PRBool)aReflowState.mFlags.mSpecialHeightReflow;
       ((nsHTMLReflowState::ReflowStateFlags&)aReflowState.mFlags).mSpecialHeightReflow = PR_FALSE;
 
       SplitRowGroup(aPresContext, aDesiredSize, aReflowState, tableFrame, aStatus);
 
       ((nsHTMLReflowState::ReflowStateFlags&)aReflowState.mFlags).mSpecialHeightReflow = specialReflow;
     }
+    if (mNextInFlow) {
+      aStatus = NS_FRAME_NOT_COMPLETE;
+    }
   }
   SetHasStyleHeight((NS_UNCONSTRAINEDSIZE != aReflowState.mComputedHeight) &&
                     (aReflowState.mComputedHeight > 0)); 
   
   if (aReflowState.mFlags.mSpecialHeightReflow) {
     SetNeedSpecialReflow(PR_FALSE);
   }

and to add a addtional protection layer that avoids the reference to allready deleted nextinflows in the rowgroup array.

Comment 24

[Bernd]

Just to make sure that my point of view is correctly visible:
<grin>this is to much bloat and implemented</grin>

and I am sorry that I did not find when 

// XXX Check aStatus to see if the frame is complete...   

this line from troys checkin got removed.

Comment 25

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

(In reply to comment #23)
> >nsTableFrame::ReflowChildren calls nsContainerFrame::ReflowChild on C1,
> >which empties out C2
> 
> This is not exactly what is happening. Troy allready in 1999 found that this
> is to much bloat and implemented
> http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/tables&command=DIFF_FRAMESET&file=nsTableRowGroupFrame.cpp&rev2=3.134&rev1=3.133

Those are in incremental reflow ... are we doing IRs here?

Anyway, those are not correct. The frame is complete when all its content fits, even if there is still a next in flow frame. In this incremental reflow situation, the frame can check that it is complete by checking that a) its list of overflowing children is empty and b) all next-in-flows have no children.

Comment 26

[Bernd]

Attachment: patch

Do exactly what Chris told to do allready in 2001...

Comment 27

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

I think I'd prefer a safer patch that always recreates the row group list if kidFrame had any next-in-flow before we reflowed it. comparing the pointers this way doesn't feel good to me because the next-in-flow might just possibly have been destroyed and then recreated at the same address.

Also, do you want to file a followup bug about removing the bogus status-setting like
http://lxr.mozilla.org/seamonkey/source/layout/tables/nsTableRowGroupFrame.cpp#1486
?

Comment 28

[Bernd]

Attachment: reorder more frequently

Comment 29

[Bernd]

I filed bug 324683 as the followup.

Comment 30

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

Comment on attachment 209600 [details] [diff] [review]
reorder more frequently

That's OK but it would be slightly nicer if instead of keeping kidNextInFlow, you had a PRBool hadNextInFlow = kidFrame->GetNextInFlow() != nsnull; ... that wy you don't have to keep around a dangling pointer.

Comment 31

[Bernd]

fix checked in

Comment 32

[Martijn Wargers (dead)]

Is this patch something that can be considered for the 1.8.1 branch?
It doesn't seem to have caused any regressions and it fixes this crash and the crash in bug 336427 (which is supposed to be a topcrash bug).

Comment 33

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

Comment on attachment 209600 [details] [diff] [review]
reorder more frequently

a=dbaron on behalf of drivers.  Please land on MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have.

Comment 34

[Bernd]

Attachment: patch against MOZILLA_1_8_BRANCH

Comment 35

[Bernd]

fixed on 1.8.1

Comment 36

[Carsten Book [:Tomcat]]

verified on Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b1) Gecko/20060813 BonEcho/2.0b1

Can anyone verify this on Linux ?

Comment 37

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060803 BonEcho/2.0b1. 

I no longer crash when print previewing the site in the URL of this bug. Thanks for the help, Tomcat.

Comment 38

[Daniel Veditz [:dveditz]]

This may or may not be part of the patch for bug 346980 (see bug 346980 comment 7). Nominating for 1.8.0.x separately to make sure it gets covered.

Comment 39

[Bernd]

http://tinderbox.mozilla.org/bonsai/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/tables&command=DIFF&root=/cvsroot&file=nsTableFrame.cpp&rev1=3.616.12.3&rev2=3.616.12.4#31

Comment 40

[alice nodelman [:alice] [:anode]]

when printing https://bugzilla.mozilla.org/attachment.cgi?id=206869&action=view the browser should not crash.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060829 Firefox/1.5.0.7pre

verified 1.8.0.7

Comment 41

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 42

[Bernd]

This got already checked in into 1.8.0.7 (http://lxr.mozilla.org/mozilla1.8.0/source/layout/tables/nsTableFrame.cpp#3228) no need to block for 1.8.0.9