Biju Reporter
	Description • 16 years ago

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) 
Gecko/20080915032512 Minefield/3.1b1pre

In correct line wrap on text "Festival of Light,"
at http://en.wikipedia.org/wiki/Diwali


1. Go to http://en.wikipedia.org/wiki/Diwali
2. start reducing the page width 
3. at some point you see "Festival" 
   as the only word in the line

   See attached line_wrap.png

Expected:
with that window size we have enough room to keep "of" on same line


PS: If you find difficult to recreate problem, try adjusting width 
when the words "Diwali, or Deepavali," are the only words on the first line.

	Biju Reporter
	Comment 1 • 16 years ago

instead of http://en.wikipedia.org/wiki/Diwali 
you can use Diwali.htm for testing...

	Jesse Ruderman
	Comment 2 • 16 years ago

I can reproduce on Mac using Firefox trunk.  I used Lithium to make a reduced testcase.

	David Baron :dbaron:
	Comment 3 • 16 years ago

Would you mind attaching said reduced testcase?

	Jesse Ruderman
	Comment 4 • 16 years ago

Dumps "WRAP-FAIL" when it notices the "qqq" span move *up* in response to the body becoming *wider*.

	Jesse Ruderman
	Comment 7 • 16 years ago

If you load "reftest: testcase" and resize the window, the last word will jump around.

	Nochum Sossonko [:Natch]
	Comment 8 • 16 years ago

Regression: 20080811053724 - 20080812045727
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008%2F11%2F08&enddate=2008%2F12%2F08
-->Bug 441259 (I think)

	Daniel Holbert [:dholbert]
	Comment 9 • 16 years ago

Using a debug build, I get these assertions on loading the "reftest: testcase" (and when loading "Diwali.htm" at a window-width that reproduces the bug)

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /scratch/work/builds/central/central.08-10-01.16-24/mozilla/gfx/thebes/src/gfxSkipChars.cpp, line 92
###!!! ASSERTION: Text run does not map enough text for our reflow: 'gfxSkipCharsIterator(iter).ConvertOriginalToSkipped(offset + length) <= mTextRun->GetLength()', file /scratch/work/builds/central/central.08-10-01.16-24/mozilla/layout/generic/nsTextFrameThebes.cpp, line 5768

	Daniel Holbert [:dholbert]
	Comment 10 • 16 years ago

(In reply to comment #8)
> -->Bug 441259 (I think)

I don't think so -- the only functional change of that bug is at nsLineLayout.cpp:879, inside of a clause that only gets run if we have placeholder frames.  The reftest here doesn't use positioned / floated content, so there are no placeholders.  (I tried putting a breakpoint there just in case, and it never got hit)

/me looks for other possibly-guilty checkins...

	Daniel Holbert [:dholbert]
	Comment 11 • 16 years ago

This bug goes away if I run "hg backout f07d3fa9b667" (the checkin for bug 230555). ==> Looks like a regression from that bug.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 12 • 16 years ago

During reflow we get into a state where we've reflowed the second line but it overflows and we need to reflow it again with a break forced before the third "aa". The frame tree looks like this:

            line 0xe75c43c: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x4120] {0,0,936,840} <
              Text(0)@0xe75c310[0,3,F]  next=0xe75c758 next-continuation=0xe75c758 {0,0,936,840} [state=91600000] [content=0x8797240] sc=0xe75c194 pst=:-moz-non-element<
                "aa "
              >
            >
            line 0xe75c79c: count=2 state=inline,dirty,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8021] {0,840,936,840} <
              Text(0)@0xe75c758[3,4,T]  next=0xe75c374 prev-continuation=0xe75c310 {0,840,1872,840} [state=00200405] [content=0x8797240] sc=0xe75c194 pst=:-moz-non-element<
                "aa a"
              >
              Inline(b)(1)@0xe75c374 next=0xe75c870 next-continuation=0xe75c870 {468,1680,468,840} [content=0x878b3a0] [sc=0xe75c1e4]<
                Text(0)@0xe75c3fc[0,2,F]  next-continuation=0xe75c8d0 {0,0,468,840} [state=81000000] [content=0x878b3d0] sc=0xe75c3ac pst=:-moz-non-element<
                  "a "
                >
              >
              Overflow-list<
                Text(0)@0xe75c8d0[2,1,T]  prev-continuation=0xe75c3fc {0,0,0,0} [state=00000406] [content=0x878b3d0] sc=0xe75c3ac pst=:-moz-non-element<
                  "a"
                >
              >
            >

