Harden the machinery around updateWidgetPositions()

Harden the machinery around updateWidgetPositions()

updateWidgetPositions() can blow away the RenderView by running script or
calling into plugins. This patch moves it from RenderView to FrameView
since having this method on RenderView which might destroy itself is not
safe. It also switches to using normal RefPtr instead of manually managing
the refcount and finally adds RefPtr to callers of updateWidgetPositions()
to avoid use-after-frees.

There's one final call inside RenderLayerScrollableArea::setScrollOffset
which is not safe but is difficult to mitigate since we're way down a
callstack by the time this call is made which can destroy the render tree
and the RenderLayerScrollableArea. This patch adds a RELEASE_ASSERT to
kill the renderer in case we get into a sitaution where this happens.
In the future we should detangle this concept entirely so such an
ASSERT isn't needed and so that the render tree can never destroy itself
from the inside.

It's not clear how to write a test for this since you need to get us to
go into the scrolling code with a dirty tree or have a plugin that does
something nefarious.

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

[esprehn, 2014-01-10 23:39:10]

ping

[eseidel, 2014-01-10 23:54:30]

https://codereview.chromium.org/132913002/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/132913002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:1263: void FrameView::addWidget(RenderWidget*
object)
So these are effectively children, since FrameView is itself a Widget.  Although
RenderWidget is the RenderObject wrapper around Widget... although James MacLean
is about to move the Widget* off of RenderWidget and onto DOM nodes.  I wonder
if this shouldn't just hold onto the Widget* directly?  It feels odd for
FrameView to hold onto pointers into the rendering tree besides RenderView (the
root of the tree).

[eseidel, 2014-01-10 23:56:35]

https://codereview.chromium.org/132913002/diff/1/Source/core/frame/FrameView.h
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/132913002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.h:466: HashSet<RefPtr<RenderWidget> > m_widgets;
So this didn't used to be possible due to RenderArena.  I guess RenderArena is
gone and now it's safe to hold RenderWidget in a RefPtr?

[esprehn, 2014-01-12 20:18:58]

https://codereview.chromium.org/132913002/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/132913002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:1263: void FrameView::addWidget(RenderWidget*
object)
On 2014/01/10 23:54:30, eseidel wrote:
> So these are effectively children, since FrameView is itself a Widget. 
Although
> RenderWidget is the RenderObject wrapper around Widget... although James
MacLean
> is about to move the Widget* off of RenderWidget and onto DOM nodes.  I wonder
> if this shouldn't just hold onto the Widget* directly?  It feels odd for
> FrameView to hold onto pointers into the rendering tree besides RenderView
(the
> root of the tree).

I'm happy to make that change in the next patch. I didn't want to change
behavior much and messing with RenderWidget and Widget lifetimes by moving the
refs or things you call into around seems like something I should do separate of
this patch.

https://codereview.chromium.org/132913002/diff/1/Source/core/frame/FrameView.h
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/132913002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.h:466: HashSet<RefPtr<RenderWidget> > m_widgets;
On 2014/01/10 23:56:35, eseidel wrote:
> So this didn't used to be possible due to RenderArena.  I guess RenderArena is
> gone and now it's safe to hold RenderWidget in a RefPtr?

Yup, now that we use PartitionAlloc we don't need all that manual ref counting.

[eseidel, 2014-01-12 20:24:58]

OK.  Please add a FIXME About making these Widget* instead.  lgtm.

[commit-bot: I haz the power, 2014-01-14 09:16:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/132913002/170001

[commit-bot: I haz the power, 2014-01-14 10:19:02]

Change committed as 165052