Summary: | Teach OwnPtr and RefPtr to understand Arena'd objects | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | NEW --- | ||||||
Severity: | Normal | CC: | abarth, esprehn, ggaren, hyatt, inferno, jchaffraix, koivisto, mjs, rniwa, sam, simon.fraser, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Eric Seidel (no email)
2012-11-30 13:44:35 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! 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. (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. 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. :( 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(). (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. (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. > 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(); }
(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. (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. 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); > 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 :(. (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. 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. Created attachment 177130 [details]
proof of concept
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). (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. (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. Btw, as mentioned earlier m_node should never be null, even for generated content. RenderWidget doing setNode(0) seems wrong... |