Bug 103772 – Teach OwnPtr and RefPtr to understand Arena'd objects

• Component: Layout and Rendering
• Status: NEW

[Eric Seidel (no email)]

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.)

[Maciej Stachowiak]

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!

[Eric Seidel (no email)]

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.

[Maciej Stachowiak]

(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.

[Eric Seidel (no email)]

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. :(

[Geoffrey Garen]

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().

[Maciej Stachowiak]

(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.

[Maciej Stachowiak]

(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.

[Geoffrey Garen]

> 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(); }

[Maciej Stachowiak]

(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.

[Eric Seidel (no email)]

(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.

[Eric Seidel (no email)]

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);

[Geoffrey Garen]

> 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 :(.

[Maciej Stachowiak]

(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.

[Maciej Stachowiak]

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.

[Eric Seidel (no email)]

Created attachment 177130 [details]
proof of concept

[Eric Seidel (no email)]

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).

[Eric Seidel (no email)]

(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.

[Julien Chaffraix]

(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.

[Elliott Sprehn]

Btw, as mentioned earlier m_node should never be null, even for generated content. RenderWidget doing setNode(0) seems wrong...