Crash in BuildFloaterList() [myway.com]

[GoogleCodeExporter]

Reported through Report-A-Bug: multiple relatively simple pages crash in
9.0.4 and 9.1-internal, all in BuildFloaterList(). The stack trace is

nsBlockFrame::BuildFloaterList()
nsBlockFrame::Reflow(nsIPresContext*,nsHTMLReflowMetrics&,const
nsHTMLReflowState&,unsigned int&)
nsBlockFrame::Reflow(nsIPresContext*,nsHTMLReflowMetrics&,const
nsHTMLReflowState&,unsigned int&)
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&,nsLineList_iterator,int*)
nsBlockFrame::ReflowLine(nsBlockReflowState&,nsLineList_iterator,int*,int)
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&)
nsBlockFrame::Reflow(nsIPresContext*,nsHTMLReflowMetrics&,const
nsHTMLReflowState&,unsigned int&)

This does not appear to occur in 9.0, so I am marking it as a regression.
Here is one offending URL (careful, this will crash Classilla):

http://apnews.myway.com//article/20100222/D9E1DA4G1.html

I suspect some of the 1.7/1.8 pullups triggered this. The problem occurs
during line iteration in BuildFloaterList(); the line->HasFloaters() is the
point of crash, apparently a bogus pointer.

Original issue reported on code.google.com by classi...@floodgap.com on 22 Feb 2010 at 11:33

[GoogleCodeExporter]

Upping to critical as I would like to ship 9.1 with a fix for this if at all
possible, since this is a good choice for Classilla users and it used to work.

Original comment by classi...@floodgap.com on 22 Feb 2010 at 11:34

• Added labels: Priority-Critical
• Removed labels: Priority-High

[GoogleCodeExporter]

Backing out a few obvious pullups didn't work.
Replacing ::ReflowLine() with 9.0's version didn't work.
Wolf-fencing by running a line iterator and marking the lines dirty to hit the 
bad
pointer implicates ReflowDirtyLines:

nsBlockFrame::Reflow(...) {
[...]
  NS_ASSERTION(NS_SUCCEEDED(rv), "setting up reflow failed");
  if (NS_FAILED(rv)) return rv;

  TestLines();

  // Now reflow...
  rv = ReflowDirtyLines(state);
  NS_ASSERTION(NS_SUCCEEDED(rv), "reflow dirty lines failed");
  if (NS_FAILED(rv)) return rv;

  TestLines(); // CRASH MOVES HERE
[...]
}

Original comment by classi...@floodgap.com on 23 Feb 2010 at 12:45

[GoogleCodeExporter]

We know this can't be M209694 (thank goodness) because that was applied to 9.0, 
so
this must be a problem with either M206516 (doubtful), M290297 (which includes 
part
of our pullups), M257612 (very probable), and/or any of the 1.7/1.8 pullups 
added to
9.0.4 (also very probable especially since many intersect with M257612).

Original comment by classi...@floodgap.com on 23 Feb 2010 at 12:52

[GoogleCodeExporter]

Further wolf-fencing:

nsBlockFrame::ReflowDirtyLines(...) {
[...]
      // Reflow the dirty line. If it's an incremental reflow, then force
      // it to invalidate the dirty area if necessary
      // bug 207477. I replaced doInvalidates because I'm lazy. -- CK
      PRBool forceInvalidate = incrementalReflow &&
!aState.GetFlag(BRS_DAMAGECONSTRAINED);
      TestLines();
      rv = ReflowLine(aState, line, &keepGoing, forceInvalidate); //
incrementalOverflow was in bug 207477
      TestLines(); // CRASH MOVES HERE
[...]
}

Even backing out M207477 didn't fix this (I left it backed out above). However,
testing aLine at the end of ReflowLine with ->MarkDirty() elicited no problem.

Original comment by classi...@floodgap.com on 23 Feb 2010 at 2:03

[GoogleCodeExporter]

The crash is wallpapered if, in nsBlockFrame::ReflowLine(), we either turn

if (aState.GetFlag(BRS_COMPUTEMAXWIDTH) && isBeginningLine) {

into

if (0 && aState.GetFlag(BRS_COMPUTEMAXWIDTH) && isBeginningLine) { // disabling 
it

or, within that code block, don't call ReflowInlineFrames() ... but this 
garbles the
layout, whereas the first blockout makes layout semi-ok. Either way, with these
changes, there is no crash, but this is not acceptable.

Original comment by classi...@floodgap.com on 23 Feb 2010 at 2:21

[GoogleCodeExporter]

M105520 is exonerated but we should test it after we're done here.

Original comment by classi...@floodgap.com on 23 Feb 2010 at 2:22

[GoogleCodeExporter]

In ::PlaceLine,

  // See if the line fit. If it doesn't we need to push it. Our first
  // line will always fit.
  if ((mLines.front() != aLine) && (newY > aState.mBottomEdge)) {
        //&& aState.mBottomEdge != NS_UNCONSTRAINEDSIZE) { // added by 1.7
    // Push this line and all of it's children and anything else that
    // follows to our next-in-flow
    NS_ASSERTION((aState.mCurrentLine == aLine), "oops");
    PushLines(aState, aLine.prev());

    // Stop reflow and whack the reflow status if reflow hasn't
    // already been stopped.
    if (*aKeepReflowGoing) {
      NS_ASSERTION(NS_FRAME_COMPLETE == aState.mReflowStatus, 
                   "lost reflow status");
      aState.mReflowStatus = NS_FRAME_NOT_COMPLETE;
      *aKeepReflowGoing = PR_FALSE;
    }
    return PR_TRUE;
  }
// return PR_FALSE;

  aState.mY = newY;

If PR_FALSE comes after newY, then the crash occurs.

Replacing (with tweaks to compile) nsBlockFrame, nsLineLayout and
nsBlockReflowContext from 9.0 did not fix the bug. I am absolutely beside 
myself.
Where did this come from??!

Original comment by classi...@floodgap.com on 23 Feb 2010 at 4:13

[GoogleCodeExporter]

Need to verify M186593 after this, as the changes I'm making are gutting this 
bug.

Original comment by classi...@floodgap.com on 23 Feb 2010 at 4:13

[GoogleCodeExporter]

An extensive diff between 9.0 and 9.0.4's layout yielded little if any clue, so 
I
have given up and reverted to 9.1-internal 20100215.

Disabling JavaScript on the affected page does fix the problem, even though the
problem is not JavaScript per se. The workaround is to allow JavaScript for
my.myway.com, but not apnews.myway.com, etc. The articles then show up, albeit
without navigation, but also without crashing.

Because there is now a workaround, I am downgrading this to High. It may not be
possible to fix this given Classilla's current layout engine without 
significantly
undoing large parts of code.

Original comment by classi...@floodgap.com on 25 Feb 2010 at 3:39

• Added labels: Priority-High
• Removed labels: Priority-Critical

[GoogleCodeExporter]

It occurs to me this could also be DOM.

Original comment by classi...@floodgap.com on 25 Feb 2010 at 6:16

[GoogleCodeExporter]

A modified version of https://bugzilla.mozilla.org/show_bug.cgi?id=282173 may 
allow us to finally solve this bug. It has no regressions, so if it compiles, 
it will probably work. However, it seems to have landed after bug 300030, so it 
may depend on reflow refactoring.

Original comment by classi...@floodgap.com on 25 Jun 2012 at 3:50