Remove updateFirstLetter from RenderTableCell::layout.

Remove updateFirstLetter from RenderTableCell::layout.

This call no longer seems to be necessary. All tests pass with it removed.
The original CL which added it added a crash test which no longer crashes
without this line.

https://src.chromium.org/viewvc/blink?revision=117309&view=revision

BUG=391288

[dsinclair, 2014-07-03 19:37:24]

PTAL.

kenrb@ does this seem sane?

[kenrb, 2014-07-03 20:09:43]

I believe first-letter selectors no longer cause the render tree to be modified
during layout. If that is correct, then removing this line should be fine
because the use-after-free that it fixed depended on a text node child of a
RenderTableCell being deleted during layoutRunsAndFloats() for that block.

[dsinclair, 2014-07-04 00:57:59]

On 2014/07/03 20:09:43, kenrb wrote:
> I believe first-letter selectors no longer cause the render tree to be
modified
> during layout. If that is correct, then removing this line should be fine
> because the use-after-free that it fixed depended on a text node child of a
> RenderTableCell being deleted during layoutRunsAndFloats() for that block.

They still do. I'm currently working on making them not do that. As a first
step, I've been trying to identify the various call sites and the associated
tests. For calls that don't seem to have any effect, I'm hoping to remove them
to limit the amount of stuff I need to deal with as I move updateFirstLetter out
of layout().

I'm assuming the test in r117309 was a crashing test? It isn't crashing anymore
with the updateFirstLetter removed. There has been a lot of changes in the
rendering code the last few months, is it possible this was fixed by another
patch along the way?

[kenrb, 2014-07-04 14:37:15]

On 2014/07/04 00:57:59, dsinclair wrote:
> On 2014/07/03 20:09:43, kenrb wrote:
> > I believe first-letter selectors no longer cause the render tree to be
> modified
> > during layout. If that is correct, then removing this line should be fine
> > because the use-after-free that it fixed depended on a text node child of a
> > RenderTableCell being deleted during layoutRunsAndFloats() for that block.
> 
> They still do. I'm currently working on making them not do that. As a first
> step, I've been trying to identify the various call sites and the associated
> tests. For calls that don't seem to have any effect, I'm hoping to remove them
> to limit the amount of stuff I need to deal with as I move updateFirstLetter
out
> of layout().
> 
> I'm assuming the test in r117309 was a crashing test? It isn't crashing
anymore
> with the updateFirstLetter removed. There has been a lot of changes in the
> rendering code the last few months, is it possible this was fixed by another
> patch along the way?

It's possible, but it is also possible that a change caused the test case to no
longer manifest the use-after-free but it could be exercised by a different test
case. Cluster-fuzz generates these tests and they are often brittle.

I would like to have a higher level of confidence that this won't re-introduce
the bug before we remove that line. If it helps, here is what was happening (on
the off chance that there is something obvious that has changed that would block
this sequence from happening):

RenderTableCell with a first-letter selector
 \-- text child (RenderText)
 \-- floating div child (RenderBlock)

- following an initial layout, the text child of the RenderTableCell is reset
without dirtying the preferred logical widths of the RenderTable (so
updateFirstLetter on the RenderTable doesn't get called)
- the RenderTableCell has a floating RenderBlock as a child, which gets layout
in the middle of layoutRunsAndFloats()
- layout of the float calls updateFirstLetter which updates RenderTableCell's
first-letter, destroying the text node during line layout
- the text node is subsequently referenced, resulting in a use-after-free

[esprehn, 2014-07-21 19:04:07]

Who updates the first letter of table cells now once you remove this?

[dsinclair, 2014-07-21 19:16:23]

On 2014/07/21 19:04:07, esprehn wrote:
> Who updates the first letter of table cells now once you remove this?

So, according to the CL which added the check this would be done in the
computePreferredLogicalWidths(). This call to updateFirstLetter was added to
fixup a security race condition.

I'm going to close this CL as we aren't sure if the crash is still fixed with
this removed, or if things have just changed enough that we don't trigger the
crash.

This call will be left until we move updateFirstLetter to happen after
reccalcStyle and before layout.