[Webkit-unassigned] [Bug 89000] [CSS Regions] Fix the lifecycle for the flow objects and their renderers

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=89000





--- Comment #44 from Andrei Bucur <abucur at adobe.com>  2012-07-13 02:52:08 PST ---
(In reply to comment #43)
> (From update of attachment 151916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151916&action=review
> 
> LGTM.
> 
> > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:68
> > +    RefPtr<WebKitNamedFlow> newFlow = WebKitNamedFlow::create(this, flowName);
> > +    m_namedFlows.add(newFlow.get());
> > +
> > +    return newFlow.release();
> 
> Some of the WTF add functions return what they've added.  It's possible you could make these 3 into one line if that's true.
That's a good point. Unfortunately the "add" method returns a struct with an iterator and a bool result if the object was successfully added. I think chaining everything together would obfuscate the code and make it harder to read.

> 
> > Source/WebCore/rendering/RenderNamedFlowThread.h:83
> > +    bool canBeDestroyed() const { return m_regionList.isEmpty() && m_contentNodes.isEmpty(); }
> 
> Is this virtual?  Or specific to RenderNamedFlowThread?  What about when the whole RenderView tree goes down?  Will things correctly get cleaned up then?
This method is not virtual, it's specific to the RenderNamedFlowThread and self-describes what that condition means for the renderer.
I've thought this through and I can't see how this can have unpredictable results.
The render tree looks something like this:

RenderView
- Tree for normal flow
- RenderNamedFlowThread 1 - renderers for content nodes
- RenderNamedFlowThread 2 - renderers for content nodes
- etc

Before this patch, the RenderNamedFlowThread objects were destroyed when all the RenderTree was destroyed (e.g. when the Document is detached). Let's assume this is not buggy as no crash popped up until now :) (though I think there may be a situation that could cause a crash - not related to this change, I need to investigate a bit).
Now there's another case, when there are no content nodes or regions. The problems would've appear if it was possible to have a region child of it's own flow thread. However, this is not possible so I don't see any way how a crash can occur.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.