Move updateLayout() call from RenderLayer to RenderView

Move updateLayout() call from RenderLayer to RenderView

This is necessary because otherwise, RenderLayer may end up destroying
itself, leading to unhappiness.

Further, the updateLayout call needs to recursively update the child
documents' layout as well, see the comment in the patch.

This was uncovered when I removed superfluous layout() calls from
event sender, but there is likely a way to trigger this from web
content also.

R=ojan@chromium.org
BUG=241090
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150612

[ojan, 2013-05-16 23:57:54]

https://codereview.chromium.org/15219003/diff/1/Source/core/rendering/RenderV...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/15219003/diff/1/Source/core/rendering/RenderV...
Source/core/rendering/RenderView.cpp:91: // into a child document, it could
trigger a layout on the parent document, which can destroy RenderLayers
When does hit-testing in a child document trigger a layout in the parent
document? I expect this code is right, I just want to understand the codepath
that requires the recursive bit before lgtm'ing.

[cbiesinger, 2013-05-17 00:17:03]

https://codereview.chromium.org/15219003/diff/1/Source/core/rendering/RenderV...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/15219003/diff/1/Source/core/rendering/RenderV...
Source/core/rendering/RenderView.cpp:91: // into a child document, it could
trigger a layout on the parent document, which can destroy RenderLayers
On 2013/05/16 23:57:54, ojan wrote:
> When does hit-testing in a child document trigger a layout in the parent
> document? I expect this code is right, I just want to understand the codepath
> that requires the recursive bit before lgtm'ing.

Document::updateLayout calls updateLayout on its parent:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

But now that you ask, I don't fully understand why that is a problem... I think
I need to spend more time in gdb.

[eseidel, 2013-05-17 01:04:39]

seamless iframes depend on their parent document having resolved style.  I'm not
sure about having resolved layout however.

[cbiesinger, 2013-05-17 01:05:43]

Here's my printf output (with some manual formatting). Clearly, something marks
the outer document as needing layout... I also didn't expect the reentrant
RenderView::hitTest calls.


EventSender::mouseDown(0, 0)
-> RenderView::hitTest(document=0x32dee11ae020 layer=0x32dee145edb8)
  -> Document::updateLayout(this=0x32dee11ae020)
  <- Document::updateLayout
  -> RenderView::hitTest(document=0x32dee11b1420 layer=0x32dee149d038)
    -> Document::updateLayout(this=0x32dee11b1420)
      -> Document::updateLayout(this=0x32dee11ae020)
      <- Document::updateLayout
      Document updating style(this=0x32dee11b1420)
      FrameView::layout(document=0x32dee11b1420)
    <- Document::updateLayout
    -> RenderView::hitTest(document=0x32dee11b1420 layer=0x32dee149d038)
      -> Document::updateLayout(this=0x32dee11b1420)
        -> Document::updateLayout(this=0x32dee11ae020)
           Document updating style(this=0x32dee11ae020)
           FrameView::layout(document=0x32dee11ae020)
        <- Document::updateLayout
      <- Document::updateLayout
      -> RenderView::hitTest(document=0x32dee11b1420 layer=0x32dee149d038)
        -> Document::updateLayout(this=0x32dee11b1420)
          -> Document::updateLayout(this=0x32dee11ae020)
          <- Document::updateLayout
        <- Document::updateLayout

[cbiesinger, 2013-05-17 01:07:07]

perhaps this is easier to read:

EventSender::mouseDown(0, 0)
-> RenderView::hitTest(document=OUTER layer=L1)
  -> Document::updateLayout(this=OUTER)
  <- Document::updateLayout
  -> RenderView::hitTest(document=INNER layer=L2)
    -> Document::updateLayout(this=INNER)
      -> Document::updateLayout(this=OUTER)
      <- Document::updateLayout
      Document updating style(this=INNER)
      FrameView::layout(document=INNER)
    <- Document::updateLayout
    -> RenderView::hitTest(document=INNER layer=L2)
      -> Document::updateLayout(this=INNER)
        -> Document::updateLayout(this=OUTER)
           Document updating style(this=OUTER)
           FrameView::layout(document=OUTER)
        <- Document::updateLayout
      <- Document::updateLayout
      -> RenderView::hitTest(document=INNER layer=L2)
        -> Document::updateLayout(this=INNER)
          -> Document::updateLayout(this=OUTER)
          <- Document::updateLayout
        <- Document::updateLayout

[ojan, 2013-05-17 01:11:45]

Ick. Document::updateLayout calls ownerElement()->document()->updateLayout(),
i.e. calling updateLayout on a document walks up the frame tree calling
updateLayout on each ancestor document. That's crazy talk. Not sure why we do
that, but it seems totally wrong to me.

