Bug 103772 - Teach OwnPtr and RefPtr to understand Arena'd objects
Summary: Teach OwnPtr and RefPtr to understand Arena'd objects
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-30 13:44 PST by Eric Seidel (no email)
Modified: 2017-07-18 08:30 PDT (History)
12 users (show)

See Also:


Attachments
proof of concept (6.18 KB, patch)
2012-12-01 23:26 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-11-30 13:44:35 PST
Teach OwnPtr and RefPtr to understand Arena'd objects

I believe it should be possible with template-specializations for OwnPtr<RenderObject>?

RenderObject's always have a node (even if that node is the Document), so node()->document()->arena() should always work for retrieving the arena.

This would allow deployment of OwnPtr/RefPtr throughout the rendering tree and should make the code cleaner and the lifetimes of many objects easier to follow.

(If such c++ template-foolery is not possible, please feel encouraged to correct me.)
Comment 1 Maciej Stachowiak 2012-11-30 22:06:02 PST
Oh, I hadn't thought about getting the Arena via Node! Given that, it is definitely possible to apply OwnPtr by defining an appropriate specialization/overload of deleteOwnedPtr. That is what OwnPtr uses instead of calling delete directly. See OwnPtrCommon.h for how this is done for some other types.

For RefPtr it would also be doable, by having deref() look up the arena, since RefPtr makes no assumptions whatsoever regarding how the refcount is managed or how objects are destroyed, it just calls ref() and deref().

Great insight, Eric!
Comment 2 Eric Seidel (no email) 2012-12-01 10:31:54 PST
It appears we already do this:

RenderArena* renderArena() const { return document()->renderArena(); }

void RenderObject::destroy()
{
    willBeDestroyed();
    arenaDelete(renderArena(), this);
}

So then OwnPtrRenderObject just needs:

template <> void deleteOwnedPtr<RenderObject>(RenderObject* object)
{
    if (object)
        object->destroy();
}

