285608 - hangs when page with float printed

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[JaFojtik]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050225 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050225 Firefox/1.0.1

Firefox crashes when this page is printed.

Reproducible: Always

Steps to Reproduce:
1. Open a page
2. Print it


Actual Results:  
Firefox hangs - no response

Comment 1

[Adam Hauner]

Same with 2005031005/Seamonkey-trunk/WXP in Print Preview, marking NEW.
-> Core / Printing

Comment 2

[Wayne Mery (:wsmwk)]

fails also Deer park alpha 2 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.8b4) Gecko/20050715 Firefox/1.0+

Comment 3

[Marius Strumyla]

Attachment: Reduced testcase

I reduced the URL to three floats.

Comment 4

[Wayne Mery (:wsmwk)]

fails Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060612 Minefield/3.0a1

are bug 331039 and bug 255982 dups?

Comment 5

[Bernd]

Mats, is this one for you? 

Comment 6

[Wladimir Palant]

Bug 354713 is probably a dupe of this one.

Comment 7

[Joshua Kinard]

Anyone tried this on Firefox/Linux or MacOS X?  I submitted Bug #354713, and just looking at the testcase from the Reuters site where I saw it.  It does have a few floats in it, and I was able to confirm the Reuters site will fail across Windows XP SP2, Linux (Gentoo), and MacOS X when trying to print to a real printer or file.  I'm curious to know if this my bug is a dupe or not.  I would imagine with as big of a site as Reuters, and with their poll, the issue has to have been run into way more often now.

Comment 10

[Blu3]

Any work towards fixing this bug?  There are a lot of workout/nutrition plans on this site I discovered this bug on that I'd like to print without the workaround hassle.

Comment 11

[Leon Sha]

Attachment: Patch

Comment 12

[Leon Sha]

After apply this patch, this bug and bug 360224, bug 255982, bug 382573, bug 376218, bug 331039 will not happen. There is a problem that the text beyond the page will not show up in preview or get printed. So I'd rather call this patch as a workaround. But it is much better than hang.

Comment 13

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