Anyways, that explains what's going on here. I don't think we need to block
fixing this on figuring that out thought. I'm fine with this patch if you change
the comment to a FIXME with an explanation of this codepath.

[ojan, 2013-05-17 01:12:24]

CC some other people who might have some insight into the Document::updateLayout
behavior.

[cbiesinger, 2013-05-17 01:15:30]

Sigh, there was a bug in my printfs, don't pay too much attention to them (<-
RenderView::hitTest was never printed)

I'll add the FIXME.

[jamesr, 2013-05-17 01:15:52]

Who calls RenderView::hitTest() without first updating layout?  Can this just be
an ASSERT() that layout is up to date?

I think it should be up to entry points to this function to make sure layout's
sufficient up to date.  If the hit testing code is inside WebCore, it should
know whether layout's up to date or not and do the right thing.  If the caller
is outside of core/, it should call layout() on the top level before doing the
hit test.

[ojan, 2013-05-17 01:22:41]

The code to walk up the owner chain updating layout was added in
http://trac.webkit.org/changeset/20936. We still have that and it passes,
although, I think this is one of those big-hammer solutions. We should totally
get rid of that call and fix that test in a more targeted way.

On 2013/05/17 01:15:52, jamesr wrote:
> Who calls RenderView::hitTest() without first updating layout?  Can this just
be
> an ASSERT() that layout is up to date?
> 
> I think it should be up to entry points to this function to make sure layout's
> sufficient up to date.  If the hit testing code is inside WebCore, it should
> know whether layout's up to date or not and do the right thing.  If the caller
> is outside of core/, it should call layout() on the top level before doing the
> hit test.

RenderView's hitTest is called from dozens of places. At a quick glance, I
couldn't find any that ensure layout is up to date.

[cbiesinger, 2013-05-17 01:23:57]

On 2013/05/17 01:15:52, jamesr wrote:
> Who calls RenderView::hitTest() without first updating layout?

Everyone, as far as I can tell...

Notably Document::prepareMouseEvent, EventHandler::hitTestAtPoint,
TreeScope.cpp:nodeFromPoint, I stopped looking

[jamesr, 2013-05-17 01:25:50]

Huh, interesting.  Having the layout() inside RenderView is definitely better
than having it in RenderLayer(), although I think we should try to push it out
further.

[ojan, 2013-05-17 01:39:06]

On 2013/05/17 01:25:50, jamesr wrote:
> Huh, interesting.  Having the layout() inside RenderView is definitely better
> than having it in RenderLayer(), although I think we should try to push it out
> further.

Yup. That's what I was thinking too.

[cbiesinger, 2013-05-17 02:13:29]

So I have news about where the additional scheduled layout comes from.

When the inner frame finishes up layout, it calls
FrameView::updateCompositingLayersAfterLayout. That function does a few things
but [for this testcase] ends up destroying the child frame's root layer, which
in turn marks the parent document as needing style recalc.

So in the given testcase, we start out with a clean root document and dirty
child document (so if we just call rootdoc->updateLayout() it does nothing),
when we reach the child to call updateLayout() it schedules a style recalc on
the root doc (!), then when we revisit the child again for an additional hit
test it calls updateLayout() again on itself and the root doc, which ends up
destroying some RenderLayer objects that we need. Oops.

It's not clear to me whether it's a good thing that laying out the child doc can
dirty its parent like this.

[cbiesinger, 2013-05-17 02:14:35]

On 2013/05/17 01:39:06, ojan wrote:
> On 2013/05/17 01:25:50, jamesr wrote:
> > Huh, interesting.  Having the layout() inside RenderView is definitely
better
> > than having it in RenderLayer(), although I think we should try to push it
out
> > further.
> 
> Yup. That's what I was thinking too.

I'm not necessarily opposed to pushing the layout call out further, but there's
lots of callsites. Should we update them all?

[jamesr, 2013-05-17 02:17:14]

On 2013/05/17 02:13:29, cbiesinger wrote:
> So I have news about where the additional scheduled layout comes from.
> 
> When the inner frame finishes up layout, it calls
> FrameView::updateCompositingLayersAfterLayout. That function does a few things
> but [for this testcase] ends up destroying the child frame's root layer, which
> in turn marks the parent document as needing style recalc.
> 
> So in the given testcase, we start out with a clean root document and dirty
> child document (so if we just call rootdoc->updateLayout() it does nothing),
> when we reach the child to call updateLayout() it schedules a style recalc on
> the root doc (!), then when we revisit the child again for an additional hit
> test it calls updateLayout() again on itself and the root doc, which ends up
> destroying some RenderLayer objects that we need. Oops.
> 
> It's not clear to me whether it's a good thing that laying out the child doc
can
> dirty its parent like this.