Unless I'm missing something.
Comment 3 Maciej Stachowiak 2012-12-01 11:16:10 PST
(In reply to comment #2)
> It appears we already do this:
> 
> RenderArena* renderArena() const { return document()->renderArena(); }
> 
> void RenderObject::destroy()
> {
>     willBeDestroyed();
>     arenaDelete(renderArena(), this);
> }
> 
> So then OwnPtrRenderObject just needs:
> 
> template <> void deleteOwnedPtr<RenderObject>(RenderObject* object)
> {
>     if (object)
>         object->destroy();
> }
> 
> Unless I'm missing something.

Yes, I think that would be sufficient to use OwnPtr and PassOwnPtr for RenderObjects. There might be other arena-allocated Render* types that don't inherit from RenderObject though.
Comment 4 Eric Seidel (no email) 2012-12-01 14:50:56 PST
So I have a working patch...  But unfortunately it doesn't really make sense to ever have an OwnPtr<RenderObject> as RenderWidget (a RenderObject subclass) expects to be ref-counted. :(
Comment 5 Geoffrey Garen 2012-12-01 14:58:55 PST
I've heard that a renderer's node can sometimes be null. Generated / anonymous content like :before, or something. See also bug 103784 for a null node.

I'm not familiar enough with this code to say for sure, but this might also block unconditionally dereferencing document().
Comment 6 Maciej Stachowiak 2012-12-01 15:20:24 PST
(In reply to comment #5)
> I've heard that a renderer's node can sometimes be null. Generated / anonymous content like :before, or something. See also bug 103784 for a null node.
> 
> I'm not familiar enough with this code to say for sure, but this might also block unconditionally dereferencing document().

Anonymous objects should still have an m_node, though node() will be null:

    Node* node() const { return isAnonymous() ? 0 : m_node; }

However I'm not expert enough to know if there are edge cases.
Comment 7 Maciej Stachowiak 2012-12-01 15:22:09 PST
(In reply to comment #4)
> So I have a working patch...  But unfortunately it doesn't really make sense to ever have an OwnPtr<RenderObject> as RenderWidget (a RenderObject subclass) expects to be ref-counted. :(

RenderWidget is an odd duck. It would be nicer if all RenderObjects were refcounted in part so we could avoid the strangeness of Widget being refcounted while being part of a class hierarchy that generally uses single ownership.

However, OwnPtr<RenderWidget> would still do the right thing, because RenderWidget overloads destroy() to actually deref() - it kind of has to anyway to interoperate with even the manual render tree memory management. See RenderWidget::destroy in RenderWidget.cpp.
Comment 8 Geoffrey Garen 2012-12-01 16:35:31 PST
> Anonymous objects should still have an m_node, though node() will be null:

If so, this function may crash...

RenderArena* renderArena() const { return document()->renderArena(); }

...because this function deferences m_node:

    Document* document() const { return m_node->document(); }
Comment 9 Maciej Stachowiak 2012-12-01 16:43:59 PST
(In reply to comment #8)
> > Anonymous objects should still have an m_node, though node() will be null:
> 
> If so, this function may crash...
> 
> RenderArena* renderArena() const { return document()->renderArena(); }
> 
> ...because this function deferences m_node:
> 
>     Document* document() const { return m_node->document(); }

What I'm saying is that (to the best of my understanding) node() is null for anonymous objects but m_node is not. node() specifically checks for isAnonymous() to achieve this. There may well also be cases where even m_node is null. I do not know enough about rendering details to say for sure.
Comment 10 Eric Seidel (no email) 2012-12-01 16:50:25 PST
(In reply to comment #9)
> (In reply to comment #8)
> > > Anonymous objects should still have an m_node, though node() will be null:
> > 
> > If so, this function may crash...
> > 
> > RenderArena* renderArena() const { return document()->renderArena(); }
> > 
> > ...because this function deferences m_node:
> > 
> >     Document* document() const { return m_node->document(); }
> 
> What I'm saying is that (to the best of my understanding) node() is null for anonymous objects but m_node is not. node() specifically checks for isAnonymous() to achieve this. There may well also be cases where even m_node is null. I do not know enough about rendering details to say for sure.

That matches my understanding.
Comment 11 Eric Seidel (no email) 2012-12-01 16:52:08 PST
RenderObject::RenderObject(Node* node)
    : CachedImageClient()
    , m_style(0)
    , m_node(node)
{
...
    ASSERT(node);
}

RenderWidget is again however lame:

    // Grab the arena from node()->document()->renderArena() before clearing the node pointer.
    // Clear the node before deref-ing, as this may be deleted when deref is called.
    RenderArena* arena = renderArena();
    setNode(0);
    deref(arena);
Comment 12 Geoffrey Garen 2012-12-01 17:08:42 PST
> What I'm saying is that (to the best of my understanding) node() is null for anonymous objects but m_node is not.

Ah. Duh :).

> RenderWidget is again however lame:

This might explain bug 103784, cited above.

It's not clear to me how reference-counting RenderWidget works at all. After destroy(), the whole arena can be deleted, so the reference count seems meaningless :(.
Comment 13 Maciej Stachowiak 2012-12-01 17:27:36 PST
(In reply to comment #12)
> > What I'm saying is that (to the best of my understanding) node() is null for anonymous objects but m_node is not.
> 
> Ah. Duh :).
> 
> > RenderWidget is again however lame:
> 
> This might explain bug 103784, cited above.
> 
> It's not clear to me how reference-counting RenderWidget works at all. After destroy(), the whole arena can be deleted, so the reference count seems meaningless :(.

I believe its refcounting works only in a limited set of circumstances, specifically for the use that RenderWidgetProtector makes of it.
Comment 14 Maciej Stachowiak 2012-12-01 17:28:53 PST
CC-ing some people who might understand how RenderWidget's refcounting is supposed to work, and/or in what circumstances RenderObject::m_node can legitimately be null.
Comment 15 Eric Seidel (no email) 2012-12-01 23:26:42 PST
Created attachment 177130 [details]
proof of concept
Comment 16 Eric Seidel (no email) 2012-12-01 23:28:05 PST
Didn't mean to assign the bug to me.  This is mostly a proof of concept.

It's probably more interesting to add RefPtr support and use that with RenderLayer.  RenderObject is extra tricky because of RenderWidget, as well as the fact that we are currently using a union to store the RenderObject* on Node (the RareData optimization).
Comment 17 Eric Seidel (no email) 2012-12-01 23:40:07 PST
(In reply to comment #16)
> It's probably more interesting to add RefPtr support and use that with RenderLayer.  RenderObject is extra tricky because of RenderWidget, as well as the fact that we are currently using a union to store the RenderObject* on Node (the RareData optimization).

I was confused.  RenderLayer is not ref-counted.  I'm not sure OwnPtr is even a win there, since the code seems to imply that it's only safe to call destroy() from inside RenderLayerModeObject::destroyLayer.
Comment 18 Julien Chaffraix 2012-12-03 11:09:36 PST
(In reply to comment #17)
> (In reply to comment #16)
> > It's probably more interesting to add RefPtr support and use that with RenderLayer.  RenderObject is extra tricky because of RenderWidget, as well as the fact that we are currently using a union to store the RenderObject* on Node (the RareData optimization).
> 
> I was confused.  RenderLayer is not ref-counted.  I'm not sure OwnPtr is even a win there, since the code seems to imply that it's only safe to call destroy() from inside RenderLayerModeObject::destroyLayer.

I would still support doing the change for the sake of using more readable / better patterns in the render tree. Note that another way to switch RenderLayer to OwnPtr is to fix bug 87523. It's more of an experiment but it would make the move trivial.
Comment 19 Elliott Sprehn 2012-12-03 11:19:28 PST
Btw, as mentioned earlier m_node should never be null, even for generated content. RenderWidget doing setNode(0) seems wrong...