Bug (CVE-2011-0114) – Non-layout style change does not update nested first-letter

• Component: Layout and Rendering
• Status: RESOLVED FIXED

Updating of first letter style is based on needsLayout(). As a result, style changes that do no require a layout do not update the style of a first letter nested in an inline. See the attached test case. Note that <http://trac.webkit.org/projects/webkit/changeset/21742> fixed the non-nested case).

Created attachment 15430 [details]
Test case

[Simon Fraser (smfr)]

Created attachment 31268 [details]
Possible patch

Possible patch attached.

[Simon Fraser (smfr)]

Notes from irc conversation:

dhyatt: setStyle is top down
5:22pm dhyatt: so you get to the outermost block with first-letter on it
5:22pm dhyatt: and update its style to be green
5:22pm dhyatt: so then the cached first letter style is also green
5:22pm dhyatt: or would be
5:22pm dhyatt: it just inherits
5:22pm dhyatt: but the issue is when you make a pseudo style
5:22pm dhyatt: you pass in a parent
5:23pm dhyatt: and you are passing in the first line style of the <span>
5:23pm dhyatt: and he has not yet been properly updated
5:23pm smfr: ah
5:23pm dhyatt: so his color is black
5:23pm dhyatt: so the first letter style inherits from that
5:23pm dhyatt: later on the <span> is going to update
5:23pm dhyatt: to green
5:23pm dhyatt: but because he;'s a RenderInline
5:23pm dhyatt: first letter isn't updated
dhyatt suspects you may need some kind of updateFirstLetter that runs in both cases
5:24pm dhyatt: for both RenderInline and RenderBlock
5:24pm dhyatt: if you had that you could probably simplify a lot
5:24pm smfr: so this bug is more involved than I thought
5:24pm dhyatt: since you wouldn't have to drill if you eventually found the right object
5:24pm dhyatt: it's very involved yes

dhyatt: smfr: here are the issues as i see them....
5:26pm dhyatt: i.e., my laundry liust of first-letter problems
5:26pm dhyatt: list
5:26pm dhyatt: (1) we do a full-blown Detach when first-letter rules change.  that sucks.
5:26pm dhyatt: but is not the end of the world
5:26pm dhyatt: in fact this basically means that first-letter being updated only needs to happen because of inheritance
5:26pm dhyatt: and not for any other reason
5:27pm dhyatt: and since first-letter objects have to be the child of either a RenderInline or RenderBlock
5:27pm dhyatt: i think you could just rewrite updateFirstLetter
5:27pm dhyatt: to be a common function to both
5:27pm dhyatt: like before/after stuff
5:27pm smfr: yep
5:27pm dhyatt: then finding it for updating would be really simple
5:28pm dhyatt: but really i do hate that we do a detach when first-letter styles change

[Alexey Proskuryakov]

See also: bug 15602.

[Yuzo Fujishima]

I've uploaded a patch to https://bugs.webkit.org/show_bug.cgi?id=45620 that also fixes this.

[Dave Hyatt]

Moving the discussion here from 45620.

I think you could subclass styleDidChange for the RenderTextFragment and that gives you easy access to the first letter container to update its style.  You can just get right to it.

[Yuzo Fujishima]

Created attachment 69764 [details]
Patch

[Yuzo Fujishima]

Dan, Dave,

Thank you for your reviews and comments. Can you take another look?

Note:

An alternative approach, "Update first letter after style has been recalculated for descendants",
properly handles:

   <p id="target"><span>first letter stays black</span></p>
   ... document.getelementbyid('target').style.color = 'green';

but not:

    <p><span id="target">first letter stays black</span></p> 
   ... document.getelementbyid('target').style.color = 'green';
   (Stale cached pseudo style is used in this case.)

[Yuzo Fujishima]

Ping?

[Dave Hyatt]

Comment on attachment 69764 [details]
Patch