That's generally speaking bad, but it's been hard to hammer out all the cases
where this can happen.  Please file a separate bug about the specific content
that's causing the reach back up here.

I think the best way to be robust to stuff like this is to do a full tree layout
before starting on the top level logic here (say before starting to process the
input event that requires hit testing) and then trying not to enter layout()
again until we're at the top level again.  I.e. if we run some event handlers
and then want to hit test again, we'd definitely have to layout() again.

For there where there are a lot of callsites it's not immediately obvious to me
what to do.  Are the callsites all representing a more limited set of logical
operations?  For instance, all of the calls that are in response to handling an
input event from the user definitely bottleneck further up the stack.  Where are
the other callers coming from?

[ojan, 2013-05-17 02:23:52]

On 2013/05/17 02:13:29, cbiesinger wrote:
> It's not clear to me whether it's a good thing that laying out the child doc
can
> dirty its parent like this.

It's clear to me that this is a bad thing. We should have an explicit way of
laying out your whole ancestor chain and call it in the places where we need it
instead of always doing that on every updateLayout call.

On 2013/05/17 02:14:35, cbiesinger wrote:
> On 2013/05/17 01:39:06, ojan wrote:
> > On 2013/05/17 01:25:50, jamesr wrote:
> > > Huh, interesting.  Having the layout() inside RenderView is definitely
> better
> > > than having it in RenderLayer(), although I think we should try to push it
> out
> > > further.
> > 
> > Yup. That's what I was thinking too.
> 
> I'm not necessarily opposed to pushing the layout call out further, but
there's
> lots of callsites. Should we update them all?

Yeah, this isn't totally clear to me. I like the principle of it though. Our
reliance on these big hammers often leads to wrong code elsewhere, e.g. you can
imagine code that grabs a RenderLayer, then does a hit test, and then does
something with the RenderLayer, but now the RenderLayer has been freed. It's
problematic that hitTesting can destroy RenderLayers. Heh, now that I've said
all that, I've convinced myself that it's bad to do this in hit-testing. lol.

Anyways, I think this patch is a step in that direction and we should do this
first, but maybe there should be another FIXME for moving this out of
RenderView's hitTest.

[cbiesinger, 2013-05-17 02:30:52]

On 2013/05/17 02:23:52, ojan wrote:
> On 2013/05/17 02:13:29, cbiesinger wrote:
> > It's not clear to me whether it's a good thing that laying out the child doc
> can
> > dirty its parent like this.
> 
> It's clear to me that this is a bad thing. We should have an explicit way of
> laying out your whole ancestor chain and call it in the places where we need
it
> instead of always doing that on every updateLayout call.

I filed
https://code.google.com/p/chromium/issues/detail?id=241659&thanks=241659&ts=1...
on this setStyleNeedsRecalc thing. I'll look into the parent->updateLayout()
thing tomorrow.

> On 2013/05/17 02:14:35, cbiesinger wrote:
> > On 2013/05/17 01:39:06, ojan wrote:
> > > On 2013/05/17 01:25:50, jamesr wrote:
> > > > Huh, interesting.  Having the layout() inside RenderView is definitely
> > better
> > > > than having it in RenderLayer(), although I think we should try to push
it
> > out
> > > > further.
> > > 
> > > Yup. That's what I was thinking too.
> > 
> > I'm not necessarily opposed to pushing the layout call out further, but
> there's
> > lots of callsites. Should we update them all?
> 
> Yeah, this isn't totally clear to me. I like the principle of it though. Our
> reliance on these big hammers often leads to wrong code elsewhere, e.g. you
can
> imagine code that grabs a RenderLayer, then does a hit test, and then does
> something with the RenderLayer, but now the RenderLayer has been freed. It's
> problematic that hitTesting can destroy RenderLayers. Heh, now that I've said
> all that, I've convinced myself that it's bad to do this in hit-testing. lol.
> 
> Anyways, I think this patch is a step in that direction and we should do this
> first, but maybe there should be another FIXME for moving this out of
> RenderView's hitTest.

Alright, I added a fixme. I'll think more about moving this to the callers.

[ojan, 2013-05-17 04:20:14]

lgtm

[commit-bot: I haz the power, 2013-05-17 18:33:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/15219003/21001

[commit-bot: I haz the power, 2013-05-17 19:23:28]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-17 21:09:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/15219003/21001

[commit-bot: I haz the power, 2013-05-17 21:47:51]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[cbiesinger, 2013-05-17 21:49:14]

Committed patchset #2 manually as r150612 (presubmit successful).