Oilpan: fix build after r183913.

Oilpan: fix build after r183913.

TBR=oilpan-reviews,haraken
BUG=391288
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183928

[sof, 2014-10-17 21:41:22]

Please take a look.

[sof, 2014-10-17 21:41:53]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-17 21:42:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/659953004/1

[commit-bot: I haz the power, 2014-10-17 21:43:02]

Committed patchset #1 (id:1) as 183928

[esprehn, 2014-10-17 22:02:05]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2014-10-17 22:02:05]

I think you just added a security bug.

https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderTextFragment.cpp:56: #if !ENABLE(OILPAN)
This is not correct so skip in oilpan, you're leaving bad ptrs to RenderObjects
now since you skip the setRemainingTextRenderer.

[sof, 2014-10-18 04:50:52]

https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderTextFragment.cpp:56: #if !ENABLE(OILPAN)
On 2014/10/17 22:02:05, esprehn wrote:
> This is not correct so skip in oilpan, you're leaving bad ptrs to
RenderObjects
> now since you skip the setRemainingTextRenderer.

FirstPseudoElement traces its "remaining text renderer" object. How can this
object die without m_firstLetterPseudoElement also dying at the same time?

[dsinclair, 2014-10-18 04:56:32]

On 2014/10/18 04:50:52, sof wrote:
>
https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderTextFragment.cpp (right):
> 
>
https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderTextFragment.cpp:56: #if !ENABLE(OILPAN)
> On 2014/10/17 22:02:05, esprehn wrote:
> > This is not correct so skip in oilpan, you're leaving bad ptrs to
> RenderObjects
> > now since you skip the setRemainingTextRenderer.
> 
> FirstPseudoElement traces its "remaining text renderer" object. How can this
> object die without m_firstLetterPseudoElement also dying at the same time?


I'm not sure what it means for first letter to trace the remaining text
renderer, but I don't think that's correct.

There are two renderers involved with first letter. The renderer for the letter
and the remaining text renderer. The remaining text renderer is not  a child of
the pseudo element. It has a lifecycle which isn't tied to the pseudo element
(but, setting text of the remaining text renderer can cause the first letter to
be destroyed during a subsequent style recalc). If the remaining text renderer
is removed we _have_ to unlink it from the pseudo element or bad things are
going to happen because the pseudo element may attempt to access the renderer
which has been destroyed.

[sof, 2014-10-18 04:59:46]

On 2014/10/18 04:56:32, dsinclair wrote:
> On 2014/10/18 04:50:52, sof wrote:
> >
>
https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
> > File Source/core/rendering/RenderTextFragment.cpp (right):
> > 
> >
>
https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
> > Source/core/rendering/RenderTextFragment.cpp:56: #if !ENABLE(OILPAN)
> > On 2014/10/17 22:02:05, esprehn wrote:
> > > This is not correct so skip in oilpan, you're leaving bad ptrs to
> > RenderObjects
> > > now since you skip the setRemainingTextRenderer.
> > 
> > FirstPseudoElement traces its "remaining text renderer" object. How can this
> > object die without m_firstLetterPseudoElement also dying at the same time?
> 
> 
> I'm not sure what it means for first letter to trace the remaining text
> renderer, but I don't think that's correct.
> 
> There are two renderers involved with first letter. The renderer for the
letter
> and the remaining text renderer. The remaining text renderer is not  a child
of
> the pseudo element. It has a lifecycle which isn't tied to the pseudo element
> (but, setting text of the remaining text renderer can cause the first letter
to
> be destroyed during a subsequent style recalc). If the remaining text renderer
> is removed we _have_ to unlink it from the pseudo element or bad things are
> going to happen because the pseudo element may attempt to access the renderer
> which has been destroyed.

Thanks for adding details on what needs to happen.

That detach happens in FirstLetterPseudoElement::detach(), right? It clears
these pointers.

[sof, 2014-10-18 05:27:10]

On 2014/10/18 04:59:46, sof wrote:
> On 2014/10/18 04:56:32, dsinclair wrote:
> > On 2014/10/18 04:50:52, sof wrote:
> > >
> >
>
https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
> > > File Source/core/rendering/RenderTextFragment.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
> > > Source/core/rendering/RenderTextFragment.cpp:56: #if !ENABLE(OILPAN)
> > > On 2014/10/17 22:02:05, esprehn wrote:
> > > > This is not correct so skip in oilpan, you're leaving bad ptrs to
> > > RenderObjects
> > > > now since you skip the setRemainingTextRenderer.
> > > 
> > > FirstPseudoElement traces its "remaining text renderer" object. How can
this
> > > object die without m_firstLetterPseudoElement also dying at the same time?
> > 
> > 
> > I'm not sure what it means for first letter to trace the remaining text
> > renderer, but I don't think that's correct.
> > 
> > There are two renderers involved with first letter. The renderer for the
> letter
> > and the remaining text renderer. The remaining text renderer is not  a child
> of
> > the pseudo element. It has a lifecycle which isn't tied to the pseudo
element
> > (but, setting text of the remaining text renderer can cause the first letter
> to
> > be destroyed during a subsequent style recalc). If the remaining text
renderer
> > is removed we _have_ to unlink it from the pseudo element or bad things are
> > going to happen because the pseudo element may attempt to access the
renderer
> > which has been destroyed.
> 
> Thanks for adding details on what needs to happen.
> 
> That detach happens in FirstLetterPseudoElement::detach(), right? It clears
> these pointers.

But if dropping a RenderTextFragment from the render tree must synchronously
unlink/detach it from its pseudo element, then I think we need to do that via a
destroy() for Oilpan.

[sof, 2014-10-18 06:59:44]

On 2014/10/18 05:27:10, sof wrote:
> On 2014/10/18 04:59:46, sof wrote:
> > On 2014/10/18 04:56:32, dsinclair wrote:
> > > On 2014/10/18 04:50:52, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
> > > > File Source/core/rendering/RenderTextFragment.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/659953004/diff/1/Source/core/rendering/Render...
> > > > Source/core/rendering/RenderTextFragment.cpp:56: #if !ENABLE(OILPAN)
> > > > On 2014/10/17 22:02:05, esprehn wrote:
> > > > > This is not correct so skip in oilpan, you're leaving bad ptrs to
> > > > RenderObjects
> > > > > now since you skip the setRemainingTextRenderer.
> > > > 
> > > > FirstPseudoElement traces its "remaining text renderer" object. How can
> this
> > > > object die without m_firstLetterPseudoElement also dying at the same
time?
> > > 
> > > 
> > > I'm not sure what it means for first letter to trace the remaining text
> > > renderer, but I don't think that's correct.
> > > 
> > > There are two renderers involved with first letter. The renderer for the
> > letter
> > > and the remaining text renderer. The remaining text renderer is not  a
child
> > of
> > > the pseudo element. It has a lifecycle which isn't tied to the pseudo
> element
> > > (but, setting text of the remaining text renderer can cause the first
letter
> > to
> > > be destroyed during a subsequent style recalc). If the remaining text
> renderer
> > > is removed we _have_ to unlink it from the pseudo element or bad things
are
> > > going to happen because the pseudo element may attempt to access the
> renderer
> > > which has been destroyed.
> > 
> > Thanks for adding details on what needs to happen.
> > 
> > That detach happens in FirstLetterPseudoElement::detach(), right? It clears
> > these pointers.
> 
> But if dropping a RenderTextFragment from the render tree must synchronously
> unlink/detach it from its pseudo element, then I think we need to do that via
a
> destroy() for Oilpan.

https://codereview.chromium.org/660413002/ does that.

[haraken, 2014-10-18 11:08:33]

LGTM