My one concern here is if updateFirstLetter() causes the RenderTextFragment to be destroyed (e.g., if the display type of first-letter changes from inline to block).  I'd like to see a test for that so that we know we won't crash with this patch in place.

[Yuzo Fujishima]

Created attachment 70398 [details]
Patch

[Yuzo Fujishima]

Created attachment 70399 [details]
Added test for first letter removal/addition

[Yuzo Fujishima]

Thank you for the review.

Added a test where RenderTextFragment for the first-letter is removed or added by
display property change ore before pseudo-element change.

My understanding is that in such cases the changes are considered as "Detach" and the
render tree is re-constructed. That is, RenderTextFragment::styleDidChange is not called
where it would destroy the fragment instance if it were called.

(In reply to comment #10)
> (From update of attachment 69764 [details])
> My one concern here is if updateFirstLetter() causes the RenderTextFragment to be destroyed (e.g., if the display type of first-letter changes from inline to block).  I'd like to see a test for that so that we know we won't crash with this patch in place.

[Yuzo Fujishima]

Ping? Sorry to being naggy, but I'd like to fix this issue as soon as possible.

[Abhishek Arya]

*** Bug 45620 has been marked as a duplicate of this bug. ***

[Abhishek Arya]

Yuzo, can you please try reaching Dave on irc.

[Dave Hyatt]

(In reply to comment #13)
> Thank you for the review.
> 
> Added a test where RenderTextFragment for the first-letter is removed or added by
> display property change ore before pseudo-element change.
> 
> My understanding is that in such cases the changes are considered as "Detach" and the
> render tree is re-constructed. That is, RenderTextFragment::styleDidChange is not called
> where it would destroy the fragment instance if it were called.
> 

We only allow the block display type on floating first letters, so instead of switching between block and inline you need to switch between floating and not floating.

You left in the updateFirstLetter() in RenderBlock::styleDidChange.  That call is now wasteful/redundant in most cases, although it does admittedly catch the display change as a result of the float status change. 

I think you could still trip a display change from RenderTextFragment::styleDidChange though.  If the first-letter style has float:inherit, and you put it inside a span inside a div, and then make the span float (and have the span have its own first-letter style rule also).

[Dave Hyatt]

It might not actually crash if the display changes.  That's what I want to see.

[Yuzo Fujishima]

Created attachment 71022 [details]
Patch

[Yuzo Fujishima]

Thank you for the review.

(In reply to comment #17)
> (In reply to comment #13)
> > Thank you for the review.
> > 
> > Added a test where RenderTextFragment for the first-letter is removed or added by
> > display property change ore before pseudo-element change.
> > 
> > My understanding is that in such cases the changes are considered as "Detach" and the
> > render tree is re-constructed. That is, RenderTextFragment::styleDidChange is not called
> > where it would destroy the fragment instance if it were called.
> > 
> 
> We only allow the block display type on floating first letters, so instead of switching between block and inline you need to switch between floating and not floating.
> 
> You left in the updateFirstLetter() in RenderBlock::styleDidChange.  That call is now wasteful/redundant in most cases, although it does admittedly catch the display change as a result of the float status change. 
> 
> I think you could still trip a display change from RenderTextFragment::styleDidChange though.  If the first-letter style has float:inherit, and you put it inside a span inside a div, and then make the span float (and have the span have its own first-letter style rule also).

I've added tests as suggested. (Please let me know if I misunderstood anything).

As far as I understand, updateFirstLetter can destroy:
- Anonymous RenderInline/RenderBlock generated for first letter
- Original RenderText that is "split" into text fragments to achieve first letter rendering
but not the text fragments created by the "split"

[Yuzo Fujishima]

Created attachment 71024 [details]
Added tests for float

[Chris Evans]

Is this one almost ready to land?

[Dave Hyatt]

Comment on attachment 71024 [details]
Added tests for float

r=me

[Abhishek Arya]

Committed r70797: <http://trac.webkit.org/changeset/70797>

[Vincent Danen]

This was assigned CVE-2011-0114.