LINE_REFLOW_TRUNCATED should not be set for lines that are at the top of the page. See:

  else if (NS_FRAME_IS_TRUNCATED(frameReflowStatus)) {
    // if the frame is a placeholder and was complete but truncated (and not at the top
    // of page), the entire line will be pushed to give it another chance to not truncate.
    if (nsGkAtoms::placeholderFrame == aFrame->GetType()) {
      *aLineReflowStatus = LINE_REFLOW_TRUNCATED;
    }
 
and nsHTMLReflowState::SetTruncated.

So in theory the only part of your patch that should be needed is this one:
-      } else {
+      } else if (!aState.IsAdjacentWithTop()) {
         // There's nowhere to retry placing the line. Just treat it as if
         // we placed the float but it was truncated so we need this line
         // to go to the next page/column.
         lineReflowStatus = LINE_REFLOW_TRUNCATED;

Comment 14

[Leon Sha]

> So in theory the only part of your patch that should be needed is this one:
> -      } else {
> +      } else if (!aState.IsAdjacentWithTop()) {
>          // There's nowhere to retry placing the line. Just treat it as if
>          // we placed the float but it was truncated so we need this line
>          // to go to the next page/column.
>          lineReflowStatus = LINE_REFLOW_TRUNCATED;
> 
Actually this is the only place that I do not confirm.
I just add it according to the comments of  PushTruncatedPlaceholderLine.

The following one prevent the hang for this bug and other wiki related bugs.

>-      if (LINE_REFLOW_TRUNCATED == lineReflowStatus) {
>+      if (LINE_REFLOW_TRUNCATED == lineReflowStatus && >!aState.IsAdjacentWithTop()) {
>         // Push the line with the truncated float 
>         PushTruncatedPlaceholderLine(aState, aLine, lastPlaceholder, >*aKeepReflowGoing);

And the following one prevent the hang for bug 255982.
>-    else { 
>+    else if (!wasAdjacentWIthTop) { 
>       // At least one float is truncated, so fix up any placeholders that got >split and 
>       // push the line. XXX It may be better to put the float on the next >line, but this 
>       // is not common enough to justify the complexity. Or maybe it is now...
 

Comment 15

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

Then I need you to explain how LINE_REFLOW_TRUNCATED is being returned here with !aState.IsAdjacentWithTop().

Comment 16

[Leon Sha]

I have check the aState.IsAdjacentWithTop() here.
It's true.
The result is not confirm with the log mentioned.

  else if (NS_FRAME_IS_TRUNCATED(frameReflowStatus)) {
    // if the frame is a placeholder and was complete but truncated (and not at the top
    // of page), the entire line will be pushed to give it another chance to not truncate.
    if (nsGkAtoms::placeholderFrame == aFrame->GetType()) {
      *aLineReflowStatus = LINE_REFLOW_TRUNCATED;

Comment 17

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

We should assert aState.IsAdjacentWithTop there. And I'd like someone (i.e. you :-) ) to figure out why NS_FRAME_IS_TRUNCATED(frameReflowStatus) is true even though we're adjacent to the top. That seems like the real underlying bug.

Comment 18

[Leon Sha]

  [1] nsHTMLReflowState::SetTruncated(this = 0x803d2d0, aMetrics = STRUCT, aStatus = 0x803da24), line 2252 in "nsHTMLReflowState.cpp"
  [2] nsHTMLScrollFrame::Reflow(this = 0x9725558, aPresContext = 0x8d05750, aDesiredSize = STRUCT, aReflowState = STRUCT, aStatus = 0), line 805 in "nsGfxScrollFrame.cpp"
  [3] nsBlockReflowContext::ReflowBlock(this = 0x803d240, aSpace = STRUCT, aApplyTopMargin = 1, aPrevMargin = STRUCT, aClearance = 0, aIsAdjacentWithTop = 0, aComputedOffsets = STRUCT, aFrameRS = STRUCT, aFrameReflowStatus = 0), line 370 in "nsBlockReflowContext.cpp"
  [4] nsBlockFrame::ReflowFloat(this = 0x9bba210, aState = CLASS, aPlaceholder = 0x8c56e50, aFloatCache = 0x97f4c80, aReflowStatus = 0), line 5302 in "nsBlockFrame.cpp"
  [5] nsBlockReflowState::FlowAndPlaceFloat(this = 0x803e548, aFloatCache = 0x97f4c80, aIsLeftFloat = 0x803d674, aReflowStatus = 0, aForceFit = 0), line 738 in "nsBlockReflowState.cpp"
  [6] nsBlockReflowState::AddFloat(this = 0x803e548, aLineLayout = CLASS, aPlaceholder = 0x8c56e50, aInitialReflow = 0, aReflowStatus = 0), line 546 in "nsBlockReflowState.cpp"
  [7] nsLineLayout::AddFloat(this = 0x803db5c, aFrame = 0x8c56e50, aReflowStatus = 0), line 255 in "nsLineLayout.h"
  [8] nsLineLayout::ReflowFrame(this = 0x803db5c, aFrame = 0x8c56e50, aReflowStatus = 0, aMetrics = (nil), aPushedFrame = 0), line 923 in "nsLineLayout.cpp"
=>[9] nsBlockFrame::ReflowInlineFrame(this = 0x9bba210, aState = CLASS, aLineLayout = CLASS, aLine = CLASS, aFrame = 0x8c56e50, aLineReflowStatus = 0x803dad0), line 3434 in "nsBlockFrame.cpp"

This is the stack when frameReflowStatus is set to NS_FRAME_TRUNCATED.
At this time mFlags.mIsTopOfPage is false.
And I see the comments here.
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockReflowState.cpp#705
Here mY changed, which makes the IsAdjacentWithTop false.

Comment 19

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

Okay, I see. Then we need to find a way to make IsAdjacentWithTop return true as long as we are placing floats at the beginning of the first line, even if some of those floats have been placed and moved mY down.

Comment 20

[Leon Sha]

Roc, I don't know how to do this.
Reset mY before ReflowFloat?
Or pass a "WasAdjacentWithTop" to ReflowFloat?

(In reply to comment #19)
> Okay, I see. Then we need to find a way to make IsAdjacentWithTop return true
> as long as we are placing floats at the beginning of the first line, even if
> some of those floats have been placed and moved mY down.
> 

Comment 21

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

I think instead of IsAdjacentWithTop testing mY, it should test whether there is a non-empty line before the current line. Roughly what this does:
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#2578

But the easiest way to do it might be to have a flag to indicate whether a non-empty line has been placed. That flag could be updated where we currently update mY (but not if we're updating mY just because we are placing a float and moving to the next band).

Comment 22

[Leon Sha]

Roc, I am not familiar with that part of code.
Can you take over this bug?

Comment 23

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

ok

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(Is there any reason that the variable is called "wasAdjacentWIthTop" instead of "wasAdjacentWithTop"?)

Comment 25

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

ain't autcomplete great?

Comment 26

[Eli Friedman]

Taking; I think I've got a pretty good handle on this.

Comment 27

[Eli Friedman]

Attachment: Patch v1

This patch fixes a some of the hangs, including this bug, bug 323652, bug 255982, and both testcases in bug 331039.  It doesn't fix bug 350684 or bug 360224; however, as far as I can tell, those aren't float bugs; something odd is triggering in text frame reflow (even though suppressInitialBreak is getting passed to BreakAndMeasureText, it's saying that 0 characters fit).

This approach isn't especially clean, but it basically works, and it's relatively clear what's going on.  Any suggestions?

Comment 28

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

Comment on attachment 269750 [details] [diff] [review]
Patch v1

Looks good...

+ if (placed && (forceFit || !NS_FRAME_IS_TRUNCATED(aReflowStatus))) {

I think it would be clearer if you wrote this as
  if (forceFit || (placed && !NS_FRAME... 

+      if (IsAdjacentWithTop()) {
+        aReflowStatus = NS_INLINE_LINE_BREAK_BEFORE();

Need a comment here pointing out that forceFit is false, therefore aLineLayout.LineIsBreakable() must be true ... possibly with an asertion of
aLineLayout.LineIsBreakable().

Comment 29

[Eli Friedman]

Attachment: Final patch

Comment 30

[Eli Friedman]

Checked in.

Comment 31

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

      NS_WARN_IF_FALSE(NS_FRAME_IS_TRUNCATED(reflowStatus) && aForceFit,
                       "This situation currently leads to data not printing");

This fires when the frame is complete. Should we add "&& NS_FRAME_IS_NOT_COMPLETE(reflowStatus)"?

Comment 32

[Eli Friedman]

Attachment: Warning fix

Yeah, I messed up that warning...

Comment 38

[Giuliano Masseroni [:jooliaan]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; it; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7
Build Identifier: 2007091417

Firefox crashes when this page is printed: http://it.wikipedia.org/wiki/Sistema_di_numerazione_arabo

Reproducible: Always

Steps to Reproduce:
1. Open the page
2. Print it


Actual Results:  
Firefox hangs - no response

Comment 39

[Eli Friedman]

We know... it will be fixed in Firefox 3.

Comment 40

[Giuliano Masseroni [:jooliaan]]

Thanks for your answer Eli. 

Anyway I would inform you that the issue I described above is still presente in Gran paradiso a8: http://releases.mozilla.org/pub/mozilla.org/firefox/releases/granparadiso/alpha8/win32/en-US/Gran%20Paradiso%20Setup%20Alpha%208.exe

Moreover I would let you know i have filled bug 397528 referring to this specific topic related to wikipedia.

Comment 41

[Henry Hartley]

When I came across this problem, I tracked it to the CSS used by Wikipedia.  The problem is an interaction between 'overflow: hidden' and 'clear: right'.  In Wikipedia's style sheet, you will find these two class definitions.

div.thumb {
    margin-bottom: 0.5em;
    border-style: solid;
    border-color: White;
    width: auto;
    overflow: hidden;
}
div.tright {
    float: right;
    clear: right;
    border-width: 0.5em 0 0.8em 1.4em;
}

If either the overflow or clear properties are commented out in the css code,
the page prints (and previews).  With both properties present, Firefox hangs.  The reason so many pages do not exhibit this behavior is that many pages do not actually use both these classes.  If they are both used on a page, it will hang.  

The code to fix the bug has been written.  The fact that it isn't in any particular alpha release doesn't bother me.  It IS in the 3.0a9pre code so it WILL be in the 3.0 release (as long as there are no problems with it).  Just give it time.

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/