Last Comment Bug 638937 - Crash when mixing columns, first-letter & float
: Crash when mixing columns, first-letter & float
Status: RESOLVED FIXED
[inbound]
: crash, testcase
Product: Core
Classification: Components
Component: Layout: Floats (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla13
Assigned To: Mats Palmgren (:mats)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 724414
  Show dependency treegraph
 
Reported: 2011-03-04 13:51 PST by neil@parkwaycc.co.uk
Modified: 2012-02-22 10:38 PST (History)
5 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case (tries to crash after 10 seconds) (4.93 KB, text/html)
2011-03-04 13:51 PST, neil@parkwaycc.co.uk
no flags Details
testcase 2 (210 bytes, text/html)
2011-07-08 21:38 PDT, Jesse Ruderman
no flags Details
frame dump (96.96 KB, text/html)
2012-02-17 13:20 PST, Mats Palmgren (:mats)
no flags Details
fix + tests (9.29 KB, patch)
2012-02-17 14:13 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
fix + tests (16.28 KB, patch)
2012-02-18 06:52 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2011-03-04 13:51:27 PST
Created attachment 516992 [details]
Test case (tries to crash after 10 seconds)

If you style a first letter with a float style, and the paragraph starts at the bottom of a column, there is no room for the letter, and it "floats" to the next column. However the rest of the paragraph is then displayed in the bottom line! The browser can subsequently abort; I found that altering the column rules was most likely to trigger the abort.

###!!! ASSERTION: aForFrame not found in block, someone lied to us: 'isValid', file nsTextFrameThebes.cpp, line 1175
###!!! ABORT: comparing iterators over different lists: 'mListLink == aOther.mListLink', file nsLineBox.h, line 722
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-03-07 09:54:08 PST
roc, this seems like your sort of thing...
Comment 2 Jesse Ruderman 2011-07-08 21:38:46 PDT
Created attachment 544947 [details]
testcase 2
Comment 3 Mats Palmgren (:mats) 2012-02-17 13:20:06 PST
Created attachment 598348 [details]
frame dump

(scroll about 60% down to see the interesting frames)

We're trying to build a text run for the text frame "N" (red) which
is the child frame of a float first-letter.
nsFirstLetterFrame::CanContinueTextRun() returns 'true' so 
FindLineContainer() walks up the ancestor to 0x7fffdcf083b8 (green),
which isn't the line container for the text frame.

This should be easily fixed by returning 'false' in
CanContinueTextRun() for a floating first-letter.

Still, there's something odd about the frame tree --
why has the float been pushed into a different block from the
text is supposed to be the first letter for?
(namely 0x7fffdcd08f68 (yellow))

That seems wrong, but maybe we're not done reflowing yet?
Comment 4 Mats Palmgren (:mats) 2012-02-17 14:13:12 PST
Created attachment 598372 [details] [diff] [review]
fix + tests

Try results pending:
https://tbpl.mozilla.org/?tree=Try&rev=6f52d96cba0f
Comment 5 Mats Palmgren (:mats) 2012-02-17 16:35:35 PST
Hmm, it appears text-transform:capitalize assumes that the start of a text run
is also the start of a word?
https://tbpl.mozilla.org/?tree=Try&rev=6f52d96cba0f
(Reftest layout/reftests/bugs/431341-1.html)
Comment 6 Mats Palmgren (:mats) 2012-02-17 18:02:27 PST
Looking at the text-run / line-breaker code it looks hard to get things right
when a word is broken into two text-runs.  I'll see if I can make BuildTextRuns
deal instead.
Comment 7 Mats Palmgren (:mats) 2012-02-18 06:52:31 PST
Created attachment 598531 [details] [diff] [review]
fix + tests

For floating first-letter text, I think the correct line-container to use
for the line iterators here is the parent of the placeholder frame.
We've just assumed that the floating first-letter parent is the same
as the placeholder parent.  I think that's actually a bug when it's not,
but let's fix this crash first and then file a second bug on that.

The last two hunks fixes compile warnings - an unused variable and a
signed/unsigned int comparison.

The assert(18) are all "Computed overflow area must contain frame bounds"
due to the "margin: -562949953421311em;" in the test causing nscoord
overflows.  I'm not worried about that.
Comment 8 Mats Palmgren (:mats) 2012-02-18 06:54:03 PST
The Try run:
https://tbpl.mozilla.org/?tree=Try&rev=78f2975c3fa5
Comment 9 Mats Palmgren (:mats) 2012-02-21 16:07:47 PST
Filed bug 729352 to follow-up on the misplaced float first-letter frame.
Comment 11 Ed Morley [:emorley] 2012-02-22 10:38:35 PST
https://hg.mozilla.org/mozilla-central/rev/0edd2447ab9b

Note You need to log in before you can comment on or make changes to this bug.