392435 - ::first-line and ::first-letter - font-size and font-weight is propagated to all lines of affected block.

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[philippe (part-time)]

Attachment: test case

Testcase: a paragraph <p> with p::first-letter {font-size:200%;}, a div with div::first-line {font-weight:bold;}. 
The whole block uses the property declared on the :first-line or ::first-letter

Also happens with font-variant:small-caps, probably other text properties.

regressed between:
20070815_1129_firefox-3.0a8pre.en-US.mac (works)
20070815_1159_firefox-3.0a8pre.en-US.mac (fails)

--> bug 385270
Note:
If one switches tabs, and wait a few moments, then switch back to the tab with the testcase, the styling is applied correctly in case of the first-letter, but the line-wrapping is not correct.

Comment 1

[Daniel Holbert [:dholbert]]

Here's a reduced testcase:

test:        http://people.mozilla.com/~dholbert/tests/392435/392435-1.html
reference:   http://people.mozilla.com/~dholbert/tests/392435/392435-1-ref.html
(style file: http://people.mozilla.com/~dholbert/tests/392435/392435-style.css )

Will post as reftest patch at some point.

Comment 2

[Daniel Holbert [:dholbert]]

Also, it looks like thie affects all font-* properties, not just font-weight and font-style.
(I've tried font-weight, font-style, font-size, font-variant, and font-family)

Other attributes like color, background, and text-decoration don't seem to be affected.

Comment 3

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

Attachment: fix

This patch fixes it, but I need to think about whether it's the right patch.

A better way might be to just test whether the textruns are equal and the style contexts are equal.

Comment 5

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

It's supposed to work like this:
-- A nsFirstLetterFrame wraps the entire text (nsTextFrame)
-- the nsTextFrame reflows, creates a textrun for the entire text, but ends its reflow after the first letter
-- the nsFirstLetterFrame creates a continuation for the nsTextFrame; the continuation has the same style context as the first textframe, so it takes a reference to the first textframe's text run
-- the nsFirstLetterFrame puts that continuation on its overflow list
-- the block or containing inline creates a continuation for the nsFirstLetterFrame, and reflows it
-- The continuation nsFirstLetterFrame does a DrainOverflowFrames which pulls the textframe from the first nsFirstLetterFrame's overflow list
-- nsFirstLetterFrame::DrainOverflowFrames does a SetStyleContext on the pulled child to give it non-first-letter style
-- That calls nsTextFrame::DidSetStyleContext which clears the frame's textrun
-- Then we reflow the continuation textframe, which recreates the textrun
-- Recreating the textrun notices that the style context, and indeed font style, changes between the first text frame and its continuation and starts a new textrun for the continuation

Some step along here is broken, not sure which one yet.

Comment 6

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

Attachment: fix

The problem is that ClearTextRun has a precondition that I'd basically forgotten about: it does nothing if the frame is not one of the "textrun owners" --- the frames that are the heads of next-in-flow-chains associated with the textrun. In the cases here we're calling DidSetStyleContext() on frames that aren't owners, so they don't clear out their text runs and we use stale bad ones.

Removing that precondition allows ClearTextRun to work no matter which frame you call it on. This makes a lot more sense. I think I originally set it up with the owner check because I was concerned about O(N^2) performance tearing down a list of textframe continuations, but this isn't actually a problem since the first ClearTextRun to a frame in the chain will null out all the mTextRun pointers (which is O(N), but that's unavoidable) and all subsequent ClearTextRuns will exit immediately since mTextRun is null.

Comment 7

[philippe (part-time)]

So. I had a crash with this patch (OS X 10.4.10 ppc).
A very simple test file, p::first-letter {font-size:2em;}
Leaving the page by either closing the tab or using the back button would hang and then crash the browser:

0   libthebes.dylib          	0x062956c0 gfxTextRun::~gfxTextRun [in-charge deleting]() + 120 (gfxFont.h:419)
1   libthebes.dylib          	0x062956bc gfxTextRun::~gfxTextRun [in-charge deleting]() + 116 (gfxFont.cpp:735)
2   libgklayout.dylib        	0x07ba1b54 nsContinuingTextFrame::Destroy() + 56 (nsTextFrameThebes.cpp:3359)
3   libgklayout.dylib        	0x07b67754 nsLineBox::DeleteLineList(nsPresContext*, nsLineList&) + 124 (nsLineBox.cpp:361)

With a debug Minefield build, I see this:
firefox-bin(11650,0xa000ed88) malloc: ***  Deallocation of a pointer not malloced: 0x26c6600; This could be a double free(), or free() called with the middle of an allocated block; Try setting environment variable MallocHelp to see tools to help debug
(didn't try the Malloc suggestion, don't understand any of it...:)
Full crash log and testcase coming.

Live URL: http://www.yomiuri.co.jp/dy/

Comment 8

[philippe (part-time)]

Attachment: crash log, comment 7

With Minefield debug build; an opt Camino build gives the same stack.

Comment 9

[philippe (part-time)]

Attachment: test case crashes patched browser

close tab or use back button and this page crashes.
loading a page over it does not cause problems

Comment 10

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

Attachment: updated fix

Thanks for catching that. We need to call ClearTextRun before unlinking the continuation chain.

Comment 11

[philippe (part-time)]

(In reply to comment #10)
> Created an attachment (id=279396) [details]
> updated fix

Yup, this one does what it says. No problems anymore.

Comment 12

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

Comment on attachment 279396 [details] [diff] [review]
updated fix

>? layout/reftests/first-letter/available-width-1.html
>? layout/reftests/first-letter/available-width-ref.html
>? layout/reftests/first-letter/cluster-1-ref.html
>? layout/reftests/first-letter/cluster-1.html
>? layout/reftests/text/white-space-1.html
>? layout/reftests/text/white-space-1d.html

Are these tests that you should be checking in?

a1.9=dbaron

Comment 13

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

No, I will check in a test based on comment #1

Comment 14

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

checked in

Comment 15

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

Checked in testcase

Comment 16

[Jean-Marc Desperrier]

I confirm it fixes the problem I had on liberation.fr in bug 394146 

Comment 17

[philippe (part-time)]

verified with
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a8pre) Gecko/2007090717 Minefield/3.0a8pre