We're going to force a break at offset 6 in textnode 0x8797240. But when we come to reflow the textframe 0xe75c758 again, for some reason its mTextRun is null (it must have just been cleared!) and we rebuild text runs for 0xe75c758. While we're doing that, the textrun for 0xe75c3fc is also built, but it's built incorrectly because we don't search the overflow-list for inlines. So things go haywire from there.

We can extend the scanner to search the overflow-lists of inlines. However, we shouldn't have lost the textrun during repeated reflows of the same line. I need to look into that.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 13 • 16 years ago

Okay, here are two fixes.

Fix #1 is in nsContinuingTextFrame::Destroy. We don't need to always clear the text run. Often we're just deleting unused continuations because the prev-continuation is taking up the text. If the prev-continuation has the same style as the frame we're destroying, we don't need to mess with the textrun.

Fix #2 is to have the text scanner look into the overflow-lists of inlines. This should not often be needed, since inlines should only have frames in their overflow-lists temporarily during a single reflow, and all textruns should have been created before those frames were moved to the overflow-list. But it can happen in first-letter/first-line situations, which require aggressive reconstruction of textruns due to styles on text changing during reflow.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 14 • 16 years ago

I verified that fix #2 fixes this bug without fix #1; fix #1 makes the warning go away.

	David Baron :dbaron:
	Comment 15 • 16 years ago

Initially landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/1f5d2c249f86
http://hg.mozilla.org/mozilla-central/rev/026147c91538
and then backed out due to crashes caused by bug 430332:
http://hg.mozilla.org/mozilla-central/rev/9eaf91dad68c
and then relanded on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/df41ce61d237
and thus fixed on both mozilla-central and releases/mozilla-1.9.1.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 16 • 16 years ago

The change to nsContinuingTextFrame::Destroy wasn't landed because it wasn't in the roc-2.bundle, I intended to split it out into roc-3.bundle but failed to put the right patch in roc-3.bundle. I'll reland that myself in a separate bug, bug 467150.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 17 • 16 years ago

Backed out to try to fix random Windows orange.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 18 • 16 years ago

Oops, it was the patch in the other bug that was actually backed out. Sorry for the spam.

	Daniel Holbert [:dholbert]
	Comment 19 • 16 years ago

Checked into CVS trunk to fix bug 431260.
=====================
Checking in layout/generic/nsTextFrameThebes.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.187; previous revision: 3.186
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/455826-1-ref.html,v
done
Checking in layout/reftests/bugs/455826-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/455826-1-ref.html,v  <--  455826-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/455826-1.html,v
done
Checking in layout/reftests/bugs/455826-1.html;
/cvsroot/mozilla/layout/reftests/bugs/455826-1.html,v  <--  455826-1.html
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.471; previous revision: 1.470
done

	Al Billings [:abillings - ex-MoCo]
	Comment 20 • 16 years ago

Maybe I'm being dumb here but using the attached testcases and sample page, I'm not seeing any rendering difference between 1.9.0.7 and the 1.9.0.8 nightly on my OS X box.

	Daniel Holbert [:dholbert]
	Comment 21 • 16 years ago

You're right, this bug wasn't reproducible on 190x because it broke in trunk after 190x branched off (see comment 11 for regression).  However, i think the patch is fairly benign, and it's required for the existing (& more-important) patch for bug 431260 to apply.  Maybe 'fixed1.9.0.8' isn't an appropriate keyword in this situation - i'm not sure.

	Al Billings [:abillings - ex-MoCo]
	Comment 22 • 16 years ago

Verified for 1.9.0.9 because I've verified bug 431260 is fixed in 1.9.0.9.

	Daniel Holbert [:dholbert]
	Comment 23 • 16 years ago

Marking bug 467150 as depending on this bug, since it improves a chunk of this bug's posted patch.